Fixes #11370 - IllegalStateException when last write fails. #11439
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
Removed the call to `ServletChannel.abort()` from the write callback. As the write was issued from `ServletChannel.handle()` case COMPLETE, it was eventually calling `ServletChannelState.completed(Throwable)`, which is expecting the requestState to be COMPLETING. However, calling `abort()` would set the requestState to COMPLETED, causing the IllegalStateException. There should be no need to call `abort()` from the callback of failed writes, since failing the various callbacks should be enough, eventually failing the `HttpStream`, which would take care of tearing down the connection (HTTP/1) or the stream (HTTP/2+). Signed-off-by: Simone Bordet <[email protected]>
Now aborting the response from ServletChannelState.completed(Throwable). Fixed SizeLimitHandler exception message. Signed-off-by: Simone Bordet <[email protected]>
gregw
requested changes
Feb 23, 2024
| @@ -143,8 +143,8 @@ public void write(boolean last, ByteBuffer content, Callback callback) | |||
| { | |||
| if (_responseLimit >= 0 && (_written + content.remaining()) > _responseLimit) | |||
| { | |||
| callback.failed(new HttpException.RuntimeException(HttpStatus.INTERNAL_SERVER_ERROR_500, "Response body is too large: " + | |||
| _written + content.remaining() + ">" + _responseLimit)); | |||
| String message = "Response body is too large: %d > %d".formatted(_written + content.remaining(), _responseLimit); | |||
There was a problem hiding this comment.
You put spaces around the > sign. You need to fix the test to match
Fixed SizeLimitHandlerTest. Signed-off-by: Simone Bordet <[email protected]>
|
@gregw I reverted the renaming "aborted" -> "failed" because there are few public methods that are named abort* so felt weird to rename some (the private ones) but not others, so I kept "abort". |
gregw
approved these changes
Feb 26, 2024
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.
Removed the call to
ServletChannel.abort()from the write callback.As the write was issued from
ServletChannel.handle()case COMPLETE, it was eventually callingServletChannelState.completed(Throwable), which is expecting the requestState to be COMPLETING. However, callingabort()would set the requestState to COMPLETED, causing the IllegalStateException.There should be no need to call
abort()from the callback of failed writes, since failing the various callbacks should be enough, eventually failing theHttpStream, which would take care of tearing down the connection (HTTP/1) or the stream (HTTP/2+).