Handle remote peer exceptions by ERogalla · Pull Request #2520 · palantir/conjure-java

ERogalla · 2025-04-23T14:47:05Z

Before this PR

We have experienced many IO exceptions originating from this undertow message.

After this PR

Don't rethrow on this error.

pkoenig10 · 2025-04-23T15:10:29Z

...rtow-runtime/src/main/java/com/palantir/conjure/java/undertow/runtime/ConjureExceptions.java

+                && (!exchange.getConnection().isOpen()
+                        || throwable.equals(UndertowMessages.MESSAGES.couldNotReadContentLengthData()))) {


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.

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.

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

ERogalla · 2025-04-25T14:13:43Z

...rtow-runtime/src/main/java/com/palantir/conjure/java/undertow/runtime/ConjureExceptions.java

+                .equals(UndertowMessages.MESSAGES
+                        .couldNotReadContentLengthData()
+                        .getMessage())) {
+            log.info(


Does it make sense to not send an error here? Or should it be a 400 error.
For reference, this error message is pulled directly from UndertowMessages

We could not read the full request from the client and client has closed its side of the connection so we can't send a response, but we need to close and clean up our exchange.

schlosna

I'd suggest #2523 on top of this PR

schlosna · 2025-04-26T19:22:58Z

...rtow-runtime/src/main/java/com/palantir/conjure/java/undertow/runtime/ConjureExceptions.java

+        } else if (throwable instanceof IOException) {
+            ioException(exchange, (IOException) throwable);


nit: can use instanceof pattern variable to avoid cast:

Suggested change

} else if (throwable instanceof IOException) {

ioException(exchange, (IOException) throwable);

} else if (throwable instanceof IOException ioException) {

ioException(exchange, ioException);

schlosna · 2025-04-26T19:29:57Z

...rtow-runtime/src/main/java/com/palantir/conjure/java/undertow/runtime/ConjureExceptions.java

+            log.info(
+                    "Remote peer closed connection before all data could be read. The request may have been aborted by"
+                            + " the client",
+                    ioException);


Suggested change

ioException);

ioException);

IoUtils.safeClose(exchange.getConnection());

schlosna · 2025-04-26T19:37:50Z

...rtow-runtime/src/main/java/com/palantir/conjure/java/undertow/runtime/ConjureExceptions.java

