-
Notifications
You must be signed in to change notification settings - Fork 966
Add streamTimeout option to WebSocketService/WebSocketClient; send close frame on inbound failure; clean up related resources #6357
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
base: main
Are you sure you want to change the base?
Conversation
…uestLog, ensure close frame on cancel/abort; add tests (H2C close-frame/log, H1 channel close)
…utbound, closed outbound when inbound ends, introduced InboundCompleteException, and added a client-side newCloseWebSocketFrame() in WebSocketUtil.
jrhee17
left a comment
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 good overall, left some comments
core/src/main/java/com/linecorp/armeria/common/stream/TimeoutStreamMessage.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/websocket/WebSocketServiceBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketSession.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketSession.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketSession.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketSession.java
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6357 +/- ##
============================================
- Coverage 74.46% 74.16% -0.30%
- Complexity 22234 23035 +801
============================================
Files 1963 2064 +101
Lines 82437 86188 +3751
Branches 10764 11317 +553
============================================
+ Hits 61385 63922 +2537
- Misses 15918 16859 +941
- Partials 5134 5407 +273 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fix: In WebSocketSession endRequest -> requestCause
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketSession.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/TimeoutStreamMessage.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/TimeoutStreamMessage.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/internal/common/websocket/WebSocketUtil.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/internal/server/websocket/DefaultWebSocketService.java
Outdated
Show resolved
Hide resolved
… frame (#6375) Motivation: Currently, once an `HttpRequest` and `HttpResponse` is completed, the underlying HTTP2 stream is cancelled using a `RST_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 a `CLOSE` 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 corresponding `HttpRequest` and `HttpResponse`. 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 outbound `WebSocket`s (so sending a close frame also closes the inbound) in #6357 , this option can be thought of similar to netty's `forceCloseTimeoutMillis` option. (which acts as a timeout since sending the CLOSE frame) Modifications: - Added a `closeHttp2StreamDelayMillis` option to `ServiceConfig` and relevant implementations - `WebSocketService` sets a `closeHttp2StreamDelayMillis` of 10 seconds by default - `HttpServerHandler` decides when to send a `RST_STREAM` based on the `closeHttp2StreamDelayMillis` - Added `Http2StreamLifecycleHandler` which maintains the lifecycle of reset futures. This ensures that scheduled futures aren't leaked for servers with high throughput. - Every time a request/response is closed but the corresponding stream is alive, `maybeResetStream` is called. - Every time a stream is closed, `notifyStreamClosed` is called to clean up possibly scheduled futures. Result: - Users can set a timeout for closing a websocket session - Fix a bug where closing a websocket session could send a `RST_STREAM` frame <!-- Visit this URL to learn more about how to write a pull request description: https://armeria.dev/community/developer-guide#how-to-write-pull-request-description -->
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketClientBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketSession.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/logging/RequestLogBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/TimeoutStreamMessage.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/StreamMessage.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/internal/server/websocket/DefaultWebSocketService.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/client/websocket/WebSocketClientStreamTimeoutTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/client/websocket/WebSocketClientStreamTimeoutTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/client/websocket/WebSocketClientStreamTimeoutTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/client/websocket/WebSocketClientStreamTimeoutTest.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/InboundCompleteException.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/stream/TimeoutStreamMessage.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/websocket/WebSocketIdleTimeoutException.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketSession.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/internal/server/websocket/DefaultWebSocketService.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/client/websocket/WebSocketClientStreamTimeoutTest.java
Outdated
Show resolved
Hide resolved
ikhoon
left a comment
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.
Nice work, @sjy982! 🚀 🎉
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketClientBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketSession.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/internal/server/websocket/DefaultWebSocketService.java
Outdated
Show resolved
Hide resolved
…cketClientBuilder.java Co-authored-by: Ikhun Um <[email protected]>
…cketSession.java Co-authored-by: Ikhun Um <[email protected]>
…ket/DefaultWebSocketService.java Co-authored-by: Ikhun Um <[email protected]>
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketSession.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/InboundCompleteException.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/websocket/WebSocketClientBuilder.java
Show resolved
Hide resolved
| if (cause instanceof StreamTimeoutException) { | ||
| wrapped = new WebSocketIdleTimeoutException("WebSocket inbound idle-timeout exceeded", | ||
| cause); | ||
| } else { | ||
| wrapped = new InboundCompleteException("inbound stream was cancelled", cause); | ||
| } |
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.
Question) Is it possible to just use InboundCompleteException to indicate the outbound stream was cancelled by the inbound stream? If users would like to differentiate the reason, they could just check InboundCompleteException#cause?
| if (cause instanceof StreamTimeoutException) { | |
| wrapped = new WebSocketIdleTimeoutException("WebSocket inbound idle-timeout exceeded", | |
| cause); | |
| } else { | |
| wrapped = new InboundCompleteException("inbound stream was cancelled", cause); | |
| } | |
| wrapped = new WebSocketIdleTimeoutException("WebSocket inbound idle-timeout exceeded", cause); |
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.
If we apply the code you suggested, non-timeout exceptions would also be wrapped with WebSocketIdleTimeoutException.
Or were you suggesting that we should instead use only InboundCompleteException as the single wrapper and let callers distinguish the concrete exception via its cause?
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.
Or were you suggesting that we should instead use only InboundCompleteException as the single wrapper and let callers distinguish the concrete exception via its cause?
Right, I understood that the meaning of InboundCompleteException is that "the outbound was cancelled due to the inbound".
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.
The reason we use WebSocketIdleTimeoutException is that the existing StreamTimeoutException may not clearly convey its meaning to WebSocket users. That’s why it was introduced, and since it already makes the intention explicit, we didn’t wrap it again with InboundCompleteException.
Do you think it would be better to wrap it once more using InboundCompleteException and include it as the cause?
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.
I realized that I copy-pasted the incorrect exception in the suggestion above, I meant to modify the above block to:
wrapped = new InboundCompleteException("inbound stream was cancelled", cause);
because WebSocketIdleTimeoutException seems like a very specific exception that likely won't be used anywhere else - less exception types users have to handle/worry about
If you prefer to introduce the above exception, feel free to leave as-is as this is a nit comment.
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.
Let me elaborate on why WebSocketIdleTimeoutException was added.
Many WebSocket implementations disable request timeouts and instead detect and close idle sessions. Since this behavior is quite common for users working with WebSockets, I thought it would be better to introduce a more specific error type to convey the state clearly.
core/src/main/java/com/linecorp/armeria/common/websocket/WebSocketIdleTimeoutException.java
Outdated
Show resolved
Hide resolved
…Exception.java Co-authored-by: jrhee17 <[email protected]>
…cketIdleTimeoutException.java Co-authored-by: jrhee17 <[email protected]>
Motivation
StreamMessage.timeout(..., UNTIL_NEXT), expose it as an option onWebSocketServiceBuilder/WebSocketClientBuilderso both server and client can enable it consistently and easily.Modifications
Add
WebSocketServiceBuilder#streamTimeout(Duration).TimeoutStreamMessagewithStreamTimeoutMode.UNTIL_NEXTinsideDefaultWebSocketService#serve(...).Update
DefaultWebSocketService#serve(...):outbound.abort(mappedCause)sorecoverAndResumesends aCloseWebSocketFrame.CancelledSubscriptionException/AbortedStreamExceptiontoInboundCompleteExceptionto avoid being skipped by the recovery logic.Add
WebSocketClientBuilder#streamTimeout(Duration).TimeoutStreamMessagewithStreamTimeoutMode.UNTIL_NEXTinsideDefaultWebSocketClient#connect(...).Update
WebSocketSession#setOutbound(...):recoverAndResume(...)to outbound so that on send failure it emits aCloseWebSocketFrameand records the exception inRequestLog(endRequest(cause)/endResponse(cause)). ForClosedStreamException, do not send a close frame—just abort.abort(...)outbound with the mapped cause so the recovery stream kicks in.Add
InboundCompleteException extends CancellationException:Add a client-side
newCloseWebSocketFrame(Throwable)inWebSocketUtil.Result
WebSocketService/WebSocketClientcan applyStreamMessage.timeout(..., UNTIL_NEXT)to inbound via the simplestreamTimeout(Duration)option.ClosedStreamException).RequestLog.