Avoid leaking request contexts from server-side#6166
Merged
Conversation
Contributor
minwoox
approved these changes
Mar 19, 2025
Contributor
minwoox
left a comment
minwoox
left a comment
There was a problem hiding this comment.
The new approach looks nice. 👍
|
|
||
| if (responseEncoder instanceof ServerHttp2ObjectEncoder) { | ||
| ((ServerHttp2ObjectEncoder) responseEncoder) | ||
| .maybeResetStream(req.streamId(), Http2Error.CANCEL); |
Contributor
There was a problem hiding this comment.
question:
Can we do this only when needsDisconnection is false?
ikhoon
pushed a commit
to ikhoon/armeria
that referenced
this pull request
Apr 8, 2025
Motivation: It has been pointed out that server-side may hold strong references to the `ServiceRequestContext` in line#6108. In general, a server-side `HttpRequest` may be closed either 1) by the client 2) or by the user. When the request is closed by the client, the connection or stream state is reflected to the `HttpRequest` - hence the two are in sync. However, if the `HttpRequest` is closed by the user the state is not always reflected to the connection or stream. For instance, setting `ServerBuilder#requestAutoAbortDelayMillis` will close the request but not the underlying connection or stream. The handling of `ContentTooLargeException` attempts to handle this issue. However, if `setShouldResetOnlyIfRemoteIsOpen` is set after the response is fully written, then the underlying stream may still never be closed. https://github.com/line/armeria/blob/6dbed576fe8f0774f445716c74c01c7572799ee6/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseSubscriber.java#L389 **Proposal** I propose the following: - HTTP1: - If there is a protocol failure, the `Http1RequestDecoder` is responsible for closing the connection - Otherwise, the underlying stream will be completed once the remote completes the request. Because pipelining is rarely used, HTTP1 connections will have a constraint of 1 concurrent request which probably won't impose a memory burden. - HTTP2: - Once the `HttpRequest`/`HttpResponse` is completed, the underlying stream state is checked. If the underlying stream is not closed yet, a `RST_STREAM` is sent to sync the stream state with the `HttpRequest`/`HttpResponse` state. - Note that this behavior relies on the fact that `Http2Stream#State` is updated synchronously when `Http2FrameWriter#writeHeaders` is called. Modifications: - Introduced `ServerHttp2ObjectEncoder#maybeResetStream` which sends a `RST_STREAM` if the underlying state is not complete. - `ServerHttp2ObjectEncoder#maybeResetStream` is called when the protocol is HTTP2 and the request/response is complete. - Removed `resetOnlyIfRemoteIsOpen` since `maybeResetStream` replaces this functionality. Result: - Users no longer observe memory pressure due to incomplete HTTP2 streams.
jrhee17
added a commit
that referenced
this pull request
Jun 20, 2025
…6279) Motivation: The PR #6166 modified server-side behavior to send a `RST_STREAM` frame when the corresponding Armeria's `HttpRequest` and `HttpResponse` are completed. The assumption was that since the user was done using the `HttpRequest` and `HttpResponse`, the underlying stream can be cleaned up. https://github.com/line/armeria/blob/a4bcb3e3b2a90a41b0066601d97da2fb04af5347/core/src/main/java/com/linecorp/armeria/server/ServerHttp2ObjectEncoder.java#L146-L149 The code was written to check Netty's `Http2Stream` before sending a `RST_STREAM` - this is because sending a `endStream` would close the underlying stream anyways regardless of whether a `RST_STREAM` is sent. However, when frames are flow controlled, `Http2Stream`'s state is updated after armeria's post-response future is invoked. To be more specific, it seems like Netty's implementation updates the state by adding a listener on the `write*` future when the flow controlled frame is completed. This is awkward because Armeria also completes the `HttpResponse` when a write future with `endStream` is completed. 1. Armeria layer: Write `endStream` and wait on the write future before invoking `res.close` https://github.com/line/armeria/blob/a4bcb3e3b2a90a41b0066601d97da2fb04af5347/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseSubscriber.java#L397-L417 2. `RequestAndResponseCompleteHandler` is completed https://github.com/line/armeria/blob/a4bcb3e3b2a90a41b0066601d97da2fb04af5347/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java#L847-L848 4. `Http2Stream.state` is updated (the future is invoked after Armeria's) https://github.com/netty/netty/blob/a919dd3a53a50da080dc925a6fb085c81cab6294/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java#L629 Note that there is no issue when receiving requests since the state is updated before the `Http2FrameListener` is invoked. i.e. https://github.com/netty/netty/blob/a919dd3a53a50da080dc925a6fb085c81cab6294/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java#L257 I propose that we track a separate state which determines whether an `endStream` was sent. This value can be used to determine whether a separate `RST_STREAM` should be sent to close the underlying stream. Modifications: - Added a `endStreamSentKey` property key which indicates whether an `endStream` was sent for a `Http2Stream`. - Check if `endStreamSentKey` was set when determining whether `RST_STREAM` should be used to clean up the underlying stream. - To determine if the local side (server->client) is open, both `Http2Stream.State.localSideOpen() == true` and `endStreamSentKey == false` must be satisfied. - If the remote is open (client->server), then the `RST_STREAM` is sent regardless. - For client-side, a late `RST_STREAM` is always logged if the response is closed. If the stream is active, then the `RST_STREAM` was sent to close the stream and hence there is no need to log with WARN level. - Added `SimpleHttp2Connection` to easily send and receive HTTP2 frames directly from test code Result: - An unecessary `RST_STREAM` is not sent after the server sends an `endStream`. <!-- 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 -->
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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:
It has been pointed out that server-side may hold strong references to the
ServiceRequestContextin #6108.In general, a server-side
HttpRequestmay be closed either 1) by the client 2) or by the user.When the request is closed by the client, the connection or stream state is reflected to the
HttpRequest- hence the two are in sync.However, if the
HttpRequestis closed by the user the state is not always reflected to the connection or stream.For instance, setting
ServerBuilder#requestAutoAbortDelayMilliswill close the request but not the underlying connection or stream.The handling of
ContentTooLargeExceptionattempts to handle this issue. However, ifsetShouldResetOnlyIfRemoteIsOpenis set after the response is fully written, then the underlying stream may still never be closed.armeria/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseSubscriber.java
Line 389 in 6dbed57
Proposal
I propose the following:
Http1RequestDecoderis responsible for closing the connectionHttpRequest/HttpResponseis completed, the underlying stream state is checked. If the underlying stream is not closed yet, aRST_STREAMis sent to sync the stream state with theHttpRequest/HttpResponsestate.Http2Stream#Stateis updated synchronously whenHttp2FrameWriter#writeHeadersis called.Modifications:
ServerHttp2ObjectEncoder#maybeResetStreamwhich sends aRST_STREAMif the underlying state is not complete.ServerHttp2ObjectEncoder#maybeResetStreamis called when the protocol is HTTP2 and the request/response is complete.resetOnlyIfRemoteIsOpensincemaybeResetStreamreplaces this functionality.Result: