You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "rp-arielrodriguez (via GitHub)" <gi...@apache.org> on 2026/09/29 22:13:02 UTC
[PR] Fail unprocessed local streams on GOAWAY [httpcomponents-core]
rp-arielrodriguez opened a new pull request, #711:
URL: https://github.com/apache/httpcomponents-core/pull/711
We found this with HTTP/2 multiplexing in HttpClient 5: when the server sent
GOAWAY(NO_ERROR) during a restart, requests on the same connection that the
server did not process were not failed. They waited until the server closed
the connection.
On GOAWAY(NO_ERROR), `AbstractH2StreamMultiplexer` must fail the local streams
with an id greater than Last-Stream-ID with `RequestNotExecutedException`
(RFC 9113, section 6.8). The check used `!streams.isSameSide(...)`, so it
selected streams initiated by the peer instead.
Fix: drop the negation (one line).
Measured with an end-to-end test (real client, raw-frame server that sends
GOAWAY last-stream-id=1 with stream 3 in flight and closes the connection 3s
later):
| Stream 3 (not processed) | Result |
|---|---|
| master | `ConnectionClosedException` after ~3.2s (at connection close) |
| with fix | `RequestNotExecutedException` immediately |
Stream 1 completes normally in both cases. I did not include that test, to keep
the change small; I can add it if you want it.
The same line is on 5.4.x, so the fix applies there as is.
Tests:
- New: `testGoAwayNoErrorFailsUnprocessedLocalStreamsOnly` (fails on master,
passes with the fix).
- Changed: `testGoAwayReservedBitInLastStreamIdAffectsStreamCulling` used remote
streams, so it depended on the inverted check. It now uses local streams and
still checks that the reserved bit is masked.
- `./mvnw -pl httpcore5-h2,httpcore5-testing -am test`: all pass (httpcore5-h2
387, httpcore5-testing 443 with 4 skipped). Checkstyle and RAT pass.
I used an AI assistant (Claude Code) for this change. I reviewed all of it and
I can answer questions about it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Fail unprocessed local streams on GOAWAY [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c merged PR #711:
URL: https://github.com/apache/httpcomponents-core/pull/711
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Fail unprocessed local streams on GOAWAY [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #711:
URL: https://github.com/apache/httpcomponents-core/pull/711#issuecomment-5918792583
@rp-arielrodriguez Cherry-picked to 5.4.x though I had to port the tests manually. Please double-check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Fail unprocessed local streams on GOAWAY [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #711:
URL: https://github.com/apache/httpcomponents-core/pull/711#discussion_r4142107088
##########
httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/nio/TestAbstractH2StreamMultiplexer.java:
##########
@@ -2026,52 +2026,68 @@ void testGoAwayReservedBitInLastStreamIdAffectsStreamCulling() throws Exception
h2StreamListener,
() -> streamHandler);
- // Create 3 remote (even) streams by feeding inbound HEADERS on 2,4,6.
- final ByteArrayBuffer headerBuf = new ByteArrayBuffer(256);
- final HPackEncoder encoder = new HPackEncoder(
- H2Config.INIT.getHeaderTableSize(),
- CharCodingSupport.createEncoder(CharCodingConfig.DEFAULT));
+ // Create 3 local (odd) streams: 1, 3, 5.
+ final H2StreamHandler streamHandler1 = Mockito.mock(H2StreamHandler.class);
+ final H2StreamHandler streamHandler3 = Mockito.mock(H2StreamHandler.class);
+ final H2StreamHandler streamHandler5 = Mockito.mock(H2StreamHandler.class);
+ mux.createStream(mux.createChannel(1), streamHandler1);
+ mux.createStream(mux.createChannel(3), streamHandler3);
+ mux.createStream(mux.createChannel(5), streamHandler5);
- final List<Header> headers = Arrays.asList(
- new BasicHeader(":method", "GET"),
- new BasicHeader(":scheme", "https"),
- new BasicHeader(":path", "/"),
- new BasicHeader(":authority", "example.test"));
- encoder.encodeHeaders(headerBuf, headers, h2Config.isCompressionEnabled());
+ // GOAWAY last-stream-id = 4, but with reserved MSB set.
+ // Correct masking keeps streams <= 4 (1 and 3) and drops only stream 5.
+ final ByteBuffer goAwayPayload = ByteBuffer.allocate(8);
+ goAwayPayload.putInt(0x80000004); // reserved bit set, last-stream-id = 4
+ goAwayPayload.putInt(H2Error.NO_ERROR.getCode());
+ goAwayPayload.flip();
- final RawFrame h2 = FRAME_FACTORY.createHeaders(2,
- ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()),
- true, // END_HEADERS
- false // END_STREAM
- );
- final RawFrame h4 = FRAME_FACTORY.createHeaders(4,
- ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()),
- true,
- false
- );
- final RawFrame h6 = FRAME_FACTORY.createHeaders(6,
- ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()),
- true,
- false
- );
+ final RawFrame goAway = new RawFrame(FrameType.GOAWAY.getValue(), 0, 0, goAwayPayload);
+
+ Assertions.assertDoesNotThrow(() -> mux.onInput(ByteBuffer.wrap(encodeFrame(goAway))));
- feedFrame(mux, h2);
- feedFrame(mux, h4);
- feedFrame(mux, h6);
+ Mockito.verify(streamHandler5, Mockito.times(1)).failed(exceptionCaptor.capture());
+ Assertions.assertInstanceOf(org.apache.hc.core5.http.RequestNotExecutedException.class, exceptionCaptor.getValue());
Review Comment:
@rp-arielrodriguez Good catch.
Please do me a little favor and get rid of fully qualified `org.apache.hc.core5.http.RequestNotExecutedException.class`? It can be imported.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Fail unprocessed local streams on GOAWAY [httpcomponents-core]
Posted by "rp-arielrodriguez (via GitHub)" <gi...@apache.org>.
rp-arielrodriguez commented on code in PR #711:
URL: https://github.com/apache/httpcomponents-core/pull/711#discussion_r4145193089
##########
httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/nio/TestAbstractH2StreamMultiplexer.java:
##########
@@ -2026,52 +2026,68 @@ void testGoAwayReservedBitInLastStreamIdAffectsStreamCulling() throws Exception
h2StreamListener,
() -> streamHandler);
- // Create 3 remote (even) streams by feeding inbound HEADERS on 2,4,6.
- final ByteArrayBuffer headerBuf = new ByteArrayBuffer(256);
- final HPackEncoder encoder = new HPackEncoder(
- H2Config.INIT.getHeaderTableSize(),
- CharCodingSupport.createEncoder(CharCodingConfig.DEFAULT));
+ // Create 3 local (odd) streams: 1, 3, 5.
+ final H2StreamHandler streamHandler1 = Mockito.mock(H2StreamHandler.class);
+ final H2StreamHandler streamHandler3 = Mockito.mock(H2StreamHandler.class);
+ final H2StreamHandler streamHandler5 = Mockito.mock(H2StreamHandler.class);
+ mux.createStream(mux.createChannel(1), streamHandler1);
+ mux.createStream(mux.createChannel(3), streamHandler3);
+ mux.createStream(mux.createChannel(5), streamHandler5);
- final List<Header> headers = Arrays.asList(
- new BasicHeader(":method", "GET"),
- new BasicHeader(":scheme", "https"),
- new BasicHeader(":path", "/"),
- new BasicHeader(":authority", "example.test"));
- encoder.encodeHeaders(headerBuf, headers, h2Config.isCompressionEnabled());
+ // GOAWAY last-stream-id = 4, but with reserved MSB set.
+ // Correct masking keeps streams <= 4 (1 and 3) and drops only stream 5.
+ final ByteBuffer goAwayPayload = ByteBuffer.allocate(8);
+ goAwayPayload.putInt(0x80000004); // reserved bit set, last-stream-id = 4
+ goAwayPayload.putInt(H2Error.NO_ERROR.getCode());
+ goAwayPayload.flip();
- final RawFrame h2 = FRAME_FACTORY.createHeaders(2,
- ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()),
- true, // END_HEADERS
- false // END_STREAM
- );
- final RawFrame h4 = FRAME_FACTORY.createHeaders(4,
- ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()),
- true,
- false
- );
- final RawFrame h6 = FRAME_FACTORY.createHeaders(6,
- ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()),
- true,
- false
- );
+ final RawFrame goAway = new RawFrame(FrameType.GOAWAY.getValue(), 0, 0, goAwayPayload);
+
+ Assertions.assertDoesNotThrow(() -> mux.onInput(ByteBuffer.wrap(encodeFrame(goAway))));
- feedFrame(mux, h2);
- feedFrame(mux, h4);
- feedFrame(mux, h6);
+ Mockito.verify(streamHandler5, Mockito.times(1)).failed(exceptionCaptor.capture());
+ Assertions.assertInstanceOf(org.apache.hc.core5.http.RequestNotExecutedException.class, exceptionCaptor.getValue());
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org