[jrhee17]

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.

armeria/core/src/main/java/com/linecorp/armeria/server/ServerHttp2ObjectEncoder.java

Lines 146 to 149 in a4bcb3e

	if (stream.state().localSideOpen() || stream.state().remoteSideOpen()) {
	encoder().writeRstStream(ctx(), streamId, http2Error.code(), ctx().voidPromise());
	ctx().flush();
	}

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

armeria/core/src/main/java/com/linecorp/armeria/server/AbstractHttpResponseSubscriber.java

Lines 397 to 417 in a4bcb3e

	WriteHeadersFutureListener writeHeadersFutureListener(boolean endOfStream) {
	if (!endOfStream) {
	// Reuse in case sending multiple informational headers.
	if (cachedWriteHeadersListener == null) {
	cachedWriteHeadersListener = new WriteHeadersFutureListener(false);
	}
	return cachedWriteHeadersListener;
	}
	return new WriteHeadersFutureListener(true);
	}
	WriteDataFutureListener writeDataFutureListener(boolean endOfStream, boolean wroteEmptyData) {
	if (!endOfStream && !wroteEmptyData) {
	// Reuse in case sending streaming data.
	if (cachedWriteDataListener == null) {
	cachedWriteDataListener = new WriteDataFutureListener(false, false);
	}
	return cachedWriteDataListener;
	}
	return new WriteDataFutureListener(endOfStream, wroteEmptyData);
	}

2. RequestAndResponseCompleteHandler is completed

armeria/core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java

Lines 847 to 848 in a4bcb3e

	((ServerHttp2ObjectEncoder) responseEncoder)
	.maybeResetStream(req.streamId(), Http2Error.CANCEL);

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.

[codecov]

Codecov Report

Attention: Patch coverage is 68.57143% with 22 lines in your changes missing coverage. Please review.

Project coverage is 74.78%. Comparing base (8150425) to head (bfde2e9).
Report is 104 commits behind head on main.

Files with missing lines	Patch %	Lines
.../internal/testing/netty/SimpleHttp2Connection.java	74.35%	9 Missing and 1 partial ⚠️
...ia/internal/testing/netty/HttpFrameAggregator.java	58.33%	4 Missing and 1 partial ⚠️
...nal/testing/netty/Http2ClientFrameInitializer.java	71.42%	2 Missing and 2 partials ⚠️
.../linecorp/armeria/client/Http2ResponseDecoder.java	33.33%	1 Missing and 1 partial ⚠️
...armeria/internal/testing/NettyServerExtension.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6279      +/-   ##
============================================
+ Coverage     74.46%   74.78%   +0.32%     
- Complexity    22234    22474     +240     
============================================
  Files          1963     1985      +22     
  Lines         82437    83013     +576     
  Branches      10764    10779      +15     
============================================
+ Hits          61385    62082     +697     
+ Misses        15918    15764     -154     
- Partials       5134     5167      +33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

🔍 Build Scan® (commit: bfde2e9)

Job name	Status	Build Scan®
build-ubicloud-standard-16-jdk-8	❌ (failure)	https://ge.armeria.dev/s/qmxseb2ptfuto
build-ubicloud-standard-16-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/yn4zv7dbtwvhy
build-ubicloud-standard-16-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/klwwcujfizzvi
build-ubicloud-standard-16-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/j4jqjfncuqc5c
build-ubicloud-standard-16-jdk-17-leak	✅	https://ge.armeria.dev/s/dxsauhdphlnug
build-ubicloud-standard-16-jdk-11	✅	https://ge.armeria.dev/s/ojv26g6b3mn2o
build-macos-latest-jdk-21	✅	https://ge.armeria.dev/s/py4l6eidypwdo

[minwoox]

Is this set only once for a connection?
For example, if endStreamSentKey is set to true for streamId 3, then the following streams (e.g. 5, 7.. ) are all set to true?

[jrhee17]

Is this set only once for a connection?

Unless my reading of the code is incorrect, I believe the property is set to the stream, so the connection is unaffected.

https://github.com/netty/netty/blob/a73b7f77dcf3ba31b72b47978d33d2e27ed1df01/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java#L469-L471

For example, if endStreamSentKey is set to true for streamId 3, then the following streams (e.g. 5, 7.. ) are all set to true?

I assume that if streamId3.endStreamSentKey is set, then the other streams are unaffected.
Let me know if my assumption is wrong though

[minwoox]

Ah, that is correct. I misread the code. 😓

[ikhoon]

Question) In what situation can localSideOpen() be true when this method is called?

[jrhee17]

I guess the description wasn't super clear on exactly what condition this can be possible. Modified the PR description to the following:

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.

[ikhoon]

From my understanding, this method is called when resWriteFuture completes. It means the HttpResponse was fully sent to the Netty channel.
So I was wondering if we can remove (stream.state().localSideOpen() && !isEndStreamSent(stream)) condition and check only remoteSideOpen().

Suggested change

	if ((stream.state().localSideOpen() && !isEndStreamSent(stream)) || stream.state().remoteSideOpen()) {
	if (stream.state().remoteSideOpen()) {

[jrhee17]

Good point, I agree the remote side should more or less always be in sync, so we may only need to send the RST frame when local side is open only.
Updated.

[minwoox]

Looks great! Thanks!

[minwoox]

Ah, that is correct. I misread the code. 😓

[ikhoon]

Great! 👍👍