fix(http2): emit server 'session' event promptly (before client 'connect')#5090
Conversation
…ect') Perry deferred the http2 server 'session' event until after the in-process client's 'connect' event, matching a macOS-node quirk. Causally the server creates its session and sends its SETTINGS frame before a client can complete its connect handshake, so 'session' must fire before 'connect' (Node on Linux: server>client). Remove the deferral: drain Session events before ClientConnect and delete the local_server_session_ready gate plus the H2_CLIENT_CONNECT_EMITTED /mark_client_connect_emitted machinery that existed only to support it.
📝 WalkthroughWalkthroughHTTP/2 event processing is simplified by removing per-session deferral bookkeeping and reordering event emission. Session events now sort before ClientConnect events, and deferral logic is eliminated, allowing events to emit based on global ordering and existing readiness checks. ChangesHTTP/2 Event Ordering and Deferral Simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-ext-http-server/src/http2_server.rs (1)
954-989:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't requeue
ClientConnectbased on server-session liveness after runningsessionlisteners.With the new
Session-first drain order, a server'session'listener can synchronously callclose()/destroy()on that session before Line 986 runs. At that pointhas_active_server_session()is false, so the sameClientConnectgets requeued forever and the client's'connect'callback can be starved on every pump iteration.Possible localized fix
fn has_emitted_server_session(server_handle: i64) -> bool { let mut emitted = false; iter_handles_of::<Http2SessionHandle, _>(|session| { - if session.server_handle == server_handle - && session.session_event_emitted - && !session.closed - && !session.destroyed - { + if session.server_handle == server_handle && session.session_event_emitted { emitted = true; } }); emitted } fn local_client_connect_ready(session_handle: i64) -> bool { let Some(server_handle) = local_server_handle_for_client(session_handle) else { return true; }; - has_active_server_session(server_handle) + has_active_server_session(server_handle) || has_emitted_server_session(server_handle) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-ext-http-server/src/http2_server.rs` around lines 954 - 989, The ClientConnect requeue logic can starve the client if a preceding Session listener synchronously closed the server session; update the ClientConnect handling so it does not requeue purely because has_active_server_session() is now false after running session listeners. Concretely: in the Http2PendingEvent::ClientConnect branch, change the readiness check (local_client_connect_ready(session_handle)) or its use so that if the associated Http2SessionHandle has session_event_emitted == true (set earlier in the Http2PendingEvent::Session branch) you proceed with the connect instead of calling push_h2_event and continuing; alternatively, make local_client_connect_ready consult session_event_emitted (or skip has_active_server_session) so a client connect won't be requeued forever. Ensure the code still requeues only for truly not-yet-ready sessions and keep push_h2_event as the fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/perry-ext-http-server/src/http2_server.rs`:
- Around line 954-989: The ClientConnect requeue logic can starve the client if
a preceding Session listener synchronously closed the server session; update the
ClientConnect handling so it does not requeue purely because
has_active_server_session() is now false after running session listeners.
Concretely: in the Http2PendingEvent::ClientConnect branch, change the readiness
check (local_client_connect_ready(session_handle)) or its use so that if the
associated Http2SessionHandle has session_event_emitted == true (set earlier in
the Http2PendingEvent::Session branch) you proceed with the connect instead of
calling push_h2_event and continuing; alternatively, make
local_client_connect_ready consult session_event_emitted (or skip
has_active_server_session) so a client connect won't be requeued forever. Ensure
the code still requeues only for truly not-yet-ready sessions and keep
push_h2_event as the fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 75284f74-5336-4dbf-9c7b-2e461136b691
📒 Files selected for processing (1)
crates/perry-ext-http-server/src/http2_server.rs
Problem
Perry deferred the http2 server
'session'event until after the in-process client's'connect'event. This is causally wrong: an http2 server creates its session and sends its SETTINGS frame before a client can complete its connect handshake, so the server'session'event must fire before the client'connect'. Node on Linux does exactly this (server>client).macOS node happens to defer it, and a prior Perry change was tuned to that macOS quirk, adding an explicit deferral mechanism that is incorrect for the canonical Linux sweep environment.
Fix
In
crates/perry-ext-http-server/src/http2_server.rsprocess_pending_h2_events():Sessionevents are processed beforeClientConnect(was the reverse).local_server_session_ready()gate that re-queued theSessionevent behind the client'sconnect.H2_CLIENT_CONNECT_EMITTEDthread-local,mark_client_connect_emitted(), and its call site.Net: +7 / -64 lines, runtime emit-scheduling only — no new dispatch entries.
Verification
Target tests (run 3x, deterministic):
http2/session/sequential-session-cleanup.ts→probe order: server>client/probe settings cb: true 65535http2/session/controls.ts→server controls:andserver settings shape:now appear early (beforeping return: true)No-regression: all other http/http2/https/net tests still match local node (43/44; the 1 fail,
net/server/connection-state-limits.ts, is pre-existing — reproduced identically on pristine origin/main, unrelatedgetConnectionstiming in the untouched net path).cargo testfor perry-runtime/perry-stdlib/perry-codegen green (single-threaded; the parallel-run reds are unrelated global-state flakes — Date prototype / handle counters).Summary by CodeRabbit