-
Notifications
You must be signed in to change notification settings - Fork 902
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
Do not attempt to send a response to a closed connection/stream #141
Merged
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
return; | ||
} | ||
|
||
if (!handledLastRequest) { | ||
addKeepAliveHeaders(res); | ||
ctx.write(res).addListener(CLOSE_ON_FAILURE); | ||
} else { | ||
// Note that it is perfectly fine not to set the 'content-length' header to the last response | ||
// of an HTTP/1 connection. We just set it to work around overly strict HTTP servers that always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTTP clients
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed.
b8f5050
to
78b12ac
Compare
Motivation: An HTTP/2 stream can be closed by various reasons, such as RST_STREAM and GOAWAY frame. If Armeria server attempts to write a response to a closed connection or a closed HTTP/2 stream, Netty's Http2Connection will reject it with an unexpected exception, such as: Http2Exception: Cannot create stream 1 since this endpoint has received a GOAWAY frame with last stream id 0 Http2Exception: Request stream N is not correct for server connection Http2Exception: Request stream ODD_N is not correct for server connection Modifications: - Make sure that the connection is still active and the HTTP/2 stream associated with the response is still not closed - Replace useHeadOfBlocking flag with http2conn field which becomes non-null when upgraded to HTTP/2 - Miscellaneous: - Remove redundant Future.isDone() check - Set the 'content-length' even if the current protocol is HTTP/1 and the response being written is the last response, to work around overly strict clients Result: Less mysterious yet noisy exceptions in the log
78b12ac
to
513b831
Compare
it also solves #140, LGTM! |
Related commit: 513b831
…Java identifier part Motivation: The default logger name of the service bound at '/200' becomes: armeria.services._00 which could have been: armeria.services._200
LGTM |
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.
Motivation:
An HTTP/2 stream can be closed by various reasons, such as RST_STREAM
and GOAWAY frame. If Armeria server attempts to write a response to a
closed connection or a closed HTTP/2 stream, Netty's Http2Connection
will reject it with an unexpected exception, such as:
Modifications:
associated with the response is still not closed
non-null when upgraded to HTTP/2
the response being written is the last response, to work around
overly strict clients
Result:
Less mysterious yet noisy exceptions in the log