stream: fix writev unhandled rejection in fromWeb#62297
Open
Han5991 wants to merge 1 commit intonodejs:mainfrom
Open
stream: fix writev unhandled rejection in fromWeb#62297Han5991 wants to merge 1 commit intonodejs:mainfrom
Han5991 wants to merge 1 commit intonodejs:mainfrom
Conversation
When using Duplex.fromWeb() or Writable.fromWeb() with cork()/uncork(), writes are batched into _writev(). If destroy() is called in the same microtask, the underlying WritableStream writer gets aborted, causing SafePromiseAll() to reject with a non-array value (e.g. an AbortError). The done() callback in _writev() of both fromWeb adapter functions unconditionally called error.filter(), assuming the value was always an array. This caused a TypeError that became an unhandled rejection, crashing the process. Fix by separating the resolve and reject handlers of SafePromiseAll: use () => done() on the resolve path (all writes succeeded, no error) and done on the reject path (error passed directly to callback). Fixes: nodejs#62199
Member
|
#62291 is already open (although I believe the approach here is the correct one) |
Contributor
Author
|
Agreed — and since #62291 is already open, I've opened this as a separate issue because I believe the approach here is the correct one. @Renegade334 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62297 +/- ##
=======================================
Coverage 89.66% 89.66%
=======================================
Files 676 676
Lines 206564 206574 +10
Branches 39559 39554 -5
=======================================
+ Hits 185223 185232 +9
- Misses 13454 13471 +17
+ Partials 7887 7871 -16
🚀 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.
Summary
Fixes #62199
When using
Duplex.fromWeb()orWritable.fromWeb()withcork()/uncork(), writes are batched into_writev(). Ifdestroy()is called in the same microtask (afteruncork()), the underlyingWritableStreamwriter gets aborted, causingSafePromiseAll()to reject with a non-array value (e.g. anAbortErrorornull).The
done()callback in_writev()of bothnewStreamDuplexFromReadableWritablePairandnewStreamWritableFromWritableStreamunconditionally callederror.filter(), assuming the value was always an array fromSafePromiseAll. This caused aTypeError: Cannot read properties of null (reading 'filter')that became an unhandled rejection, crashing the process.Root cause
donewas used as both the resolve and reject handler forSafePromiseAll:done([undefined, undefined, ...])— array,.filter()worksdone(abortError)ordone(null)— non-array,.filter()throwsFix
Separate the resolve and reject handlers of
SafePromiseAll.Promise.allresolving means all writes succeeded — there is no error to report.Promise.allrejecting passes a single error value directly.() => done()— resolve path: all writes succeeded, call callback with no errordone— reject path: single error passed directly to callbackThe same fix is applied to both
newStreamDuplexFromReadableWritablePairandnewStreamWritableFromWritableStream.Test
Added
test/parallel/test-webstreams-duplex-fromweb-writev-unhandled-rejection.jswith the exact reproduction from the issue report (cork → write → write → uncork → destroy).