Commit 57e59ea
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 d1c517f commit 57e59ea
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 | |
|---|---|---|---|
@@ -314,9 +314,8 @@ function newStreamWritableFromWritableStream(writableStream, options = kEmptyObj | |||
| 314 | 314 | ||
| 315 | 315 | writev(chunks, callback) { | |
| 316 | 316 | function done(error) { | |
| 317 | - error = error.filter((e) => e); | ||
| 318 | 317 | try { | |
| 319 | - callback(error.length === 0 ? undefined : error); | ||
| 318 | + callback(error); | ||
| 320 | 319 | } catch (error) { | |
| 321 | 320 | // In a next tick because this is happening within | |
| 322 | 321 | // a promise context, and if there are any errors | |
@@ -334,7 +333,7 @@ function newStreamWritableFromWritableStream(writableStream, options = kEmptyObj | |||
| 334 | 333 | SafePromiseAll( | |
| 335 | 334 | chunks, | |
| 336 | 335 | (data) => writer.write(data.chunk)), | |
| 337 | - done, | ||
| 336 | + () => done(), | ||
| 338 | 337 | done); | |
| 339 | 338 | }, | |
| 340 | 339 | done); | |
@@ -787,9 +786,8 @@ function newStreamDuplexFromReadableWritablePair(pair = kEmptyObject, options = | |||
| 787 | 786 | ||
| 788 | 787 | writev(chunks, callback) { | |
| 789 | 788 | function done(error) { | |
| 790 | - error = error.filter((e) => e); | ||
| 791 | 789 | try { | |
| 792 | - callback(error.length === 0 ? undefined : error); | ||
| 790 | + callback(error); | ||
| 793 | 791 | } catch (error) { | |
| 794 | 792 | // In a next tick because this is happening within | |
| 795 | 793 | // a promise context, and if there are any errors | |
@@ -807,7 +805,7 @@ function newStreamDuplexFromReadableWritablePair(pair = kEmptyObject, options = | |||
| 807 | 805 | SafePromiseAll( | |
| 808 | 806 | chunks, | |
| 809 | 807 | (data) => writer.write(data.chunk)), | |
| 810 | - done, | ||
| 808 | + () => done(), | ||
| 811 | 809 | done); | |
| 812 | 810 | }, | |
| 813 | 811 | 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