Skip to content

Commit

Permalink
stream: destroy wrapped streams on error
Browse files Browse the repository at this point in the history
Stream should be destroyed and update state accordingly when
the wrapped stream emits error.

Does some additional cleanup with future TODOs that might be worth
looking into.

Backport-PR-URL: #35557
PR-URL: #34102
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
  • Loading branch information
ronag authored and MylesBorins committed Nov 3, 2020
1 parent 3e297cf commit e40b78c
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 5 deletions.
28 changes: 23 additions & 5 deletions lib/_stream_readable.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ ObjectSetPrototypeOf(Readable.prototype, Stream.prototype);
ObjectSetPrototypeOf(Readable, Stream);

const { errorOrDestroy } = destroyImpl;
const kProxyEvents = ['error', 'close', 'destroy', 'pause', 'resume'];

function prependListener(emitter, event, fn) {
// Sadly this is not cacheable as some libraries bundle their own
Expand Down Expand Up @@ -1055,10 +1054,29 @@ Readable.prototype.wrap = function(stream) {
}
}

// Proxy certain important events.
for (const kProxyEvent of kProxyEvents) {
stream.on(kProxyEvent, this.emit.bind(this, kProxyEvent));
}
stream.on('error', (err) => {
errorOrDestroy(this, err);
});

stream.on('close', () => {
// TODO(ronag): Update readable state?
this.emit('close');
});

stream.on('destroy', () => {
// TODO(ronag): this.destroy()?
this.emit('destroy');
});

stream.on('pause', () => {
// TODO(ronag): this.pause()?
this.emit('pause');
});

stream.on('resume', () => {
// TODO(ronag): this.resume()?
this.emit('resume');
});

// When we try to consume some more bytes, simply unpause the
// underlying stream.
Expand Down
31 changes: 31 additions & 0 deletions test/parallel/test-stream2-readable-wrap-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';
const common = require('../common');
const assert = require('assert');

const Readable = require('_stream_readable');
const EE = require('events').EventEmitter;

class LegacyStream extends EE {
pause() {}
resume() {}
}

{
const oldStream = new LegacyStream();
const r = new Readable({ autoDestroy: true })
.wrap(oldStream)
.on('error', common.mustCall(() => {
assert.strictEqual(r.destroyed, true);
}));
oldStream.emit('error', new Error());
}

{
const oldStream = new LegacyStream();
const r = new Readable({ autoDestroy: false })
.wrap(oldStream)
.on('error', common.mustCall(() => {
assert.strictEqual(r.destroyed, false);
}));
oldStream.emit('error', new Error());
}

0 comments on commit e40b78c

Please sign in to comment.