Repository navigation
Conversation
…ode does connect() validated options.settings in the ClientHttp2Session constructor and threw for every value that was not a valid settings object. node reads options.settings in setupHandle, which runs when the socket connects. It ignores a value that is not an object. A throw from validation is caught at the connect event and destroys the socket with that error, so the session emits 'error' and then 'close'. connect() throws only when the socket is already connected, because setupHandle then runs inline. The https path is different in node: it builds the tls.connect() options with initializeTLSOptions, which throws ERR_INVALID_ARG_TYPE for an options.settings that is not an object, before a socket exists. The constructor now does the same. It holds a validation error until the connect event and destroys the socket with it, ignores a value that is not an object, and keeps the throw for a socket that is already connected. For an https URL without createConnection it checks options.settings with assertIsObject before it connects.
…ns.settings The connect handler destroyed the socket with the validation error and relied on the session's socket error handler to report it. That handler drops an error on a session that close() was called on, so a request made before a close() lost the error. node reports it. The handler now destroys the session with the error, one tick after the connect callback. The tick is needed because the socket layer reports a throw from its connect callback on the socket, so an 'error' with no listener would not reach the process. A session that close() was called on with no request pending stays quiet, as in node, where close() has already destroyed it.
…le session A pending request that was destroyed before close() no longer counts as pending when the connect handler decides whether node would still validate options.settings. node's close() destroys such a session at once. The test gains cases for a session closed with nothing pending, for https with createConnection, and for a session with no 'error' listener, where the error must reach the process as an uncaught exception. The test comments now say that the https throw needs a URL without createConnection and a value that is not an object.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughChangesHTTP/2 client setup now validates HTTP/2 settings handling
Suggested reviewers: Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review. How to reproduce. Run this script with const http2 = require("node:http2");
const vals = { null: null, number: 1, string: "x", array: [], fn: function fn() {}, bool: true, badvalue: { initialWindowSize: -1 } };
const server = http2.createServer();
server.listen(0, "127.0.0.1", async () => {
const port = server.address().port;
for (const [name, v] of Object.entries(vals)) {
let client;
try { client = http2.connect(`http://127.0.0.1:${port}`, { settings: v }); }
catch (e) { console.log(name, "SYNC THROW", e.code, "|", e.message); continue; }
const outcome = await new Promise(resolve => {
client.once("error", e => resolve(`'error' event ${e.code} | ${e.message}`));
client.once("remoteSettings", () => resolve("connected, no error"));
});
console.log(name, outcome);
client.destroy();
}
server.close();
process.exit(0);
});
The new tests are in |
|
Updated 7:19 PM PT - Sep 19th, 2026
✅ @robobun, your commit 81ea8d51187f45538907340384fbd2ecbd64da8b passed in 🧪 To try this PR locally: bunx bun-pr 43583That installs a local version of the PR into your bun-43583 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the connected-socket rethrow at src/js/node/http2.ts:5734 — it throws before the session attaches any socket listeners, exactly where the base branch's validateSettings threw, so it does not introduce a new orphaned-listener state. The typeof === "object" gate at line 5729 deliberately lets null and arrays through to validateSettings so they are reported as errors rather than ignored, matching Node.
Extended reasoning...
Four verified findings are being posted inline, so approval is off the table and a human look is already signaled. This note only records two things examined and ruled out: the synchronous rethrow for an already-connected createConnection socket happens at the same point in the constructor as the base branch's validateSettings call (before socket.on(...) registration and before #parser assignment), so the caller-visible state on that path is unchanged; and the typeof options.settings === "object" filter intentionally passes null/arrays to validation, which is the intended Node-matching outcome ('error' then 'close'), not an oversight.
…e connect event The connect handler deferred the destroy by one tick so that an 'error' with no listener could reach the process. In that tick a stream from createConnection could still deliver data to the parser, and a 'close' or 'error' from the stream could destroy the session first and lose the settings error. The handler now destroys the session at once. If destroy() throws because nobody listens for 'error', it throws the error again from the next tick. A throw from inside the socket's connect callback is reported on the socket, where the destroyed session no longer listens.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The cases move out of node-http2.test.js into node-http2-connect-settings.test.ts. The fixture now starts its servers once and exposes the cases in three groups, so each in-process test makes a few connections and stays far below the default timeout on a slow debug build. node still runs the whole fixture as one script. The case with no 'error' listener needs a child process. A debug build needs several seconds to load node:http2 in a child, so that case does not run on debug builds. Release and ASAN builds run it.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline note, I re-checked the two connect-tick concerns from the earlier review against the latest push: onConnect now calls this.destroy(settingsError) synchronously (src/js/node/http2.ts:5667-5677) instead of deferring to process.nextTick, so a createConnection stream that emits 'close'/'error' or delivers buffered data right after 'connect' can no longer beat the destroy or feed the parser first. Also confirmed null still reaches validateSettings (typeof null is "object"), matching node's rejection of settings: null.
Extended reasoning...
The latest commit replaced the nextTick-deferred destroy with a synchronous this.destroy(settingsError) inside the connect handler and added the rethrowUncaught nextTick rethrow for the no-'error'-listener case. Reading Http2Session#destroy (http2.ts:4692) confirms it latches #destroying, marks the session closed, and ends/destroys the socket in the same call, so the ordering hazards raised against the previous deferred version no longer apply. The typeof options.settings === "object" filter deliberately lets null through to validateSettings, which is the behavior node exhibits (assertIsObject rejects null). The remaining inline finding concerns test coverage on debug builds, not runtime behavior.
The case started a bun child and was skipped on debug builds, because the child needs several seconds to load node:http2 there. It now runs on every build and gets a longer timeout on debug builds only, as the other child process tests in this directory do.
This branch has not been deployed
1 participant
Problem
http2.connect(url, { settings })throws for eachsettingsvalue that is not a valid settings object:ERR_INVALID_ARG_TYPE: The "settings" argument must be of type object. Received type number (1).null, an array, or an invalid value, its session emits'error', then'close'.ClientHttp2Sessionconstructor callsvalidateSettings(options.settings)right after it creates the socket (src/js/node/http2.ts:5698on main).Fix
validateSettingsthrows while the socket connects, the connect handler destroys the session with that error.connect()still throws in node's two cases. One is acreateConnectionsocket that is already connected. The other is anhttps:URL withoutcreateConnection, whereassertIsObjectnow checksoptions.settingsfirst.test/js/node/http2/node-http2-connect-settings.test.tsruns one script under Bun and under node v26.3.0 against one expected table. Alsotest/js/node/http2/and 261 vendoredtest-http2-*.jstests.Background
setupHandleis node's function that attaches the native session to the socket and sends the first SETTINGS frame. For a connecting socket it runs in the connect event inside atry. Thecatchdestroys the socket with the error.'error'. With no listener it is an uncaught exception.H2FrameParseris Bun's native session. The constructor builds it before the connect, becauseclose()andsettings()use it. After a rejected value, the session is destroyed before the parser gets the socket.Notes
What node v26.3.0 does (
lib/internal/http2/core.js)connect()checksoptionsitself and nothing undersettings. Forhttps:withoutcreateConnectionit builds the socket options withinitializeTLSOptions, which runsassertIsObject(options.settings, 'options.settings')beforetls.connect().setupHandlereadstypeof options.settings === 'object' ? options.settings : {}and callsthis.settings(settings). That runsassertIsObject(settings, 'settings')andvalidateSettings.setupHandleintry { ... } catch (error) { socket.destroy(error) }. For a connected socket it callssetupHandleinline, so the throw leavesconnect().setupHandlereturns early for a destroyed session.close()destroys a session that has no pending or open stream at once.Outcomes (one script, each runtime)
connect()callsettings: nullor[]'error'ERR_INVALID_ARG_TYPE,'close'settings: { initialWindowSize: -1 }'error'ERR_HTTP2_INVALID_SETTING_VALUE,'close'settings: 1,"x",true, a functionsettings: null,[],1"options.settings" propertymessage"settings" argumentmessage'error','close'createConnection,settings: null'error','close'createConnection, socket still connecting'error','close'createConnection, stream that is not connectingrequest(), thenclose()before the connect'error', request cancelled with the error as causeclose()with no live request'error'listeneruncaughtExceptionuncaughtExceptionWhy the connect handler catches a throw from
destroy().destroy(error)emits'error'. With no listener that emit throws. Inside the socket's connect callback, Bun's socket layer reports a throw on the socket (SocketHandlers.errorinnet.ts), and the session's socket error handler ignores it because the session is already destroyed. The error would be lost. The handler catches it and throws it again fromprocess.nextTick, where it is an ordinary uncaught exception._http_server.tsdoes the same for an'upgrade'listener that throws. An earlier revision deferred the whole destroy by one tick. Review showed that a stream fromcreateConnectioncould deliver data, or close, inside that tick, so the destroy now runs at once.Why the session, not the socket. A first revision destroyed the socket with the error, which is what node's
catchdoes. Bun's session handler for socket errors drops an error on a session thatclose()was called on. Sorequest()thenclose()before the connect lost the error. node reports it there.close()before the connect. Bun'sclose()only schedules the destroy of an idle session (a 250 ms wait for the SETTINGS ACK), so that session can still be alive at the connect event. The connect handler treats it as node does: it does not report the rejected value. A pending request that was destroyed does not count as live.What still throws from
connect(). A value thatvalidateSettingsaccepts and the native parser rejects, and a getter that throws on a read after validation.What the two open PRs on the same lines need when this lands first
settings: { initialWindowSize: -1 }on a connecting socket. With this change that value no longer throws from the constructor, so its test passes without its fix. It needs a trigger that still throws, for example the getter that throws on its third read. Itscatchwould also destroy a caller-owned connected socket on the throw this PR keeps. node leaves that socket open.optionsto the native parser, which then readsoptions.settingsitself. The parser must get the sanitizedsettingslocal instead. With the raw value,connect()throws again for1,"x",trueand for a rejected value.Differences that this PR leaves alone
assertIsObjectaccepts a function, soconnect()does not throw and the value is ignored. Another change owns that helper.'error'and the session's'error', andsocket.destroyedat the session's'close': node:http2: destroy the socket on session.destroy() without close(), emit the session 'close' once the socket has closed #38195.settings: { customSettings: 5 }reportsERR_HTTP2_INVALID_SETTING_VALUEwhere node reportsERR_INVALID_ARG_TYPE.validateSettingsalso rejectsNaN, which node's range check lets through.DuplexfromcreateConnectionthat setsconnecting = true: node wraps it and sets the session up inline, so node throws. Bun honours the flag (older code) and now reports'error'when the stream emits'connect'.connect(). node reads it at the connect event, so a caller that mutates the object in between sees a different result.connect()(it copies the object first), Bun reports'error'.util.promisify(http2.connect)throws synchronously whenconnect()throws. node rejects the promise. This is older behaviour and a separate bug. The new https check is one more source of it.Found while working, not part of this PR
connect()listener that throws destroys the session, or is lost when the session has no'error'listener. node raisesuncaughtExceptionand keeps the session.connect()andcreateServer()skip node'sstrictSingleValueFieldscheck. httpsconnect()skips the threevalidateUint32checks ofinitializeOptions.connect("https://...", { ALPNProtocols })lets the caller's list replaceh2. node always offersh2.Test design. The tests have their own file.
node-http2.test.jshas about 400 tests, and several of them start abun-debugchild. On a loaded machine a local run of that file hits the default 5 s timeout in a dozen tests that this change does not touch. The fixture starts its servers once and exposes the cases in three groups. Under Bun each group is one in-process test, so each test makes a few connections. node runs the whole fixture as one script. The firstconnect()uses a socket that the fixture owns. If it throws, the fixture destroys the socket and no group makes anotherconnect(). On a build without this change every later case would throw after it made its own socket, and on main that socket's connect callback re-queues itself forever (#42184), which would hang the test runner. The no-listener case needs a child process, because an uncaught exception fails an in-process test. A debug build needs several seconds to loadnode:http2in a child, so that case has a 60 s timeout on debug builds only, like the child process tests innode-http2-streams-rehash.test.ts. The test does not assert the order of the request error and the session error (#38195).Suites run on the debug (ASAN) build
test/js/node/http2/node-http2-connect-settings.test.ts: 6 pass on the debug build and on a release build with this change. On bun 1.4.2: the four Bun cases fail in under a second with a diff, and the two node cases pass.test/js/node/http2/node-http2.test.js, which this PR no longer touches: 386 pass, 6 skip, 5 to 11 fail. All of the failures are 5 s timeouts in tests that start abun-debugchild. The machine had a load average near 100.test/js/node/http2/: all pass except theh2-conformance.test.tsGC case that test: deflake the h2 stream-release cases on debug builds #42357 tracks.test/js/node/test/parallel/test-http2-*.jsandsequential/test-http2-*.js, measured at 8bf94e1: 261 of 261 exit 0.test-http2-forget-closed-streams.jsneeds about 165 s on this build, so in the full run it hit the 180 s limit of my loop and passed when run alone.connect()and session settings, pass again.[human-review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file