Skip to content

Handle remote peer exceptions #2520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ERogalla wants to merge 5 commits into from
Closed

Handle remote peer exceptions #2520

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2328df4
handle remote peer exception
ERogalla Apr 23, 2025
15c878b
Update ConjureExceptions.java
ERogalla Apr 25, 2025
6c2ad3a
Update ConjureExceptions.java
ERogalla Apr 25, 2025
2081d70
fix check and test
ERogalla Apr 25, 2025
dda2d3d
Close exchange on closed client (#2523)
schlosna May 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion ...w-runtime/src/main/java/com/palantir/conjure/java/undertow/runtime/ConjureExceptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.palantir.logsafe.SafeArg;
import com.palantir.logsafe.logger.SafeLogger;
import com.palantir.logsafe.logger.SafeLoggerFactory;
import io.undertow.UndertowMessages;
import io.undertow.io.UndertowOutputStream;
import io.undertow.server.HttpServerExchange;
import io.undertow.util.Headers;
Expand Down Expand Up @@ -79,7 +80,9 @@ public void handle(HttpServerExchange exchange, Throwable throwable) {
deadlineExpiredException(exchange, (DeadlineExpiredException) throwable);
} else if (throwable instanceof Error) {
error(exchange, (Error) throwable);
} else if (throwable instanceof IOException && !exchange.getConnection().isOpen()) {
} else if (throwable instanceof IOException
&& (!exchange.getConnection().isOpen()
|| throwable.equals(UndertowMessages.MESSAGES.couldNotReadContentLengthData()))) {
Copy link
Copy Markdown
Member

@pkoenig10 pkoenig10 Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This should probably be pulled out to a helper method so we can give it a descriptive name and avoid cluttering this existing instanceof casing.

Copy link
Copy Markdown
Contributor

@carterkozak carterkozak Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this works, I'd suggest taking another pass at the test to see what's happening in practice!

Creating a new IOException + gathering its stack trace every time we catch any sort of IOException isn't ideal.

Copy link
Copy Markdown
Author

@ERogalla ERogalla Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. @carterkozak you're right. The original test I added did not trigger the added codepath.

log.info(
"I/O exception from a closed connection. The request may have been aborted by the client",
throwable);
Expand Down
12 changes: 12 additions & 0 deletions ...src/test/java/com/palantir/conjure/java/undertow/runtime/ConjureExceptionHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import com.palantir.deadlines.DeadlineExpiredException;
import com.palantir.logsafe.SafeArg;
import io.undertow.Undertow;
import io.undertow.UndertowMessages;
import io.undertow.server.HttpHandler;
import io.undertow.server.handlers.BlockingHandler;
import java.io.ByteArrayOutputStream;
Expand Down Expand Up @@ -361,6 +362,17 @@ public void handlesErrorWithoutRethrowing() {
.doesNotThrowAnyException();
}

@Test
public void handlesPeerRemoteExceptionsWithoutRethrowing() {
HttpHandler handler = new ConjureExceptionHandler(
_exchange -> {
throw UndertowMessages.MESSAGES.couldNotReadContentLengthData();
},
ConjureExceptions.INSTANCE);
assertThatCode(() -> handler.handleRequest(HttpServerExchanges.createStub()))
.doesNotThrowAnyException();
}

private static String getErrorBody(HttpURLConnection connection) {
try (InputStream response = connection.getErrorStream()) {
return new String(response.readAllBytes(), StandardCharsets.UTF_8);
Expand Down