http: publish to http.server.* diagnostics_channel channels - #29588
http: publish to http.server.* diagnostics_channel channels#29588robobun wants to merge 11 commits into
Conversation
|
Updated 12:33 AM PT - Jul 15th, 2026
❌ @robobun, your commit c11cc98 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29588That installs a local version of the PR into your bun-29588 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughLazily initializes three diagnostics_channel channels for HTTP server ( Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/diagnostics_channel/diagnostics_channel.test.ts`:
- Around line 348-405: The test currently inspects the internal _subscribers for
cleanup which couples it to internals; instead, when calling
channels.map/ch.subscribe store each listener callback in a parallel
subscriptions array (capture the function passed to ch.subscribe) and in the
finally block call ch.unsubscribe with that stored function for each channel
(use the same channel variables: channels, ch.subscribe, ch.unsubscribe) so
teardown uses the public API rather than _subscribers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22f8b3cb-0f24-4411-b3ba-6af92223da15
📥 Commits
Reviewing files that changed from the base of the PR and between 3453c22 and f8858d935a584f781ae9d5b5f0669f848a6af046.
📒 Files selected for processing (3)
src/js/node/_http_server.tssrc/js/node/diagnostics_channel.tstest/js/node/diagnostics_channel/diagnostics_channel.test.ts
2a426cc to
ff8f883
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/node/diagnostics_channel/diagnostics_channel.test.ts (1)
356-360: 🧹 Nitpick | 🔵 TrivialUnsubscribe the exact listener instead of reading
_subscribers.Teardown is reaching into an internal field here. That makes the test brittle, and
[_subscribers][0]can remove the wrong callback if the channel already has another subscriber. Store each subscribed function and pass that same reference tounsubscribe().💡 Safer teardown
- const channels = channelNames.map(n => channel(n)); - for (const ch of channels) { - ch.subscribe(msg => { + const subscriptions: { ch: ReturnType<typeof channel>; fn: (msg: unknown) => void }[] = []; + for (const name of channelNames) { + const ch = channel(name); + const fn = (msg: unknown) => { events.push({ channel: ch.name as string, payload: msg as Record<string, unknown> }); - }); + }; + ch.subscribe(fn); + subscriptions.push({ ch, fn }); } @@ - for (const ch of channels) ch.unsubscribe(ch._subscribers?.[0] ?? (() => {})); + for (const { ch, fn } of subscriptions) ch.unsubscribe(fn);Also applies to: 398-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/node/diagnostics_channel/diagnostics_channel.test.ts` around lines 356 - 360, The test currently reaches into the internal _subscribers to remove callbacks which is brittle; instead capture and store the exact listener function returned/used when calling ch.subscribe for each channel (e.g., the callback passed to ch.subscribe in the channels loop) and later call ch.unsubscribe(listener) with that same function reference to tear down; update both loops (the one at lines around the channels subscribe and the similar block at lines ~398–399) to retain the listener variable per channel and pass it to ch.unsubscribe rather than manipulating _subscribers directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/js/node/diagnostics_channel/diagnostics_channel.test.ts`:
- Around line 356-360: The test currently reaches into the internal _subscribers
to remove callbacks which is brittle; instead capture and store the exact
listener function returned/used when calling ch.subscribe for each channel
(e.g., the callback passed to ch.subscribe in the channels loop) and later call
ch.unsubscribe(listener) with that same function reference to tear down; update
both loops (the one at lines around the channels subscribe and the similar block
at lines ~398–399) to retain the listener variable per channel and pass it to
ch.unsubscribe rather than manipulating _subscribers directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a92971b-64c6-430f-b067-e381577f6f24
📥 Commits
Reviewing files that changed from the base of the PR and between f8858d935a584f781ae9d5b5f0669f848a6af046 and 2a426cccd28196f97d772fdcd92b410670552a94.
📒 Files selected for processing (1)
test/js/node/diagnostics_channel/diagnostics_channel.test.ts
96674aa to
d08aca7
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/node/_http_server.ts:669-676— Node.js publisheshttp.server.request.startonce unconditionally inparserOnIncoming(after creatingres, before any branching), so it fires for thecheckContinue-listener,checkExpectation-listener, 417 auto-response, anddropRequest/503 paths too. Here it's only published in the two branches that reachserver.emit('request', ...), so APM/OTel subscribers will silently miss those other paths in Bun. Hoist to a single publish guarded by\!is_upgradejust before theif (reachedRequestsLimit)chain and drop the two duplicated blocks.Extended reasoning...
What the bug is
In Node.js
lib/_http_server.js,onRequestStartChannel.publish({request, response, socket, server})is called once, unconditionally, inparserOnIncomingat line ~1234 — immediately afterconst res = new server[kServerResponse](req, ...)and before therequireHostHeader400 check, themaxRequestsPerSocket/dropRequest503 path, and the entireExpect-header branching. (Upgrade requests return early at the very top ofparserOnIncoming, beforeresis created, so they correctly never seerequest.start.) Consequently Node publisheshttp.server.request.startfor every non-upgrade request that gets aServerResponse, regardless of which branch ultimately handles it.This PR instead duplicates the publish inside only the two branches that lead to
server.emit('request', ...): the post-writeContinue()path and the default no-Expectpath. It is not published for:server.emit('checkContinue', http_req, http_res)(Expect: 100-continue with acheckContinuelistener)server.emit('checkExpectation', http_req, http_res)(other Expect value with acheckExpectationlistener)- the 417 Expectation Failed auto-response (other Expect value, no listener)
server.emit('dropRequest', ...)+ 503 (maxRequestsPerSocketexceeded)
Step-by-step proof
- Subscribe:
dc.subscribe('http.server.request.start', cb). - Create a server with
server.on('checkContinue', (req, res) => res.end()). - Send a request with
Expect: 100-continue. - In
onNodeHTTPRequest:http_resis constructed →response.createdpublishes → branching reacheshttp_req.headers.expect === '100-continue'→server.listenerCount('checkContinue') > 0is true →server.emit('checkContinue', ...)is called. Norequest.startpublish on this path. - Node:
parserOnIncomingconstructsres, publishesrequest.start, then enters the same branch and emitscheckContinue.cbfires. - Result:
cbis invoked in Node, not in Bun. The same divergence applies to thecheckExpectation, 417, anddropRequest/503 branches.
Why nothing prevents it
The two publish blocks were placed adjacent to the two
server.emit('request', ...)calls, presumably on the assumption thatrequest.startsemantically pairs with the'request'event. But Node's contract is broader: it pairs with "a ServerResponse was created for an inbound request", independent of which event the server ultimately emits. Nothing else inonNodeHTTPRequestpublishes to this channel.Impact
Observable Node-compat divergence in exactly the feature this PR adds. APM/OTel libraries that count or trace inbound requests via
http.server.request.startwill undercount in Bun vs Node for servers that usecheckContinue/checkExpectationlisteners, receive non-100-continueExpect headers, or hitmaxRequestsPerSocket. These are uncommon paths (hence nit), but the fix is also a code-dup cleanup, so it's worth doing.How to fix
Match Node's single placement. Since Bun already has
const is_upgrade = http_req.headers.upgrade;computed before the dispatch chain, publish once right beforeif (reachedRequestsLimit):if (\!is_upgrade && onRequestStartChannel.hasSubscribers) { onRequestStartChannel.publish({ request: http_req, response: http_res, socket, server, }); } if (reachedRequestsLimit) { ...
and delete the two duplicated publish blocks inside the
writeContinueand default branches. (The\!is_upgradeguard mirrors Node's early-return-before-publish for upgrades.)
4210834 to
1e31cb3
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/node/_http_server.ts:693-700— Node.js publisheshttp.server.request.startonce, unconditionally, right after constructing theServerResponseand before any expect-header /maxRequestsPerSocketbranching (lib/_http_server.jsparserOnIncoming~1234), so it fires for the'checkContinue'-listener,'checkExpectation'-listener, 417, anddropRequest/503 paths too — but here it's only published inside the two branches that reachserver.emit('request', ...), so those four paths never fire it (and on thewriteContinuepath the ordering is reversed: Node publishes beforewriteContinue(), this publishes after). Fix: hoist a single guarded publish to just before theif (reachedRequestsLimit)branching, gated on!is_upgrade, and drop the two nested copies.Extended reasoning...
What the bug is
In Node.js's
lib/_http_server.js,parserOnIncomingdoes (verified on main, ~lines 1221-1241):const res = new server[kServerResponse](req, ...); ... if (onRequestStartChannel.hasSubscribers) { onRequestStartChannel.publish({ request: req, response: res, socket, server }); } // ...only AFTER this: requireHostHeader/400, maxRequestsPerSocket/dropRequest/503, // expect → checkContinue / writeContinue / checkExpectation / 417
i.e.
http.server.request.startis published once for every non-upgrade request, regardless of which branch is subsequently taken. (Upgrades return early before the response is constructed, so they never publish.)This PR instead publishes
request.startonly inside the two branches that lead directly toserver.emit('request', ...): afterhttp_res.writeContinue()(lines ~676-684) and in the no-expect-headerelse(lines ~693-700). Four code paths therefore never publishrequest.startin Bun but do in Node:reachedRequestsLimit→server.emit('dropRequest', ...)→ 503Expect: 100-continuewith a'checkContinue'listener registeredExpect: <other>with a'checkExpectation'listener registeredExpect: <other>with no listener → 417
There's also a minor ordering divergence on the
writeContinuepath: Node publishesrequest.startbeforeres.writeContinue()(publish at ~1234,writeContinueat ~1308), this PR publishes after. Observable to subscribers that inspect_sent100/socket state.Step-by-step proof (path 2 — the most realistic one)
- App registers
server.on('checkContinue', (req, res) => { res.writeContinue(); handler(req, res); })— the documented Node pattern for gating large uploads. - OTel
HttpInstrumentationsubscribes tohttp.server.request.startto open the server span. - Client sends
PUT /uploadwithExpect: 100-continue. onNodeHTTPRequestruns:reachedRequestsLimitfalse,is_upgradefalsy,http_req.headers.expect === '100-continue',server.listenerCount('checkContinue') > 0→ takes theserver.emit('checkContinue', http_req, http_res)branch.- No
onRequestStartChannel.publishcall lives on that branch. Control never reaches either of the two publish sites. - Node for the same request:
parserOnIncomingconstructsres, publishesrequest.startat ~1234, then reaches the expect branching at ~1305 and emits'checkContinue'. The OTel span opens. - Result: on Node the upload is traced; on Bun (with this PR) it is silently untraced.
The same walk applies to a
'checkExpectation'listener, the 417 auto-reply, and themaxRequestsPerSocket503/dropRequest path — none of those branches contain a publish.Why existing code doesn't prevent it
The two publish sites are nested inside specific branches; nothing publishes on the other branches.
response.createdand the'finish'listener are hoisted above the branching, so those two channels do fire on all four paths — onlyrequest.startis misplaced. This is distinct from inline comment #3122715185, which is aboutresponse.createdplacement (different channel, different fix).Impact
Edge-case Node-compat gap in the feature being added. The primary path (no
Expectheader, no request limit) is correct, and the PR is a strict improvement over the prior state (no events at all) — hence nit. But APM/OTel libraries that subscribe torequest.startwill miss spans for apps that use'checkContinue'(Express/Fastify large-upload pattern) or'checkExpectation', which works on Node.How to fix
Replace the two nested publishes with a single one placed just before the
if (reachedRequestsLimit)chain (afteris_upgradeis computed andsetCloseCallback(http_res, onClose)), gated on!is_upgradeto match Node's early-return for upgrades:setCloseCallback(http_res, onClose); if (!is_upgrade && onRequestStartChannel.hasSubscribers) { onRequestStartChannel.publish({ request: http_req, response: http_res, socket, server, }); } if (reachedRequestsLimit) { ...
Then delete the two nested copies. This also fixes the
writeContinueordering as a side effect (publish now precedeshttp_res.writeContinue()).
|
@claude thanks — cross-checked against Node and that was already fixed in 1e31cb3 (before your comment landed). |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/diagnostics_channel/diagnostics_channel.test.ts`:
- Line 503: The test currently uses a dynamic require for node:net when
constructing sockets (e.g., new (require("node:net").Socket)() in the creation
of http.IncomingMessage); move the node:net import to module scope by importing
Socket alongside the existing http import and replace new
(require("node:net").Socket)() with new Socket(); do the same replacement for
the other occurrence around line 610 so all Socket uses reference the
module-scoped Socket import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4f4378a-9cf2-456d-955e-093e346e5dcf
📥 Commits
Reviewing files that changed from the base of the PR and between 4210834a4a2dbc3b851e7b830938fcfe02258898 and 1e31cb321b5ff393be393f4e21b1bdbce9047769.
📒 Files selected for processing (2)
src/js/node/_http_server.tstest/js/node/diagnostics_channel/diagnostics_channel.test.ts
f590077 to
afe3376
Compare
|
CI failures on build 47215 are all four darwin test-bun steps in |
44640cd to
71ca34f
Compare
There was a problem hiding this comment.
All prior feedback is addressed and I found no issues in this revision, but since this adds an unconditional per-request .bind() + 'finish' listener inside onNodeHTTPRequest (the node:http hot path), it's worth a maintainer signing off on the perf trade-off vs. gating the attach on hasSubscribers.
Extended reasoning...
Overview
Adds Node-compatible diagnostics_channel publishing for http.server.request.start, http.server.response.created, and http.server.response.finish in src/js/node/_http_server.ts, plus ~280 lines of tests in test/js/node/diagnostics_channel/diagnostics_channel.test.ts. Channels are now created at module-load time (matching Node's lib/_http_server.js), response.created is published from the ServerResponse constructor, request.start is published once before the 503/checkContinue/417/normal branching gated on !is_upgrade, and a 'finish' listener is attached unconditionally per request with the hasSubscribers check inside the handler.
Security risks
None identified. The change only publishes references to already-existing per-request objects (req, res, socket, server) to in-process diagnostics_channel subscribers; there's no new parsing, no auth/crypto, and no user-controlled data flowing into new sinks.
Level of scrutiny
Medium-high. onNodeHTTPRequest is the hot path for every node:http request and this file already contains micro-optimizations (e.g. canUseInternalAssignSocket to avoid .once('close', ...)). The unconditional emitResponseFinishChannel.bind({req, res, socket, server}) + http_res.on('finish', ...) runs for every request even when nobody subscribes — Node pays the same cost via resOnFinish.bind(...), so it's faithful, but Bun has historically diverged from Node for perf in exactly this function. That's a judgment call a maintainer should make rather than a bot.
Other factors
- The PR went through five revision rounds; every inline comment (mine and coderabbit's) is now resolved and reflected in the current diff: module-load-time channels (resolves the lazy-load/custom-
ServerResponsecoupling), constructor-placedresponse.created(fixes directnew ServerResponse(req)), finish-timehasSubscriberscheck (late-subscriber compat), module-scopeimport http/import net, and the upgrade-test flake/dead-code cleanup. - Test coverage is thorough (ordering, payload shapes/identity, checkContinue, 417, upgrade-no-publish, late subscribe, direct construction, no-subscriber hot path).
- CI failures on build 47215 are darwin agent expirations (infra), not test failures; all Linux/Windows/Alpine lanes pass.
- Not a simple/mechanical change, so deferring rather than approving.
|
Build #51105 failure is a Buildkite agent-fleet outage between ~10:36Z and ~12:14Z on May 4: only 6 of 287 jobs got an agent, zero jobs actually failed tests ( |
Node's node:http server publishes to three diagnostics_channel channels
at request/response lifecycle points, which observability libraries
(OpenTelemetry, Prometheus, APM tooling) rely on for instrumentation.
Bun had not implemented these, so subscribers silently collected
nothing.
Adds publishes for:
- http.server.response.created — { request, response }
- http.server.request.start — { request, response, socket, server }
- http.server.response.finish — { request, response, socket, server }
Channel placement mirrors Node's lib/_http_server.js so all observable
edges match Node:
- response.created is published from inside the ServerResponse
constructor, so it fires for direct `new http.ServerResponse(req)`
(the pattern used by light-my-request / fastify.inject() / mocks) in
addition to the live-server path. Node does the same.
- request.start is published once per non-upgrade request before the
dispatch chain, so it fires on the normal, checkContinue,
checkExpectation, 417, and dropRequest/503 branches — not only the
two that reach server.emit('request', ...).
- response.finish is wired via an always-attached res.on('finish')
listener (matching Node's resOnFinish) with the hasSubscribers guard
inside the listener, so subscribers that register between request
arrival and response finish are still observed.
Synchronous publishes keep a call-site hasSubscribers guard so no
payload object is allocated when nobody is listening.
Ordering matches Node: response.created fires before the 'request'
event so user handlers can't have mutated the response yet, and
response.finish fires after the response is sent.
Fixes #29586
Pull the listen/port and setImmediate-drain boilerplate into two helpers in the describe block; drop the channelNames/subs ceremony in the multi-channel test in favor of three flat subscribe/unsubscribe pairs; collapse a multi-line request.start publish + trim its comment to a single line.
71ca34f to
695396b
Compare
|
Rebased on main to clear the merge conflict. Two conflicts, both in
No semantic change to the diagnostics_channel behaviour. The |
…ttp2 allowHTTP1 fallback
Register the http.server.response.finish listener before
endSocketOnFinishIfNeeded so the publish observes the socket before its
writable side is ended on the Connection: close path, matching Node's
resOnFinish (which publishes before socket.destroySoon()).
The response.created publish lives in the ServerResponse constructor, so
it already fired on the http2 createSecureServer({ allowHTTP1: true })
HTTP/1 fallback path (connectionListenerHTTP1), but that path never
published request.start or response.finish, leaving orphaned events.
Publish all three there, like Node routes allowHTTP1 through the full
http1 connectionListener.
8cdc285 to
d2594e3
Compare
Drop the redundant require("node:diagnostics_channel") inside
connectionListenerHTTP1 (it shadowed the module-level dc) and resolve
the two http.server channels once at module load alongside the existing
http2.* channel constants, instead of re-resolving them on every HTTP/1
connection.
There was a problem hiding this comment.
No bugs found and all prior review feedback has been addressed — deferring only because this adds an unconditional per-request 'finish' listener + .bind() in onNodeHTTPRequest and a publish in the ServerResponse constructor, both on the HTTP server hot path, so a human should sign off on the perf trade-off.
Extended reasoning...
Overview
Adds Node-compatible diagnostics_channel publishing for the three http.server.* channels (request.start, response.created, response.finish). Touches src/js/node/_http_server.ts (~40 lines in onNodeHTTPRequest and the ServerResponse constructor), src/js/node/http2.ts (~20 lines in the allowHTTP1 fallback), and adds ~350 lines of tests. The PR has been through eight review iterations; every inline comment I and CodeRabbit raised has been addressed, and the current bug-hunting run found nothing.
Security risks
None. diagnostics_channel is read-only observability; the published payloads ({request, response, socket, server}) are the same objects already handed to the 'request' event listener. No auth, crypto, or permissions code is touched.
Level of scrutiny
Moderate-to-high. The implementation is straightforward and closely mirrors Node's lib/_http_server.js, but it modifies onNodeHTTPRequest — the per-request hot path for every node:http server request in Bun — and the ServerResponse constructor. The unconditional http_res.on('finish', emitResponseFinishChannel.bind({...})) adds one bound-function + context-object allocation per request even with zero subscribers. That matches both Node's resOnFinish.bind(...) and the existing endSocketOnFinishIfNeeded.bind(...) immediately below it, so it's consistent with established patterns — but Bun's HTTP serve performance is heavily benchmarked, so a maintainer should confirm the trade-off is acceptable rather than have it auto-approved.
Other factors
Test coverage is thorough (10 new tests covering ordering, payload shapes/identity, checkContinue/417/upgrade paths, late subscription, direct construction, the http2 allowHTTP1 fallback, and the no-subscriber hot path). No CODEOWNERS apply to these files. The PR description is accurate to the current diff (the earlier out-of-scope WeakReference change was correctly dropped and is now noted as tracked separately in #26537).
|
Build #63300: 72 lanes green, only
None touch |
…rver-diagnostics-channel
…ener
Avoids the second per-request 'finish' listener and its .bind({...})
allocation by publishing http.server.response.finish from the same
listener that already closes the socket when needed, like Node's
resOnFinish. hasSubscribers is still checked at call time so late
subscribers are observed.
|
Merged current main and folded the All 10 |
…fallback Move the publish from a res 'finish' listener into handle.onfinished, before its socket.end() call, so subscribers observe the socket before its writable side is ended on non-keep-alive requests - the same ordering resOnFinish uses in node:_http_server. Also drops the per request closure. The allowHTTP1 test now sends Connection: close and asserts socket.writableEnded === false at publish time. Refresh a stale test comment that referenced the removed endSocketOnFinishIfNeeded two-listener mechanism.
Publishing from handle.onfinished ran synchronously inside handle.end(),
before ServerResponse.prototype.end set finished = true, so subscribers
observed response.finished === false where Node (and node:_http_server's
resOnFinish) show true. Publish from a res 'finish' listener instead and
defer the socket teardown into the same listener after the publish -
both the non-keep-alive socket.end() and the close-delimited one, which
handle.end() now records on handle.closeDelimited instead of ending the
socket itself. Subscribers now observe finished === true and
writableEnded === false, the same publish-time state as the plain http
path.
Both ordering tests now assert { finished, writableEnded } at publish
time; the allowHTTP1 one fails against the previous placement.
There was a problem hiding this comment.
c11cc98 addresses my last note — the allowHTTP1 response.finish publish is back on res.once('finish', ...) and socket.end() now runs after it, so both response.finished === true and socket.writableEnded === false hold at publish time (the new test asserts exactly that). No further findings from this pass.
Deferring to a human because the http2.ts hunk now also moves the allowHTTP1 non-keep-alive / close-delimited socket.end() from synchronous-inside-handle.end() to the nextTick'd 'finish' listener — closer to Node's resOnFinish, but a socket-lifecycle timing change to pre-existing code that's worth a maintainer glance alongside the hot-path onNodeHTTPRequest / ServerResponse constructor edits.
Checked: resOnFinish fold preserves the pre-existing kMustCloseConnection → socket?.end() behavior with two extra bound args and no new listener; request.start publish sits after the is_upgrade computation and before all dispatch branches; handle.closeDelimited is initialized in the handle literal so the deferred socket.end() still fires for close-delimited bodies.
Extended reasoning...
Overview
Adds Node-compatible diagnostics_channel publishes for the three http.server.* channels. _http_server.ts: three module-level dc.channel(...) constants, response.created published from the ServerResponse constructor, request.start published in onNodeHTTPRequest before dispatch branching (gated on !is_upgrade), and endSocketOnFinishIfNeeded renamed/extended to resOnFinish which publishes response.finish before conditionally ending the socket. http2.ts: two module-level channel constants and, in connectionListenerHTTP1 (the allowHTTP1 fallback), a request.start publish plus a res.once('finish', ...) closure that publishes response.finish and then ends the socket on non-keep-alive / close-delimited responses — replacing the previous synchronous socket.end() inside handle.end() / handle.onfinished. ~370 lines of new tests covering ordering, payload shapes/identity, checkContinue/417/upgrade paths, late subscription, direct construction, publish-time socket/response state, and the allowHTTP1 fallback.
Security risks
None identified. Publishes are guarded by .hasSubscribers and pass existing request/response/socket/server references to in-process subscribers; no new parsing, no external data flow, no auth/crypto surface.
Level of scrutiny
Medium-high. onNodeHTTPRequest and the ServerResponse constructor are per-request hot paths for every node:http server; the hasSubscribers guards keep the no-subscriber cost to a property read and the resOnFinish fold avoids adding a listener. The http2.ts change goes beyond pure diagnostics wiring: it defers when socket.end() runs on the allowHTTP1 non-keep-alive / close-delimited path (from sync-inside-res.end() to the nextTick'd 'finish' event). That's the correct Node-matching order and the 'finish' emit is unconditional in ServerResponse.prototype.end, but it is a behavior change to a production socket-lifecycle path that a maintainer should confirm.
Other factors
This PR has been through eight review iterations; every prior inline comment (including all of mine) is resolved, and c11cc98 addresses the last one I left. The bug-hunting system found nothing this run. Test coverage is thorough and gate-verified (fails on main, passes on the branch). I'm deferring rather than approving solely because the change set touches the per-request hot path and reworks socket-close timing in http2.ts — that's the kind of thing the approval guidelines say warrants a human sign-off even when the review is clean.
|
Build #73188 red is unrelated to this diff:
None exercise |
|
Closing in favor of #32628. It publishes |
1 participant
What does this PR do?
Fixes #29586.
node:httpserver never published to the three standard Node.js HTTPserver
diagnostics_channelchannels that observability libraries(OpenTelemetry, Prometheus/APM tooling) rely on. Subscribers to any of
http.server.request.start,http.server.response.created, andhttp.server.response.finishsilently got nothing, so those librariescollected no HTTP server metrics/traces on Bun.
Repro
Before:
count: 0. After:count: 3(matches Node).Fix (
src/js/node/_http_server.ts)lib/_http_server.js.http.server.response.created({ request, response }) fromthe
ServerResponseconstructor, so directnew http.ServerResponse(req)(light-my-request,
fastify.inject(), mocks) also fires it.http.server.request.start(
{ request, response, socket, server }) once per non-upgrade requestbefore the dispatch branching, so it fires on the normal,
checkContinue,checkExpectation, 417, anddropRequest/503 paths.http.server.response.finishlistener unconditionally(matching Node's
resOnFinish), with thehasSubscribersguardinside the handler, so a subscriber added between request arrival and
response finish is still observed.
hasSubscribersguard so nopayload object is allocated when nobody is listening.
Note on
dc.subscribewithout a held channel referencedc.subscribe("http.server.request.start", cb)without keeping areference to the channel object can have the channel GC'd and the
subscription dropped. That is a separate
WeakReferenceGC bug indiagnostics_channel.ts, tracked/fixed in #26537, so it is out of scopehere.
How did you verify your code works?
New tests in
test/js/node/diagnostics_channel/diagnostics_channel.test.tscover ordering (
response.created→request.start→ handler →response.finish), payload shapes and object identity, the headervisibility contract (
response.createdfires before the handler mutatesthe response,
response.finishsees the final value),request.starton the checkContinue / 417 paths, no
request.starton the upgradepath, late subscription to
response.finish, directnew http.ServerResponse()construction, and the no-subscriber path.Verified against Node's observable contract from
test/parallel/test-diagnostics-channel-http.jsandtest-diagnostic-channel-http-response-created.js. All 14 tests pass;they fail (6/8) when
_http_server.tsis reverted to main, so the gateholds. Rebuilt against current main (post Rust-rewrite,
bun test v1.4.0).[review] gate passed · iteration 8 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 8
evidence per changed file
root cause · written by the author bot
Bun's node:http server implementation never published to the standard Node.js server-side diagnostics channels (http.server.request.start, http.server.response.created, and http.server.response.finish), so observability libraries subscribing to them were silently never invoked. The fix instruments the server request and response lifecycle to publish the expected {request, response, socket, server} payloads at each point, lazily creating the channels and publishing only when subscribers exist. On both the plain http path and the http2 allowHTTP1 fallback, the response.finish publish fires fr…