Repository navigation
Conversation
When the peer reset a stream before sending END_STREAM, the compat `Http2ServerRequest` still pushed EOF from its 'close' handler, so it emitted 'end' for a truncated body (on main even after 'error' and 'close'). Unlike http1, where an aborted `IncomingMessage` is destroyed and never emits 'end', applications listening for 'end' then processed the partial body as if it were complete. Record in `onStreamClose()` that the peer closed the stream before END_STREAM, since `destroy()` ends the readable side afterwards, and destroy the request instead of ending it in that case. Requests whose body was complete before the reset still end normally. Signed-off-by: Matteo Collina <hello@matteocollina.com>
|
Review requested:
|
nodejs-github-bot
added
http2
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #67061 +/- ##
=======================================
Coverage 90.65% 90.65%
=======================================
Files 797 797
Lines 278268 278284 +16
Branches 53517 53518 +1
=======================================
+ Hits 252261 252283 +22
Misses 16386 16386
+ Partials 9621 9615 -6
🚀 New features to boost your workflow:
|
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
This looks good, but only covers remote stream close. Local close is skipped (because of the locallyInitiated branch above where kIncomplete is now set) and unexpected session closure too, and both of those will hit the same bug.
Can we fix these en route? I think all 3 cases can be covered at once via h2Stream._destroy().
|
The semver-major question is interesting, I think I'd lean to calling this a simple bugfix, but it's debatable and certainly could cause some issues... There's a large breaking change coming here in v27 already for this H2 logic anyway (#63249), and before that the previous code and implied API expectations were very different, so this would need manual backporting and in the old model (where we ignored lots more errors) it might be more surprising for users. I'd say we ship this in v27 alongside #63249, and don't worry about backporting for now unless people start reporting real issues in the meantime (unlikely, since this bug has been around for ages). |
When a client resets a stream (
RST_STREAM, with any error code includingNO_ERROR) before sending END_STREAM, the compatHttp2ServerRequeststill pushes EOF from its'close'handler and emits'end'for the truncated body. On main,'end'even arrives after'error'and'close'.node:httpbehaves differently: an abortedIncomingMessageemits'aborted'and'error'(ECONNRESET) and is destroyed, never emitting'end'. Code that only listens for'end'therefore processes a partial HTTP/2 request body as if it were complete.onStreamClose()already knows whether the peer ended its side before closing, butdestroy()then ends the readable side, so the compat layer can't tell afterwards. This records it on the stream, and the compat request is destroyed instead of ended in that case. Requests whose body was complete before the reset still emit'end'as before, andrequest.completekeeps its documented semantics.This changes observable behavior for compat requests reset mid-body (no more
'end';stream.finished()and async iteration now report a premature close), so reviewers may want to weigh in on the semver label.