perry-ext-http: failed https/http2 listen emits 'error', never 'listening' (follow-up to #11144) - #11180
Conversation
…ing; distinguish busy postbox from no loop Addresses CodeRabbit review on #11144: - http2: a bind failure (Some((0, ..))) was treated as success and fired listening + the listen(cb) callback; https only printed to stderr. Both now queue the server error event, like http.Server.listen. - post_to_owner returns Accepted / NoRoute / Busy; only NoRoute is ENOTSUP, an exhausted retry budget is EAGAIN. Retries back off after 16 yields. - free the reserved id when adopt_stream refuses a SCHED_RR descriptor. - test_gap_listen_eaddrinuse.ts: fails on c1f36c2, matches Node 26.5.1.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughListen failures on HTTPS and HTTP/2 now queue server error events instead of reporting bind failures to stderr. Owner-post failures distinguish missing routes from exhausted retries. The changes also add regression coverage for EADDRINUSE across HTTP, HTTPS, h2c, and secure HTTP/2 servers. ChangesListen failure handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The reported listen-failure behavior has no remaining actionable issue in this review and is ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 8 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ready for a train. Follow-up to #11144; carries the CodeRabbit fixes that missed its merge. Bind failures (EADDRINUSE) on http2, http2-secure and https now emit 'error' only: no 'listening' event and no listen callback, matching Node. A full postbox reports EAGAIN instead of ENOTSUP, and a refused SCHED_RR descriptor frees its handle id. The new gap test test_gap_listen_eaddrinuse fails at c1f36c2 and passes on the branch. It touches perry-ext-http only. |
1 participant
Follow-up to #11144. This fixes the CodeRabbit findings that were in #11144's diff. That PR merged at
c1f36c25bbefore this fix could be pushed there.Fixes
A failed
listen()emits'error'and never'listening', on every server type.try_listen_on_turnloopreturnedSome((0, ..)). Both listen routes treated that as success, so they emitted'listening'and ran thelisten(cb)callback whileserver.listening === false.H2Listen::{Bound, Failed, NoLoop, NoTls}.Failedqueues the server's'error', and onlyBoundarms'listening'. The "tls config unavailable" message no longer covers the no-loop case.'error'fired.'error'likehttp.createServer. The posted no-loop/busy case is queued as'error'too, rather than printed.post_to_ownerseparates "no loop" from "owner busy".Posted::{Accepted, NoRoute, Busy}.NoRouteis reported asENOTSUP. An exhausted retry budget isEAGAIN.ENOTSUP.A SCHED_RR descriptor that
adopt_streamrefuses now frees its reserved handle id.Tests
test-files/test_gap_listen_eaddrinuse.ts(new). A server holds a port, then http, https, http2 and http2-secure eachlisten()on it. Node 26.5.1 printserror EADDRINUSE listen listening=false cb=false listeningEvent=falsefor all four.c1f36c25b(before the fix) it gets PARITY_FAIL:https: no-error …andhttp2*: no-error listening=false cb=true listeningEvent=true.post_outcome_separates_no_route_from_a_busy_owner(new unit test). It drives the retry policy through an injected poster. It could not be written against the oldboolreturn, which had no busy state.adopt_streamto fail needs a loop that refuses an attach, which no harness here has. I checked it by reading the code.Validation
Run on perrymaster (Linux) with Node 26.5.1, on a fresh build of this branch on main
c7d09635b(which includes main's turnloop alpha.8 bump).test_gap_listen_eaddrinuse,test_gap_turnloop_http2_server,test_gap_turnloop_https_server,test_gap_turnloop_http_server,cluster_4962.test_gap_http2_alpn_secure: PARITY_FAIL, as it already was on the perry-ext-http: node:http/https/http2 servers on turnloop only; drop hyper + hyper-util (tokio lane A2) #11144 base and branch.cargo test --release -p perry-ext-http: 146 pass, 1 fail. The failure istls_client::tests::needs_custom_client_logic, which already fails on main.cargo fmt --all -- --check,check_file_size.sh,tokio_inventory.pyandgc_runtime_root_holders.pyall OK.SKIP_COMPILE_GATES=1 run_lint_gates.sh: 87/88 script gates pass; the compile tier was not run. The one failure is its cargo-xwin step, which isn't installed on the host.cargo xwin check -p perry-ext-http -p perry-runtime -p perry-ffi --target x86_64-pc-windows-msvc: OK on the pre-rebase commit (private cargo-xwin v0.23.0 install). It shows the same 3 pre-existingcluster_bindwarnings.Not in this PR
Two findings were real but predate #11144, so I filed them as issues:
res.write()backpressure never emits'drain'.https.createServer'upgrade'listeners never fire.Not run
Summary by CodeRabbit
errorevent instead of incorrectly signaling that listening began or only logging the failure.EAGAIN;ENOTSUPis reserved for cases where no loop exists.