Fixes #8405 - onAllDataRead() is called twice under h2 if the stream … #10174
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
…times out Per Servlet semantic, HTTP/2 stream timeout should be ignored. The code was trying to fail the read via `_contentDemander.onTimeout()`, but then it was still calling `onContentProducible()`, which was returning `true` because the state of the read was IDLE (all the request content was read) and the request was suspended. Now the code checks if the read was really failed; if it is not, then `onContentProducible()` is not called and so the idle timeout is ignored. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
gregw
requested changes
Jul 31, 2023
| boolean needed = getRequest().getHttpInput().onContentProducible(); | ||
| boolean readFailed = _contentDemander.onTimeout(failure); | ||
| boolean needed = false; | ||
| if (readFailed) |
There was a problem hiding this comment.
I think a simple && would be clear here.
Needed = on timeout && contentProducable
There was a problem hiding this comment.
@gregw I disagree that it would be clearer to lose the variable names and rely on && short circuiting.
I have renamed needed to handle.
There was a problem hiding this comment.
Nah! You can keep the variable name but get rid of the manual short-circuit:
boolean = readFailed && getRequest().getHttpInput().onContentProducible();
lorban
previously approved these changes
Jul 31, 2023
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
gregw
approved these changes
Jul 31, 2023
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.
…times out
Per Servlet semantic, HTTP/2 stream timeout should be ignored.
The code was trying to fail the read via
_contentDemander.onTimeout(), but then it was still callingonContentProducible(), which was returningtruebecause the state of the read was IDLE (all the request content was read) and the request was suspended.Now the code checks if the read was really failed; if it is not, then
onContentProducible()is not called and so the idle timeout is ignored.