Skip to content

Commit bf8410a

Browse files
Han5991juanarbol
authored andcommitted
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 34bcc92 commit bf8410a

2 files changed

Lines changed: 59 additions & 6 deletions

File tree

‎lib/internal/webstreams/adapters.js‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -278,9 +278,8 @@ function newStreamWritableFromWritableStream(writableStream, options = kEmptyObj
278278

279279
writev(chunks, callback) {
280280
function done(error) {
281-
error = error.filter((e) => e);
282281
try {
283-
callback(error.length === 0 ? undefined : error);
282+
callback(error);
284283
} catch (error) {
285284
// In a next tick because this is happening within
286285
// a promise context, and if there are any errors
@@ -298,7 +297,7 @@ function newStreamWritableFromWritableStream(writableStream, options = kEmptyObj
298297
SafePromiseAll(
299298
chunks,
300299
(data) => writer.write(data.chunk)),
301-
done,
300+
() => done(),
302301
done);
303302
},
304303
done);
@@ -703,9 +702,8 @@ function newStreamDuplexFromReadableWritablePair(pair = kEmptyObject, options =
703702

704703
writev(chunks, callback) {
705704
function done(error) {
706-
error = error.filter((e) => e);
707705
try {
708-
callback(error.length === 0 ? undefined : error);
706+
callback(error);
709707
} catch (error) {
710708
// In a next tick because this is happening within
711709
// a promise context, and if there are any errors
@@ -723,7 +721,7 @@ function newStreamDuplexFromReadableWritablePair(pair = kEmptyObject, options =
723721
SafePromiseAll(
724722
chunks,
725723
(data) => writer.write(data.chunk)),
726-
done,
724+
() => done(),
727725
done);
728726
},
729727
done);
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff 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

Comments
 (0)