fix(readable): consume a body whose end has already been emitted - #5617
Merged
Conversation
consumeStart() is a free function, invoked as consumeStart(stream[kConsume]) from
the queueMicrotask() above it, so there is no `this` inside it. The endEmitted
branch nevertheless read this[kConsume] and this._readableState.encoding:
TypeError: Cannot read properties of undefined (reading 'Symbol(kConsume)')
at consumeStart (lib/api/readable.js:490:20)
at lib/api/readable.js:450:9
at node:internal/process/task_queues:149:7
The two lines are identical to the ones in the on('end') callback just below,
where they are correct because the emitter supplies the stream as the receiver.
Because the throw happens on a microtask rather than in the promise chain, the
caller cannot catch it: awaiting the consume inside try/catch still ends the
process. Draining a body and then deciding to read it is enough to get there,
which the added test does with an empty body.
The early return is part of the fix rather than tidying: consumeEnd() clears
consume.stream, and the resume() a few lines down would then dereference null.
Signed-off-by: marko1olo <marko1olo@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5617 +/- ##
========================================
Coverage 93.50% 93.51%
========================================
Files 110 110
Lines 38303 38429 +126
========================================
+ Hits 35816 35935 +119
- Misses 2487 2494 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Fixes #5614.
consumeStart()is a free function, called asconsumeStart(stream[kConsume])from thequeueMicrotask()above it, sothisisundefinedinside it under'use strict'. Thestate.endEmittedbranch readthis[kConsume]anyway:The two lines are character-for-character the same as the ones in the
on('end')callback immediately below, which is what makes it easy to read past — those work, because the emitter invokes the callback with the stream as its receiver.The part that makes it worth fixing rather than tidying is that the caller cannot catch it. The throw happens on a microtask rather than in the promise chain, so
awaiting the consume insidetry/catchstill takes the process down. Draining a body and then deciding to read it after all is enough to reach it.On the early return
Correcting only the receiver moves the crash rather than removing it:
consumeEnd()setsconsume.streamtonull, and theconsume.stream.resume()a few lines down then dereferences it — reachable only once the branch above stops throwing first. So thereturnis part of the fix.Tests
One case added to
test/readable.js: an empty body, drained, then consumed. Without the change the test runner reports it as an uncaught exception rather than a failed assertion, which is the same shape as the bug:test/readable.js11/11 andtest/client-request.js48/48 with the change; eslint clean.Possible drawbacks
The
endEmittedpath is reached by consuming a body after'end', and I have only exercised it with an empty body and with a drained 204 over a real request. A body that emitted data first is disturbed and rejects withTypeError: unusablebefore reaching this code, both before and after — I checked that separately so the change is not masking it.Node 24 on Windows only; I have not run the other matrix cells locally. I did not run
npm run test:typescript, which fails on cleanmainin Git Bash for an unrelated reason (the script relies on shell glob expansion fortypes/*.d.ts).