Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
stream: destroy wrapped streams on error
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 <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
ronag authored and MylesBorins committed Nov 16, 2020
1 parent decfc2a commit 8a10916
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
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
@@ -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 8a10916

Please sign in to comment.