Commit 3ed7835
stream: fix writev unhandled rejection in fromWeb
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: #62199
Signed-off-by: sangwook <rewq5991@gmail.com>
PR-URL: #62297
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>1 parent c400d46 commit 3ed7835
2 files changed
Lines changed: 59 additions & 6 deletions
File tree
- lib/internal/webstreams
- test/parallel
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -313,9 +313,8 @@ function newStreamWritableFromWritableStream(writableStream, options = kEmptyObj | |||
| 313 | 313 | ||
| 314 | 314 | writev(chunks, callback) { | |
| 315 | 315 | function done(error) { | |
| 316 | - error = error.filter((e) => e); | ||
| 317 | 316 | try { | |
| 318 | - callback(error.length === 0 ? undefined : error); | ||
| 317 | + callback(error); | ||
| 319 | 318 | } catch (error) { | |
| 320 | 319 | // In a next tick because this is happening within | |
| 321 | 320 | // a promise context, and if there are any errors | |
@@ -333,7 +332,7 @@ function newStreamWritableFromWritableStream(writableStream, options = kEmptyObj | |||
| 333 | 332 | SafePromiseAll( | |
| 334 | 333 | chunks, | |
| 335 | 334 | (data) => writer.write(data.chunk)), | |
| 336 | - done, | ||
| 335 | + () => done(), | ||
| 337 | 336 | done); | |
| 338 | 337 | }, | |
| 339 | 338 | done); | |
@@ -775,9 +774,8 @@ function newStreamDuplexFromReadableWritablePair(pair = kEmptyObject, options = | |||
| 775 | 774 | ||
| 776 | 775 | writev(chunks, callback) { | |
| 777 | 776 | function done(error) { | |
| 778 | - error = error.filter((e) => e); | ||
| 779 | 777 | try { | |
| 780 | - callback(error.length === 0 ? undefined : error); | ||
| 778 | + callback(error); | ||
| 781 | 779 | } catch (error) { | |
| 782 | 780 | // In a next tick because this is happening within | |
| 783 | 781 | // a promise context, and if there are any errors | |
@@ -795,7 +793,7 @@ function newStreamDuplexFromReadableWritablePair(pair = kEmptyObject, options = | |||
| 795 | 793 | SafePromiseAll( | |
| 796 | 794 | chunks, | |
| 797 | 795 | (data) => writer.write(data.chunk)), | |
| 798 | - done, | ||
| 796 | + () => done(), | ||
| 799 | 797 | done); | |
| 800 | 798 | }, | |
| 801 | 799 | done); | |
Lines changed: 55 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -0,0 +1,55 @@ | |||
| 1 | + 'use strict'; | ||
| 2 | + | ||
| 3 | + // Regression test for https://github.com/nodejs/node/issues/62199 | ||
| 4 | + // | ||
| 5 | + // When Duplex.fromWeb is corked, writes are batched into _writev. If destroy() | ||
| 6 | + // is called in the same microtask (after uncork()), writer.ready rejects with a | ||
| 7 | + // non-array value. The done() callback inside _writev unconditionally called | ||
| 8 | + // error.filter(), which throws TypeError on non-arrays. This TypeError became | ||
| 9 | + // an unhandled rejection that crashed the process. | ||
| 10 | + // | ||
| 11 | + // The same bug exists in newStreamWritableFromWritableStream (Writable.fromWeb). | ||
| 12 | + | ||
| 13 | + const common = require('../common'); | ||
| 14 | + const { Duplex, Writable } = require('stream'); | ||
| 15 | + const { TransformStream, WritableStream } = require('stream/web'); | ||
| 16 | + | ||
| 17 | + // Exact reproduction from the issue report (davidje13). | ||
| 18 | + // Before the fix: process crashes with unhandled TypeError. | ||
| 19 | + // After the fix: stream closes cleanly with no unhandled rejection. | ||
| 20 | + { | ||
| 21 | + const output = Duplex.fromWeb(new TransformStream()); | ||
| 22 | + | ||
| 23 | + output.on('close', common.mustCall()); | ||
| 24 | + | ||
| 25 | + output.cork(); | ||
| 26 | + output.write('test'); | ||
| 27 | + output.write('test'); | ||
| 28 | + output.uncork(); | ||
| 29 | + output.destroy(); | ||
| 30 | + } | ||
| 31 | + | ||
| 32 | + // Same bug in Writable.fromWeb (newStreamWritableFromWritableStream). | ||
| 33 | + { | ||
| 34 | + const writable = Writable.fromWeb(new WritableStream()); | ||
| 35 | + | ||
| 36 | + writable.on('close', common.mustCall()); | ||
| 37 | + | ||
| 38 | + writable.cork(); | ||
| 39 | + writable.write('test'); | ||
| 40 | + writable.write('test'); | ||
| 41 | + writable.uncork(); | ||
| 42 | + writable.destroy(); | ||
| 43 | + } | ||
| 44 | + | ||
| 45 | + // Regression: normal cork/uncork/_writev success path must still work. | ||
| 46 | + // Verifies that () => done() correctly signals success via callback(). | ||
| 47 | + { | ||
| 48 | + const writable = Writable.fromWeb(new WritableStream({ write() {} })); | ||
| 49 | + | ||
| 50 | + writable.cork(); | ||
| 51 | + writable.write('foo'); | ||
| 52 | + writable.write('bar'); | ||
| 53 | + writable.uncork(); | ||
| 54 | + writable.end(common.mustCall()); | ||
| 55 | + } | ||
0 commit comments