-
Notifications
You must be signed in to change notification settings - Fork 965
Fix a bug where closing a websocket session could send a RST_STREAM
frame
#6375
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6375 +/- ##
============================================
- Coverage 74.46% 74.09% -0.37%
- Complexity 22234 22984 +750
============================================
Files 1963 2061 +98
Lines 82437 86105 +3668
Branches 10764 11306 +542
============================================
+ Hits 61385 63802 +2417
- Misses 15918 16893 +975
- Partials 5134 5410 +276 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1b73e7f
to
9cdd41d
Compare
core/src/main/java/com/linecorp/armeria/server/Http2StreamLifecycleHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/Http2StreamLifecycleHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/Http2StreamLifecycleHandler.java
Outdated
Show resolved
Hide resolved
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.
Looks nice. 👍
.requestTimeoutMillis(WebSocketUtil.DEFAULT_REQUEST_RESPONSE_TIMEOUT_MILLIS) | ||
.maxRequestLength(WebSocketUtil.DEFAULT_MAX_REQUEST_RESPONSE_LENGTH) | ||
.requestAutoAbortDelayMillis(WebSocketUtil.DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS) | ||
.closeHttp2StreamDelayMillis(10_000) // follows netty's forceCloseTimeoutMillis default |
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.
Could you put this value in WebSocketUtil?
Could you update the Javadoc?
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.
👍 👍
Motivation:
Currently, once an
HttpRequest
andHttpResponse
is completed, the underlying HTTP2 stream is cancelled using aRST_STREAM
.This makes sense for normal HTTP constructs since it indicates that we are no longer interested in the request, and we would like to release resources associated with it.
However, some protocols such as WebSockets implement their own graceful shutdown procedure.
In detail, Armeria's
HttpRequest
,HttpResponse
implements WebSocket graceful shutdown and the reactive stream implementation is closed when aCLOSE
frame is both sent and received.However, although the websocket session is completed, the underlying HTTP2 stream may not necessarily be complete.
Protocol-wise, there is an inherent discrepancy between websocket session completion and HTTP2 stream completion.
The current implementation defaults to sending a
RST_STREAM
immediately once the correspondingHttpRequest
andHttpResponse
. For websockets, I propose that a delay is given so that the remote has a chance to end the stream.Assuming that we will tie the lifecycle of inbound
WebSocket
s with outboundWebSocket
s (so sending a close frame also closes the inbound) in #6357 , this option can be thought of similar to netty'sforceCloseTimeoutMillis
option. (which acts as a timeout since sending the CLOSE frame)Modifications:
closeHttp2StreamDelayMillis
option toServiceConfig
and relevant implementationsWebSocketService
sets acloseHttp2StreamDelayMillis
of 10 seconds by defaultHttpServerHandler
decides when to send aRST_STREAM
based on thecloseHttp2StreamDelayMillis
Http2StreamLifecycleHandler
which maintains the lifecycle of reset futures. This ensures that scheduled futures aren't leaked for servers with high throughput.maybeResetStream
is called.notifyStreamClosed
is called to clean up possibly scheduled futures.Result:
RST_STREAM
frame