Skip to content
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.

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

UNDERTOW-2017 debugging #1287

Conversation

carterkozak
Copy link
Contributor

It appears that transitions between states fail to store
relevant data -- this is difficult to reproduce locally because
tests use loopback or otherwise fast networks. By introducing
small buffers and writes that often make no progress, we can
reproduce issues from slower networks.

It appears that transitions between states fail to store
relevant data -- this is difficult to reproduce locally because
tests use loopback or otherwise fast networks. By introducing
small buffers and writes that often make no progress, we can
reproduce issues from slower networks.
Comment on lines 260 to 263
HttpServerExchange exchangeSnapshot = exchange;
if (exchangeSnapshot != null) {
IoUtils.safeClose(exchangeSnapshot.getConnection());
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I observed another NPE here, I will cherry-pick this into a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linking that PR here for posterity: #1289

@@ -435,4 +456,8 @@ public void failed(IOException e) {
};
}

public static ByteBufferPool getBufferPool() {
return new DefaultByteBufferPool(true, 50, 0, 0, 100);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small buffers result in more frequent write attempts

if (res == 0) {
this.headerName = headerName;
this.valueIterator = valueIterator;
this.nameIterator = nameIterator;
log.trace("Continuation");
return STATE_HDR_EOL_CR;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The transitions to STATE_HDR_EOL_LF and STATE_HDR_EOL_CR require additional data.

I might suggest saving all state prior to every transition to avoid this kind of bug in the future, however it would require more operations per invocation. That said, in the common case I'd expect the request line and all headers to fit into a single buffer, so it's likely there's no real cost.

@fl4via
Copy link
Member

fl4via commented Jan 20, 2022

Thanks for sharing all this background for testing of UNDERTOW-2017 fix @carterkozak !

@fl4via
Copy link
Member

fl4via commented Feb 6, 2022

Closing this PR that was created for analysis and reproducing of the UNDERTOW-2017. The bug fix, at #1288, is merged.

@fl4via fl4via closed this Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants