mirror of
https://github.com/nodejs/node.git
synced 2026-10-14 10:26:26 -04:00
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: https://github.com/nodejs/node/issues/62199 Signed-off-by: sangwook <rewq5991@gmail.com> PR-URL: https://github.com/nodejs/node/pull/62297 Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
This commit is contained in:
1 parent
9f0a3e636b
commit
20cacec5c6
2 files changed
+59
-6
No files matched your search
@@ -314,9 +314,8 @@ function newStreamWritableFromWritableStream(writableStream, options = kEmptyObj
|
||||
|
||||
writev(chunks, callback) {
|
||||
function done(error) {
|
||||
error = error.filter((e) => e);
|
||||
try {
|
||||
callback(error.length === 0 ? undefined : error);
|
||||
callback(error);
|
||||
} catch (error) {
|
||||
// In a next tick because this is happening within
|
||||
// a promise context, and if there are any errors
|
||||
@@ -334,7 +333,7 @@ function newStreamWritableFromWritableStream(writableStream, options = kEmptyObj
|
||||
SafePromiseAll(
|
||||
chunks,
|
||||
(data) => writer.write(data.chunk)),
|
||||
done,
|
||||
() => done(),
|
||||
done);
|
||||
},
|
||||
done);
|
||||
@@ -787,9 +786,8 @@ function newStreamDuplexFromReadableWritablePair(pair = kEmptyObject, options =
|
||||
|
||||
writev(chunks, callback) {
|
||||
function done(error) {
|
||||
error = error.filter((e) => e);
|
||||
try {
|
||||
callback(error.length === 0 ? undefined : error);
|
||||
callback(error);
|
||||
} catch (error) {
|
||||
// In a next tick because this is happening within
|
||||
// a promise context, and if there are any errors
|
||||
@@ -807,7 +805,7 @@ function newStreamDuplexFromReadableWritablePair(pair = kEmptyObject, options =
|
||||
SafePromiseAll(
|
||||
chunks,
|
||||
(data) => writer.write(data.chunk)),
|
||||
done,
|
||||
() => done(),
|
||||
done);
|
||||
},
|
||||
done);
|
||||
|
||||
@@ -0,0 +1,55 @@
|
||||
'use strict';
|
||||
|
||||
// Regression test for https://github.com/nodejs/node/issues/62199
|
||||
//
|
||||
// When Duplex.fromWeb is corked, writes are batched into _writev. If destroy()
|
||||
// is called in the same microtask (after uncork()), writer.ready rejects with a
|
||||
// non-array value. The done() callback inside _writev unconditionally called
|
||||
// error.filter(), which throws TypeError on non-arrays. This TypeError became
|
||||
// an unhandled rejection that crashed the process.
|
||||
//
|
||||
// The same bug exists in newStreamWritableFromWritableStream (Writable.fromWeb).
|
||||
|
||||
const common = require('../common');
|
||||
const { Duplex, Writable } = require('stream');
|
||||
const { TransformStream, WritableStream } = require('stream/web');
|
||||
|
||||
// Exact reproduction from the issue report (davidje13).
|
||||
// Before the fix: process crashes with unhandled TypeError.
|
||||
// After the fix: stream closes cleanly with no unhandled rejection.
|
||||
{
|
||||
const output = Duplex.fromWeb(new TransformStream());
|
||||
|
||||
output.on('close', common.mustCall());
|
||||
|
||||
output.cork();
|
||||
output.write('test');
|
||||
output.write('test');
|
||||
output.uncork();
|
||||
output.destroy();
|
||||
}
|
||||
|
||||
// Same bug in Writable.fromWeb (newStreamWritableFromWritableStream).
|
||||
{
|
||||
const writable = Writable.fromWeb(new WritableStream());
|
||||
|
||||
writable.on('close', common.mustCall());
|
||||
|
||||
writable.cork();
|
||||
writable.write('test');
|
||||
writable.write('test');
|
||||
writable.uncork();
|
||||
writable.destroy();
|
||||
}
|
||||
|
||||
// Regression: normal cork/uncork/_writev success path must still work.
|
||||
// Verifies that () => done() correctly signals success via callback().
|
||||
{
|
||||
const writable = Writable.fromWeb(new WritableStream({ write() {} }));
|
||||
|
||||
writable.cork();
|
||||
writable.write('foo');
|
||||
writable.write('bar');
|
||||
writable.uncork();
|
||||
writable.end(common.mustCall());
|
||||
}
|
||||
Reference in new issue
Block a user