Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: enable autoDestroy by default #30623

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions doc/api/stream.md
Original file line number Diff line number Diff line change
Expand Up @@ -277,8 +277,8 @@ added: v0.9.4
The `'error'` event is emitted if an error occurred while writing or piping
data. The listener callback is passed a single `Error` argument when called.

The stream is not closed when the `'error'` event is emitted unless the
[`autoDestroy`][writable-new] option was set to `true` when creating the
The stream is closed when the `'error'` event is emitted unless the
[`autoDestroy`][writable-new] option was set to `false` when creating the
stream.

After `'error'`, no further events other than `'close'` *should* be emitted
Expand Down Expand Up @@ -1664,11 +1664,7 @@ const { Writable } = require('stream');

class MyWritable extends Writable {
constructor({ highWaterMark, ...options }) {
super({
highWaterMark,
autoDestroy: true,
emitClose: true
});
super({ highWaterMark });
// ...
}
}
Expand Down Expand Up @@ -1742,6 +1738,9 @@ changes:
pr-url: https:/nodejs/node/pull/22795
description: Add `autoDestroy` option to automatically `destroy()` the
stream when it emits `'finish'` or errors.
- version: REPLACEME
pr-url: https:/nodejs/node/pull/30623
description: Change `autoDestroy` option default to `true`.
-->

* `options` {Object}
Expand Down Expand Up @@ -1773,7 +1772,7 @@ changes:
* `final` {Function} Implementation for the
[`stream._final()`][stream-_final] method.
* `autoDestroy` {boolean} Whether this stream should automatically call
`.destroy()` on itself after ending. **Default:** `false`.
`.destroy()` on itself after ending. **Default:** `true`.

<!-- eslint-disable no-useless-constructor -->
```js
Expand Down Expand Up @@ -2018,6 +2017,9 @@ changes:
pr-url: https:/nodejs/node/pull/22795
description: Add `autoDestroy` option to automatically `destroy()` the
stream when it emits `'end'` or errors.
- version: REPLACEME
pr-url: https:/nodejs/node/pull/30623
description: Change `autoDestroy` option default to `true`.
-->

* `options` {Object}
Expand All @@ -2036,7 +2038,7 @@ changes:
* `destroy` {Function} Implementation for the
[`stream._destroy()`][readable-_destroy] method.
* `autoDestroy` {boolean} Whether this stream should automatically call
`.destroy()` on itself after ending. **Default:** `false`.
`.destroy()` on itself after ending. **Default:** `true`.

<!-- eslint-disable no-useless-constructor -->
```js
Expand Down
2 changes: 1 addition & 1 deletion lib/_http_incoming.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ function IncomingMessage(socket) {
};
}

Stream.Readable.call(this, streamOptions);
Stream.Readable.call(this, { autoDestroy: false, ...streamOptions });

this._readableState.readingMore = true;

Expand Down
8 changes: 2 additions & 6 deletions lib/_stream_readable.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ function ReadableState(options, stream, isDuplex) {
this.emitClose = !options || options.emitClose !== false;

// Should .destroy() be called after 'end' (and potentially 'finish')
this.autoDestroy = !!(options && options.autoDestroy);
this.autoDestroy = !options || options.autoDestroy !== false;

// Has it been destroyed
this.destroyed = false;
Expand Down Expand Up @@ -251,11 +251,7 @@ Readable.prototype._destroy = function(err, cb) {
};

Readable.prototype[EE.captureRejectionSymbol] = function(err) {
// TODO(mcollina): remove the destroyed if once errorEmitted lands in
// Readable.
if (!this.destroyed) {
this.destroy(err);
}
ronag marked this conversation as resolved.
Show resolved Hide resolved
this.destroy(err);
};

// Manually shove something into the read() buffer.
Expand Down
2 changes: 1 addition & 1 deletion lib/_stream_writable.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ function WritableState(options, stream, isDuplex) {
this.emitClose = !options || options.emitClose !== false;

// Should .destroy() be called after 'finish' (and potentially 'end')
this.autoDestroy = !!(options && options.autoDestroy);
this.autoDestroy = !options || options.autoDestroy !== false;

// Indicates whether the stream has errored. When true all write() calls
// should return false. This is needed since when autoDestroy
Expand Down
6 changes: 6 additions & 0 deletions lib/internal/fs/streams.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ function ReadStream(path, options) {
if (options.emitClose === undefined) {
options.emitClose = false;
}
if (options.autoDestroy === undefined) {
options.autoDestroy = false;
}

this[kFs] = options.fs || fs;

Expand Down Expand Up @@ -298,6 +301,9 @@ function WriteStream(path, options) {
if (options.emitClose === undefined) {
options.emitClose = false;
}
if (options.autoDestroy === undefined) {
options.autoDestroy = false;
}

this[kFs] = options.fs || fs;
if (typeof this[kFs].open !== 'function') {
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/http2/compat.js
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ function onStreamTimeout(kind) {

class Http2ServerRequest extends Readable {
constructor(stream, headers, options, rawHeaders) {
super(options);
super({ autoDestroy: false, ...options });
this[kState] = {
closed: false,
didRead: false,
Expand Down
1 change: 1 addition & 0 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -1782,6 +1782,7 @@ class Http2Stream extends Duplex {
constructor(session, options) {
options.allowHalfOpen = true;
options.decodeStrings = false;
options.autoDestroy = false;
super(options);
this[async_id_symbol] = -1;

Expand Down
1 change: 1 addition & 0 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ function Socket(options) {
options.allowHalfOpen = true;
// For backwards compat do not emit close on destroy.
options.emitClose = false;
options.autoDestroy = false;
// Handle strings directly.
options.decodeStrings = false;
stream.Duplex.call(this, options);
Expand Down
2 changes: 1 addition & 1 deletion lib/zlib.js
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ function ZlibBase(opts, mode, handle, { flush, finishFlush, fullFlush }) {
}
}

Transform.call(this, opts);
Transform.call(this, { autoDestroy: false, ...opts });
this._hadError = false;
this.bytesWritten = 0;
this._handle = handle;
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-stream-pipe-error-handling.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ const Stream = require('stream').Stream;
const R = Stream.Readable;
const W = Stream.Writable;

const r = new R();
const w = new W();
const r = new R({ autoDestroy: false });
const w = new W({ autoDestroy: false });
let removed = false;

r._read = common.mustCall(function() {
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-stream-unshift-read-race.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const assert = require('assert');

const stream = require('stream');
const hwm = 10;
const r = stream.Readable({ highWaterMark: hwm });
const r = stream.Readable({ highWaterMark: hwm, autoDestroy: false });
const chunks = 10;

const data = Buffer.allocUnsafe(chunks * hwm + Math.ceil(hwm / 2));
Expand Down
3 changes: 3 additions & 0 deletions test/parallel/test-stream-writable-null.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ const assert = require('assert');
const stream = require('stream');

class MyWritable extends stream.Writable {
constructor(options) {
super({ autoDestroy: false, ...options });
}
_write(chunk, encoding, callback) {
assert.notStrictEqual(chunk, null);
callback();
Expand Down
27 changes: 15 additions & 12 deletions test/parallel/test-stream-writable-write-cb-twice.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ const assert = require('assert');
const writable = new Writable({
write: common.mustCall((buf, enc, cb) => {
cb();
assert.throws(cb, {
code: 'ERR_MULTIPLE_CALLBACK',
name: 'Error'
});
cb();
})
});
writable.write('hi');
writable.on('error', common.expectsError({
code: 'ERR_MULTIPLE_CALLBACK',
type: Error
ronag marked this conversation as resolved.
Show resolved Hide resolved
}));
}

{
Expand All @@ -23,14 +24,15 @@ const assert = require('assert');
write: common.mustCall((buf, enc, cb) => {
cb();
process.nextTick(() => {
assert.throws(cb, {
code: 'ERR_MULTIPLE_CALLBACK',
name: 'Error'
});
cb();
});
})
});
writable.write('hi');
writable.on('error', common.expectsError({
code: 'ERR_MULTIPLE_CALLBACK',
type: Error
ronag marked this conversation as resolved.
Show resolved Hide resolved
}));
}

{
Expand All @@ -39,12 +41,13 @@ const assert = require('assert');
write: common.mustCall((buf, enc, cb) => {
process.nextTick(cb);
process.nextTick(() => {
assert.throws(cb, {
code: 'ERR_MULTIPLE_CALLBACK',
name: 'Error'
});
cb();
});
})
});
writable.write('hi');
writable.on('error', common.expectsError({
code: 'ERR_MULTIPLE_CALLBACK',
type: Error
BridgeAR marked this conversation as resolved.
Show resolved Hide resolved
ronag marked this conversation as resolved.
Show resolved Hide resolved
}));
}
2 changes: 1 addition & 1 deletion test/parallel/test-stream2-pipe-error-handling.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ const stream = require('stream');
stream.Readable.prototype.unpipe.call(this, dest);
};

const dest = new stream.Writable();
const dest = new stream.Writable({ autoDestroy: false });
dest._write = function(chunk, encoding, cb) {
cb();
};
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-stream2-writable.js
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ const helloWorldBuffer = Buffer.from('hello world');

{
// Verify writables cannot be piped
const w = new W();
const w = new W({ autoDestroy: false });
w._write = common.mustNotCall();
let gotError = false;
w.on('error', function() {
Expand Down