Fix retrying requests with body contents by resetting the reader index of ByteBuf #1281
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
argha-c
approved these changes
Aug 3, 2022
| @@ -178,6 +178,13 @@ public Iterable<HttpContent> getBodyContents() { | |||
| return Collections.unmodifiableList(bodyChunks); | |||
| } | |||
|
|
|||
| @Override | |||
| public void resetBodyReader() { | |||
| for (final HttpContent chunk : bodyChunks) { | |||
There was a problem hiding this comment.
Is it worth only resetting this when we know we have a body, i.e. effectively setBody has been called?
There was a problem hiding this comment.
Looking at the soft contract we don't set the hasBody boolean in the setBody method. So going to not check that here as the hasBody doesn't determine any logic within the message impl currently.
|
LGTM! This is a huge improvement over the hack we put in back in 2018. |
artgon
reviewed
Aug 3, 2022
| @@ -896,6 +846,10 @@ protected void handleOriginNonSuccessResponse(final HttpResponse originResponse, | |||
| startedSendingResponseToClient, zuulRequest.hasCompleteBody(), zuulRequest.getMethod()); | |||
| //detach from current origin. | |||
| unlinkFromOrigin(); | |||
|
|
|||
| // ensure body reader indexes are reset so retry is able to access the body buffer | |||
There was a problem hiding this comment.
Worth clarifying in this comment that we want Netty to have access to that ByteBuf, rather than Zuul itself.
artgon
approved these changes
Aug 3, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For TLS handled connections, once the request body has been written to the channel, the ByteBuf.readerIndex is mutated to point to the end of the buffer. This means when retrying requests, subsequent fetches of that buffer return no bytes (as they use the readerIndex). This only affects TLS connections as the SSL handler in netty mutates those indexes (doesn't appear to be the case with plaintext).
Original issue raised with netty from 2018 (netty/netty#7753) eluded to this fact before the changes to pre-buffer bodies was put into zuul (i.e. pre-buffering on 503s that was introduced hid this actual issue of the indexes needing to be reset).
This change resets the readerIndex before retrying a request, and removes the buffering of bodies in retry scenarios.