From 3d2064b44b78dfce2c1ea09f0406fe196054ec93 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 10 Nov 2021 10:45:10 +0200 Subject: [PATCH 1/2] stream: remove thenable support Remove support for returning thenables in stream implementation methods. This is causing more confusion and issues than it's worth. Refs: https://github.com/nodejs/node/issues/39535 --- doc/api/deprecations.md | 6 +- lib/internal/streams/destroy.js | 34 +-- lib/internal/streams/readable.js | 13 +- lib/internal/streams/transform.js | 67 +---- lib/internal/streams/writable.js | 19 +- .../test-stream-construct-async-error.js | 232 ------------------ 6 files changed, 11 insertions(+), 360 deletions(-) delete mode 100644 test/parallel/test-stream-construct-async-error.js diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 32b25ee9b87f88..e0d439dac3cb78 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -3022,11 +3022,15 @@ changes: - version: - v17.2.0 - v16.14.0 + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/40773 + description: End-of-life. + - version: v17.2.0 pr-url: https://github.com/nodejs/node/pull/40860 description: Documentation-only deprecation. --> -Type: Documentation-only +Type: End-of-Life An undocumented feature of Node.js streams was to support thenables in implementation methods. This is now deprecated, use callbacks instead and avoid diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js index efa09e05eafef0..10f5471e21d3eb 100644 --- a/lib/internal/streams/destroy.js +++ b/lib/internal/streams/destroy.js @@ -106,20 +106,7 @@ function _destroy(self, err, cb) { } } try { - const result = self._destroy(err || null, onDestroy); - if (result != null) { - const then = result.then; - if (typeof then === 'function') { - then.call( - result, - function() { - process.nextTick(onDestroy, null); - }, - function(err) { - process.nextTick(onDestroy, err); - }); - } - } + self._destroy(err || null, onDestroy); } catch (err) { onDestroy(err); } @@ -285,24 +272,7 @@ function constructNT(stream) { } try { - const result = stream._construct(onConstruct); - if (result != null) { - const then = result.then; - if (typeof then === 'function') { - then.call( - result, - function() { - if (!called) { - process.nextTick(onConstruct, null); - } - }, - function(err) { - if (!called) { - process.nextTick(onConstruct, err); - } - }); - } - } + stream._construct(onConstruct); } catch (err) { onConstruct(err); } diff --git a/lib/internal/streams/readable.js b/lib/internal/streams/readable.js index 4b7613fd13d081..8081e8fde13e0c 100644 --- a/lib/internal/streams/readable.js +++ b/lib/internal/streams/readable.js @@ -493,18 +493,7 @@ Readable.prototype.read = function(n) { // Call internal read method try { - const result = this._read(state.highWaterMark); - if (result != null) { - const then = result.then; - if (typeof then === 'function') { - then.call( - result, - nop, - function(err) { - errorOrDestroy(this, err); - }); - } - } + this._read(state.highWaterMark); } catch (err) { errorOrDestroy(this, err); } diff --git a/lib/internal/streams/transform.js b/lib/internal/streams/transform.js index cbd23185fad291..fdac76e4062b4b 100644 --- a/lib/internal/streams/transform.js +++ b/lib/internal/streams/transform.js @@ -107,10 +107,8 @@ function Transform(options) { } function final(cb) { - let called = false; if (typeof this._flush === 'function' && !this.destroyed) { - const result = this._flush((er, data) => { - called = true; + this._flush((er, data) => { if (er) { if (cb) { cb(er); @@ -128,33 +126,6 @@ function final(cb) { cb(); } }); - if (result !== undefined && result !== null) { - try { - const then = result.then; - if (typeof then === 'function') { - then.call( - result, - (data) => { - if (called) - return; - if (data != null) - this.push(data); - this.push(null); - if (cb) - process.nextTick(cb); - }, - (err) => { - if (cb) { - process.nextTick(cb, err); - } else { - process.nextTick(() => this.destroy(err)); - } - }); - } - } catch (err) { - process.nextTick(() => this.destroy(err)); - } - } } else { this.push(null); if (cb) { @@ -180,9 +151,7 @@ Transform.prototype._write = function(chunk, encoding, callback) { const wState = this._writableState; const length = rState.length; - let called = false; - const result = this._transform(chunk, encoding, (err, val) => { - called = true; + this._transform(chunk, encoding, (err, val) => { if (err) { callback(err); return; @@ -204,38 +173,6 @@ Transform.prototype._write = function(chunk, encoding, callback) { this[kCallback] = callback; } }); - if (result !== undefined && result != null) { - try { - const then = result.then; - if (typeof then === 'function') { - then.call( - result, - (val) => { - if (called) - return; - - if (val != null) { - this.push(val); - } - - if ( - wState.ended || - length === rState.length || - rState.length < rState.highWaterMark || - rState.length === 0) { - process.nextTick(callback); - } else { - this[kCallback] = callback; - } - }, - (err) => { - process.nextTick(callback, err); - }); - } - } catch (err) { - process.nextTick(callback, err); - } - } }; Transform.prototype._read = function() { diff --git a/lib/internal/streams/writable.js b/lib/internal/streams/writable.js index c8f1cd62861b4d..5ee0cbc969f2ad 100644 --- a/lib/internal/streams/writable.js +++ b/lib/internal/streams/writable.js @@ -693,24 +693,7 @@ function callFinal(stream, state) { state.pendingcb++; try { - const result = stream._final(onFinish); - if (result != null) { - const then = result.then; - if (typeof then === 'function') { - then.call( - result, - function() { - if (!called) { - process.nextTick(onFinish, null); - } - }, - function(err) { - if (!called) { - process.nextTick(onFinish, err); - } - }); - } - } + stream._final(onFinish); } catch (err) { onFinish(err); } diff --git a/test/parallel/test-stream-construct-async-error.js b/test/parallel/test-stream-construct-async-error.js deleted file mode 100644 index 3fe81b4ebe2d6b..00000000000000 --- a/test/parallel/test-stream-construct-async-error.js +++ /dev/null @@ -1,232 +0,0 @@ -'use strict'; - -const common = require('../common'); -const { - Duplex, - Writable, - Transform, -} = require('stream'); -const { setTimeout } = require('timers/promises'); -const assert = require('assert'); - -{ - class Foo extends Duplex { - async _destroy(err, cb) { - await setTimeout(common.platformTimeout(1)); - throw new Error('boom'); - } - } - - const foo = new Foo(); - foo.destroy(); - foo.on('error', common.expectsError({ - message: 'boom' - })); - foo.on('close', common.mustCall(() => { - assert(foo.destroyed); - })); -} - -{ - class Foo extends Duplex { - async _destroy(err, cb) { - await setTimeout(common.platformTimeout(1)); - } - } - - const foo = new Foo(); - foo.destroy(); - foo.on('close', common.mustCall(() => { - assert(foo.destroyed); - })); -} - -{ - class Foo extends Duplex { - async _construct() { - await setTimeout(common.platformTimeout(1)); - } - - _write = common.mustCall((chunk, encoding, cb) => { - cb(); - }); - - _read() {} - } - - const foo = new Foo(); - foo.write('test', common.mustCall()); -} - -{ - class Foo extends Duplex { - async _construct(callback) { - await setTimeout(common.platformTimeout(1)); - callback(); - } - - _write = common.mustCall((chunk, encoding, cb) => { - cb(); - }); - - _read() {} - } - - const foo = new Foo(); - foo.write('test', common.mustCall()); - foo.on('error', common.mustNotCall()); -} - -{ - class Foo extends Writable { - _write = common.mustCall((chunk, encoding, cb) => { - cb(); - }); - - async _final() { - await setTimeout(common.platformTimeout(1)); - } - } - - const foo = new Foo(); - foo.end('hello'); - foo.on('finish', common.mustCall()); -} - -{ - class Foo extends Writable { - _write = common.mustCall((chunk, encoding, cb) => { - cb(); - }); - - async _final(callback) { - await setTimeout(common.platformTimeout(1)); - callback(); - } - } - - const foo = new Foo(); - foo.end('hello'); - foo.on('finish', common.mustCall()); -} - -{ - class Foo extends Writable { - _write = common.mustCall((chunk, encoding, cb) => { - cb(); - }); - - async _final() { - await setTimeout(common.platformTimeout(1)); - throw new Error('boom'); - } - } - - const foo = new Foo(); - foo.end('hello'); - foo.on('error', common.expectsError({ - message: 'boom' - })); - foo.on('close', common.mustCall()); -} - -{ - const expected = ['hello', 'world']; - class Foo extends Transform { - async _flush() { - return 'world'; - } - - _transform(chunk, encoding, callback) { - callback(null, chunk); - } - } - - const foo = new Foo(); - foo.end('hello'); - foo.on('data', common.mustCall((chunk) => { - assert.strictEqual(chunk.toString(), expected.shift()); - }, 2)); -} - -{ - const expected = ['hello', 'world']; - class Foo extends Transform { - async _flush(callback) { - callback(null, 'world'); - } - - _transform(chunk, encoding, callback) { - callback(null, chunk); - } - } - - const foo = new Foo(); - foo.end('hello'); - foo.on('data', common.mustCall((chunk) => { - assert.strictEqual(chunk.toString(), expected.shift()); - }, 2)); -} - -{ - class Foo extends Transform { - async _flush(callback) { - throw new Error('boom'); - } - - _transform(chunk, encoding, callback) { - callback(null, chunk); - } - } - - const foo = new Foo(); - foo.end('hello'); - foo.on('data', common.mustCall()); - foo.on('error', common.expectsError({ - message: 'boom' - })); - foo.on('close', common.mustCall()); -} - -{ - class Foo extends Transform { - async _transform(chunk) { - return chunk.toString().toUpperCase(); - } - } - - const foo = new Foo(); - foo.end('hello'); - foo.on('data', common.mustCall((chunk) => { - assert.strictEqual(chunk.toString(), 'HELLO'); - })); -} - -{ - class Foo extends Transform { - async _transform(chunk, _, callback) { - callback(null, chunk.toString().toUpperCase()); - } - } - - const foo = new Foo(); - foo.end('hello'); - foo.on('data', common.mustCall((chunk) => { - assert.strictEqual(chunk.toString(), 'HELLO'); - })); -} - -{ - class Foo extends Transform { - async _transform() { - throw new Error('boom'); - } - } - - const foo = new Foo(); - foo.end('hello'); - foo.on('error', common.expectsError({ - message: 'boom' - })); - foo.on('close', common.mustCall()); -} From ae072367454fc12faa06c9431e05203bd4be0c4e Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 6 Apr 2022 17:59:23 +0200 Subject: [PATCH 2/2] Update doc/api/deprecations.md Co-authored-by: Antoine du Hamel --- doc/api/deprecations.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index e0d439dac3cb78..7c9904c2aa2244 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -3019,13 +3019,12 @@ it was an aborted or graceful destroy.