+        if (ioException
+                .getMessage()
+                .equals(UndertowMessages.MESSAGES
+                        .couldNotReadContentLengthData()
+                        .getMessage())) {


ioException.getMessage() could return null leading to a NullPointerException on this error handling path, so we need to handle that.

Additionally, UndertowMessages.MESSAGES.couldNotReadContentLengthData() creates a new IOException on each invocation and adds the expensive overhead of Throwable#fillInStackTrace(). We should resolve the message once to a static final field.

I've pushed #2523 as suggested changes on top of this branch

Suggested change

if (ioException

.getMessage()

.equals(UndertowMessages.MESSAGES

.couldNotReadContentLengthData()

.getMessage())) {

if (Objects.equals(ioException.getMessage(), COULD_NOT_READ_CONTENT_LENGTH_MESSAGE)) {

schlosna · 2025-04-26T19:39:17Z

...rtow-runtime/src/main/java/com/palantir/conjure/java/undertow/runtime/ConjureExceptions.java

+                .equals(UndertowMessages.MESSAGES
+                        .couldNotReadContentLengthData()
+                        .getMessage())) {
+            log.info(


We could not read the full request from the client and client has closed its side of the connection so we can't send a response, but we need to close and clean up our exchange.

schlosna · 2025-04-26T19:41:44Z

...me/src/test/java/com/palantir/conjure/java/undertow/runtime/ConjureExceptionHandlerTest.java

+    public void handlesPeerRemoteExceptionsWithoutRethrowing() throws IOException {
+        exception = UndertowMessages.MESSAGES.couldNotReadContentLengthData();
+        HttpURLConnection connection = execute();
+        assertThat(connection.getResponseCode()).isEqualTo(200);


there shouldn't be a response code if the request wasn't fully read and we threw before processing & writing response

Suggested change

assertThat(connection.getResponseCode()).isEqualTo(200);

assertThatThrownBy(() -> connection.getResponseCode())

.isInstanceOf(SocketException.class)

.hasMessageContaining("Unexpected end of file from server");

pkoenig10 · 2025-04-27T00:43:04Z

...rtow-runtime/src/main/java/com/palantir/conjure/java/undertow/runtime/ConjureExceptions.java

                exception, exchange, UndertowDeadlineReasonResponseEncodingAdapter.INSTANCE);
    }

+    private static void ioException(HttpServerExchange exchange, IOException ioException) {


nit: Move this below error to match the ordering of the cases above.

carterkozak · 2025-05-05T14:20:27Z

Can you help me understand the core problem that we’re solving? The existing exception itself isn’t unreasonable imo, but it could certainly be handled better. Is that signal causing monitors to fire?

I’m a bit worried that we’re matching a specific failure where we should probably look for a class of failures, as there are other ways that requests can bail out depending on the transport encoding.

ERogalla · 2025-05-05T18:13:55Z

This was not causing any monitors to fire, but was just spamming error service logs. I was hoping to clear out any noisy/incorrect error logs that aren't actual errors. We can instruct our team to ignore these errors, but it is occurring on seemingly random service endpoints.

FWIW I'm totally open to close this PR out if this isn't the root cause. I just assumed that this was just some unhandled (but expected) exception.

carterkozak · 2025-05-05T19:44:38Z

In that case, I'd attempt to cast a more precise net: Instead of updating our exception handling to check messages, we may prefer to wrap the request body conduit/channel such that we can intercept I/O exceptions while reading data and handle them more precisely without impacting the flow of exceptions thrown within service implementations.

* Close exchange on closed client * Apply suggestions from code review Co-authored-by: Patrick Koenig <pkoenig10@gmail.com> --------- Co-authored-by: Patrick Koenig <pkoenig10@gmail.com>

stale · 2025-06-27T04:37:29Z

This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days.

handle remote peer exception

2328df4

ERogalla added merge when ready no changelog labels Apr 23, 2025

pkoenig10 reviewed Apr 23, 2025

View reviewed changes

ERogalla added 3 commits April 25, 2025 09:58

Update ConjureExceptions.java

15c878b

Update ConjureExceptions.java

6c2ad3a

fix check and test

2081d70

ERogalla commented Apr 25, 2025

View reviewed changes

ERogalla requested review from carterkozak and pkoenig10 April 25, 2025 14:13

schlosna mentioned this pull request Apr 26, 2025

Close exchange on closed client #2523

Merged

schlosna reviewed Apr 26, 2025

View reviewed changes

pkoenig10 reviewed Apr 27, 2025

View reviewed changes

Close exchange on closed client (#2523)

dda2d3d

* Close exchange on closed client * Apply suggestions from code review Co-authored-by: Patrick Koenig <pkoenig10@gmail.com> --------- Co-authored-by: Patrick Koenig <pkoenig10@gmail.com>

stale bot added the stale label Jun 27, 2025

stale bot closed this Oct 18, 2025

		&& (!exchange.getConnection().isOpen()
		\|\| throwable.equals(UndertowMessages.MESSAGES.couldNotReadContentLengthData()))) {

		} else if (throwable instanceof IOException) {
		ioException(exchange, (IOException) throwable);

	ioException);
	ioException);
	IoUtils.safeClose(exchange.getConnection());

-        assertThat(connection.getResponseCode()).isEqualTo(200);
+        assertThatThrownBy(() -> connection.getResponseCode())
+                .isInstanceOf(SocketException.class)
+                .hasMessageContaining("Unexpected end of file from server");

Conversation

ERogalla commented Apr 23, 2025

Before this PR

After this PR

Uh oh!

pkoenig10 Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlosna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carterkozak commented May 5, 2025

Uh oh!

ERogalla commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carterkozak commented May 5, 2025

Uh oh!

stale bot commented Jun 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pkoenig10 Apr 23, 2025 •

edited

Loading

ERogalla commented May 5, 2025 •

edited

Loading