Skip to content

node:http2: pin the slot accounting of maxConcurrentStreams - #44335

Closed
robobun wants to merge 2 commits into
robobun/2a0b1487/http2-refused-stream-countfrom
robobun/d972025c/http2-slot-pins
Closed

robobun wants to merge 2 commits into
robobun/2a0b1487/http2-refused-stream-countfrom
robobun/d972025c/http2-slot-pins

Conversation

@robobun

@robobun robobun commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • A node:http2 server counts the open streams of SETTINGS_MAX_CONCURRENT_STREAMS in JS (#peerInitiatedStreams, src/js/node/http2.ts:4044). node:http2: refuse a stream over the advertised limit before it is opened, on maxSessionInvalidFrames #44337 moves that count to native code. No test says when a stream gives its slot back.
  • Three pieces of code exist in copies: the charge against maxSessionInvalidFrames (2), the two writes of an error exit of request() (6) and the end of the local half of a stream (4), in connection.rs and h2_frame_parser.rs.

Fix

  • Add test/js/node/http2/node-http2-max-concurrent-streams.test.ts with 33 tests, written for node:test so that node runs the same file. Each test pins one order in which a stream takes a slot or gives it back.
  • Connection::count_invalid_frame, H2FrameParser::close_unsent and Stream::end_local replace the copies.
  • The tests pass without the source change.
  • Verified: the 33 tests pass on Bun 1.4.3. node --test on v26.3.0 passes 29 and skips 4 that are Bun only. The debug build of node:http2: refuse a stream over the advertised limit before it is opened, on maxSessionInvalidFrames #44337 has this source and passes test/js/node/http2/.

Behaviour change: none

Background

Downsides

  • None found. Each helper has the body of the copies that it replaces, and the http2 suites pass on a build with them.
Notes

What the tests pin. Raw HTTP/2 client, limit 1 unless the test says another value.

Order Node v26.3.0 Bun 1.4.3 and this branch
Both sides ended the stream, one request per round trip next request served same
Response of HEADERS only, sendTrailers(), compat res.end(), one request per round trip served same
The end of a response that waited for a WINDOW_UPDATE served same
Request END_STREAM after the response ended, in one write with the next HEADERS served same
RST_STREAM from the peer (CANCEL, NO_ERROR, INTERNAL_ERROR, REFUSED_STREAM), in one write with the next HEADERS served same
The same for a stream that waits for trailers, and for the compat API served same
stream.close(CANCEL) and stream.close(NO_ERROR) by the server served same
3 requests in one write, handler answers from setImmediate 1 served, 2 refused same
3 requests in one write, handler ends the response in the same turn (4 shapes): no test here, #44337 has them 1 served, 2 refused 3 served
respond() that fails in Bun (weight, parent, options, maxSendHeaderBlockLength): 4 tests, Bun only does not fail next request served
session.settings() in 'session', in a handler (lower and raise), below the open count applies from the call same
server.updateSettings() before the connection applies same
Limit 0, trailers on the open stream, pushed streams, HPACK state after a refused block, frames behind a refused HEADERS as the test says same
A stream over a limit that the client ACKed: no test, see #44337 connection error (nghttp2_session.c:3889) RST_STREAM(REFUSED_STREAM), session open
5 requests at limit 2 through the core API, the compat API, TLS, and performServerHandshake() over a Duplex 2 served, 3 refused same

The node column comes from the same scenarios as plain scripts on node v26.3.0, and from the test file itself on node.

Why the tests are sequential. They run servers in the test process. describe.concurrent does not make them faster, and each test then waits for the others inside its own timeout.

Why a new file. The rule from #37588 and #41305 is that a test of node:http2 passes on Bun and on node. node-http2-rejected-streams.test.ts uses bun:test, so node needs a shim to run it. This file uses node:test and node:assert, like node-http2-continuation.test.ts: node --test test/js/node/http2/node-http2-max-concurrent-streams.test.ts. The 4 tests that node skips need a respond() that fails, and the calls that make it fail in Bun do not fail on node.

The three helpers. count_invalid_frame has the body that both call sites had: node's count++ > max, then terminated = true and on_too_many_invalid_frames(). close_unsent has the two writes (state = CLOSED, rst_code) that each of the six exits had. end_local has the transition that four places had after this side wrote END_STREAM: CLOSED and free_resources when the peer has ended, HALF_CLOSED_LOCAL when it has not. The dispatches stay where they are.

A reset that Bun can write twice. stream.close(code) schedules an RST_STREAM, and the destroy that follows schedules one more. When a read comes between the two, both go out. CI showed it on Windows 11 aarch64. The test of close(CANCEL) counts each reset once. #43539 removes the second reset.

Open PRs on the same lines: #40240 rewrites the six request() exits.

@robobun

robobun commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: closed. The tests and the two helpers of this PR are one commit of #44337 now, which is one PR on main.

@robobun
robobun force-pushed the robobun/d972025c/http2-slot-pins branch from 06a91c9 to c93464b Compare October 1, 2026 00:59
Comment thread src/runtime/api/bun/h2/connection.rs Resolved
@robobun
robobun force-pushed the robobun/d972025c/http2-slot-pins branch 2 times, most recently from 676ab8a to 39e6256 Compare October 2, 2026 04:20
@robobun
robobun force-pushed the robobun/d972025c/http2-slot-pins branch from 39e6256 to 0c708b6 Compare October 3, 2026 07:36
@robobun

robobun commented Oct 18, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: this is folded into #44337. A review asked for one PR on main in place of a stack of three. The tests and the two helpers of this PR are one commit there.

@robobun robobun closed this Oct 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant