http2: fix onread assert when destroying session from stream handler - #65116
http2: fix onread assert when destroying session from stream handler#65116sankalpsthakur wants to merge 3 commits into
Conversation
|
Review requested:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65116 +/- ##
==========================================
- Coverage 89.99% 89.98% -0.02%
==========================================
Files 757 757
Lines 257739 257750 +11
Branches 48881 48879 -2
==========================================
- Hits 231961 231943 -18
- Misses 16861 16899 +38
+ Partials 8917 8908 -9
🚀 New features to boost your workflow:
|
pimterry
left a comment
There was a problem hiding this comment.
Thanks @sankalpsthakur!
Mostly looks good. The 3 fixes seem to all separately solve the same issue redundantly, is that right? I think for the first two that's OK and they're independently valuable (with minor tweaks) but we should drop the 3rd chunk to avoid missing major issues in future.
There's also a failing lint here, you can fix this locally with make format-cpp.
|
@pimterry ptal |
ae6f128 to
9cd30dd
Compare
When session.destroy() runs from a 'stream' handler, MakeCallback drains nextTick while nghttp2 is still inside mem_recv. Close is deferred for that window (see nodejs#64166), so later HEADERS in the same buffer created C++ streams without a JS wrapper or onread, and DATA delivery aborted with Assertion failed: onread->IsFunction(). - Reject new streams while the session is closing - Destroy the C++ handle if on_headers runs after JS destroy - Drop DATA when onread is not installed (defensive) Fixes: nodejs#64850 Signed-off-by: Sankalp Thakur <[email protected]>
9cd30dd to
eaa9b7a
Compare
pimterry
left a comment
There was a problem hiding this comment.
LGTM, thanks for working through those fixes @sankalpsthakur!
When
session.destroy()is called from a'stream'handler,MakeCallbackdrainsnextTickwhile nghttp2 is still insidemem_recv. Session close is deferred for that window (from #64166), so later HEADERS in the same receive buffer created C++ streams without a JS wrapper /onread. Subsequent DATA delivery hit:Changes
OnBeginHeadersCallback) — RST withNGHTTP2_CANCELinstead.on_headersruns after JS destroy, destroy the C++ handle (mirrors the existingsession.closedpath).onreadis not a function rather than aborting.session.destroy()from the stream handler.Test plan
test/parallel/test-http2-session-destroy-stream-handler.js(40 rounds × 8 concurrent POSTs)Fixes #64850
AI/LLM disclosure