fix(ext-http-server): defer 'listening' emit + listen callback to the event-loop tick; bind this=server in request handlers (#4903)#4924
Merged
Conversation
a6fade5 to
5484d0d
Compare
…nt-loop tick; bind this=server in request handlers (#4903) Node never invokes the listen(port, cb) callback synchronously from inside listen() — 'listening' is emitted on a later event-loop tick, after the current synchronous script segment finishes. Perry fired it inline, so the canonical corpus shape const server = http.createServer().listen(0, () => { const port = server.address().port; }); ran the callback before the `const server` assignment completed and threw "Cannot read properties of undefined (reading 'address')". The address() method itself and the synchronous ephemeral-port bind were already correct (#2132) — the report's "address() returns undefined" was really "`server` is undefined". Fix, across the http / https / http2 listen paths: - listen() now records a pending 'listening' emit and registers the callback as a once 'listening' listener (Node's exact mechanism, so emit order vs. listeners added before/after listen() matches, and listeners registered after listen() returned still fire). - The main-thread pump (js_node_http_server_process_pending) drains the pending emit at the top of each tick, firing callbacks with implicit `this` bound to the server. The queue is detached before any callback runs (re-entrant listen() can't double-fire), server_is_active keeps the loop alive while an emit is queued, and the GC root scanner pins the queued closure pointers. - 'request' listeners and the createServer(handler) handler are now also invoked with `this` = server (Node emitter semantics), so the corpus `function (req, res) { this.address().port }` idiom works. Binding wraps only the synchronous call; microtasks run outside. Of the 18 corpus tests in #4903: test-http-agent-null, test-http-client-encoding, test-http-host-headers now pass byte-for-byte; the rest get past the listen/address wall and fail on documented sibling-ticket classes (#4904/#4905/#4906/#4909/#4910). The pre-existing chained + async-res.end exit-hang reproduces on main's binary too and stays with #4909. New e2e regression test covers the chained shape, deferred-emit ordering, late 'listening' registration, and this-binding in both the listen callback and the request handler. Fixes #4903
5484d0d to
a8ee4d3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4903 — the keystone of the node:http/https runtime-fail tail (#2132).
Node never invokes the
listen(port, cb)callback synchronously from insidelisten(): the'listening'event is emitted on a later event-loop tick, after the current synchronous script segment finishes. Perry fired it inline, so the canonical corpus shaperan the callback before the
const serverassignment completed and threwCannot read properties of undefined (reading 'address'). Theaddress()method itself and the synchronous ephemeral-port bind were already correct since #2132 — the issue's "address() returns undefined" was really "serveris undefined".Changes (
crates/perry-ext-http-server/, http + https + http2 listen paths)listen()now records a pending'listening'emit and registers the callback as a once'listening'listener — Node's exact mechanism, so the emit order vs. listeners added before/afterlisten()matches (pre-registered → listen callback → late-registered), and'listening'listeners registered afterlisten()returned still fire.js_node_http_server_process_pending) drains the pending emit at the top of each tick, firing every callback with implicitthisbound to the server. The queue is detached before any callback runs (re-entrantlisten()can't double-fire),server_is_activekeeps the event loop alive while an emit is queued, and the GC root scanner pins the queued closure pointers.'request'listeners and thecreateServer(handler)handler are now also invoked withthis= server (Node's emitter semantics), so the corpusfunction (req, res) { this.address().port }handler idiom works. The binding wraps only the synchronous call; microtasks run outside it.Validation
Of the 18 corpus tests listed in #4903 (Node v22
test/parallel, run via thescripts/node_core_subset.pystaging):test-http-agent-null,test-http-client-encoding,test-http-host-headers.newof internal classes), node:http: missing server/timer/socket lifecycle methods (closeAllConnections, setTimeout, timer.unref, ...) #4905 (missing req/socket methods), node:https: client rejects self-signed test certs; rejectUnauthorized/ca/checkServerIdentity not honored #4906 (https self-signed-cert rejection), node:http/https: 89 tests exit non-zero with no output (triage; likely server-hang/framing, partly cascades from #4903) #4909 (hangs), node:http: 2 remaining byte-level diffs (timeout-overflow, many-ended-pipelines) — final piece of #2132 original scope #4910 (byte diffs).res.endexit-hang reproduces with main's binary too (non-chained shape exits cleanly on both) and stays with node:http/https: 89 tests exit non-zero with no output (triage; likely server-hang/framing, partly cascades from #4903) #4909.await new Promise(r => server.listen(0, r))(axios.get + node:http: process hangs after server.close() — event loop never drains #604 shape) still resolves correctly.test_http_createserver_v8,test_issue_2153_http_typed_feedback_dispatch,test_issue_2533_aliased_http_createserver,test_node_http_client_request,test_issue_2208_http_inline_request_chain,test_issue_1124_http_buffer_body) behave identically to main.crates/perry/tests/issue_4903_listen_callback_deferred.rscovers the chained shape, deferred-emit ordering, late'listening'registration, andthis-binding in both the listen callback and the request handler.cargo test -p perry-ext-http-servergreen; fmt / file-size / GC store-site / addr-class lint gates green.A full http+https corpus sweep against the pinned Node v22 corpus is running; I'll post the pass-delta vs the #2132 baseline (38/448) as a follow-up comment.
Per the external-metadata convention, no version bump / changelog entry in this PR — to be folded in at merge time.