Avoid leaking request contexts from server-side #6166
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
minwoox
approved these changes
Mar 19, 2025
| @@ -842,6 +843,11 @@ private void handleRequestOrResponseComplete() { | |||
| unfinishedRequests.remove(req); | |||
| } | |||
|
|
|||
| if (responseEncoder instanceof ServerHttp2ObjectEncoder) { | |||
| ((ServerHttp2ObjectEncoder) responseEncoder) | |||
| .maybeResetStream(req.streamId(), Http2Error.CANCEL); | |||
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.
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:
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: