From a055bcfd8a86bf0c1df0582a0f45fc351a1e06e3 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 10 Nov 2021 10:45:10 +0200 Subject: [PATCH 1/6] stream: deprecate thenable support Deprecate support for returning thenables in stream implementation methods. This is causing more confusion and issues than it's worth, and never was documented. Refs: https://github.com/nodejs/node/issues/39535 Co-authored-by: Antoine du Hamel --- doc/api/deprecations.md | 14 ++++++++++++++ lib/internal/streams/destroy.js | 3 +++ lib/internal/streams/readable.js | 2 ++ lib/internal/streams/transform.js | 3 +++ lib/internal/streams/utils.js | 13 +++++++++++++ lib/internal/streams/writable.js | 2 ++ test/parallel/test-stream-construct-async-error.js | 6 ++++++ 7 files changed, 43 insertions(+) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index e60ccd18cf9027..43b90c5277c727 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -3019,6 +3019,20 @@ should have the same effect. The receiving end should also check the [`readable.readableEnded`][] value on [`http.IncomingMessage`][] to get whether it was an aborted or graceful destroy. +### DEP0157: Thenable support in streams + + + +Type: Runtime + +An undocumented feature of Node.js streams was to support thenables in +implementation methods. This is now deprecated, instead use callbacks. + [Legacy URL API]: url.md#legacy-url-api [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf [RFC 6066]: https://tools.ietf.org/html/rfc6066#section-3 diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js index efa09e05eafef0..44f9855e81612d 100644 --- a/lib/internal/streams/destroy.js +++ b/lib/internal/streams/destroy.js @@ -11,6 +11,7 @@ const { Symbol, } = primordials; const { + emitThenableDeprecationWarning, kDestroyed, isDestroyed, isFinished, @@ -110,6 +111,7 @@ function _destroy(self, err, cb) { if (result != null) { const then = result.then; if (typeof then === 'function') { + emitThenableDeprecationWarning(); then.call( result, function() { @@ -289,6 +291,7 @@ function constructNT(stream) { if (result != null) { const then = result.then; if (typeof then === 'function') { + emitThenableDeprecationWarning(); then.call( result, function() { diff --git a/lib/internal/streams/readable.js b/lib/internal/streams/readable.js index d0125386c8ae8e..29a694db2e763d 100644 --- a/lib/internal/streams/readable.js +++ b/lib/internal/streams/readable.js @@ -73,6 +73,7 @@ const kPaused = Symbol('kPaused'); const { StringDecoder } = require('string_decoder'); const from = require('internal/streams/from'); +const { emitThenableDeprecationWarning } = require('internal/streams/utils'); ObjectSetPrototypeOf(Readable.prototype, Stream.prototype); ObjectSetPrototypeOf(Readable, Stream); @@ -497,6 +498,7 @@ Readable.prototype.read = function(n) { if (result != null) { const then = result.then; if (typeof then === 'function') { + emitThenableDeprecationWarning(); then.call( result, nop, diff --git a/lib/internal/streams/transform.js b/lib/internal/streams/transform.js index 26e0b07c2956c8..58aa37f26aa1c2 100644 --- a/lib/internal/streams/transform.js +++ b/lib/internal/streams/transform.js @@ -73,6 +73,7 @@ const { ERR_METHOD_NOT_IMPLEMENTED } = require('internal/errors').codes; const Duplex = require('internal/streams/duplex'); +const { emitThenableDeprecationWarning } = require('internal/streams/utils'); ObjectSetPrototypeOf(Transform.prototype, Duplex.prototype); ObjectSetPrototypeOf(Transform, Duplex); @@ -132,6 +133,7 @@ function final(cb) { try { const then = result.then; if (typeof then === 'function') { + emitThenableDeprecationWarning(); then.call( result, (data) => { @@ -207,6 +209,7 @@ Transform.prototype._write = function(chunk, encoding, callback) { try { const then = result.then; if (typeof then === 'function') { + emitThenableDeprecationWarning(); then.call( result, (val) => { diff --git a/lib/internal/streams/utils.js b/lib/internal/streams/utils.js index 854daae9de41b8..1c7570f193a08a 100644 --- a/lib/internal/streams/utils.js +++ b/lib/internal/streams/utils.js @@ -241,7 +241,20 @@ function isDisturbed(stream) { )); } +let thenableDeprecationWarningEmitted = false; +function emitThenableDeprecationWarning() { + if (!thenableDeprecationWarningEmitted) { + process.emitWarning( + 'Returning a thenable is deprecated, use callbacks instead.', + 'DeprecationWarning', + 'DEP0157' + ); + thenableDeprecationWarningEmitted = true; + } +} + module.exports = { + emitThenableDeprecationWarning, kDestroyed, isDisturbed, kIsDisturbed, diff --git a/lib/internal/streams/writable.js b/lib/internal/streams/writable.js index 7c082c075642eb..3186bb47176c3a 100644 --- a/lib/internal/streams/writable.js +++ b/lib/internal/streams/writable.js @@ -44,6 +44,7 @@ const EE = require('events'); const Stream = require('internal/streams/legacy').Stream; const { Buffer } = require('buffer'); const destroyImpl = require('internal/streams/destroy'); +const { emitThenableDeprecationWarning } = require('internal/streams/utils'); const { addAbortSignal, @@ -696,6 +697,7 @@ function callFinal(stream, state) { if (result != null) { const then = result.then; if (typeof then === 'function') { + emitThenableDeprecationWarning(); then.call( result, function() { diff --git a/test/parallel/test-stream-construct-async-error.js b/test/parallel/test-stream-construct-async-error.js index ea2d8740e29c94..ffa001ec660b55 100644 --- a/test/parallel/test-stream-construct-async-error.js +++ b/test/parallel/test-stream-construct-async-error.js @@ -9,6 +9,12 @@ const { const { setTimeout } = require('timers/promises'); const assert = require('assert'); +common.expectWarning( + 'DeprecationWarning', + 'Returning a thenable is deprecated, use callbacks instead.', + 'DEP0157' +); + { class Foo extends Duplex { async _destroy(err, cb) { From 7422c7a114824c864178e004817d2d2ff8d63c42 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Thu, 18 Nov 2021 19:26:34 +0100 Subject: [PATCH 2/6] squash! downgrade to doc-only deprecation --- doc/api/deprecations.md | 6 +++--- lib/internal/streams/destroy.js | 3 --- lib/internal/streams/readable.js | 2 -- lib/internal/streams/transform.js | 3 --- lib/internal/streams/utils.js | 13 ------------- lib/internal/streams/writable.js | 2 -- test/parallel/test-stream-construct-async-error.js | 6 ------ 7 files changed, 3 insertions(+), 32 deletions(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 43b90c5277c727..f3b571f5baa3a9 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -3024,11 +3024,11 @@ it was an aborted or graceful destroy. -Type: Runtime +Type: Documentation-only An undocumented feature of Node.js streams was to support thenables in implementation methods. This is now deprecated, instead use callbacks. diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js index 44f9855e81612d..efa09e05eafef0 100644 --- a/lib/internal/streams/destroy.js +++ b/lib/internal/streams/destroy.js @@ -11,7 +11,6 @@ const { Symbol, } = primordials; const { - emitThenableDeprecationWarning, kDestroyed, isDestroyed, isFinished, @@ -111,7 +110,6 @@ function _destroy(self, err, cb) { if (result != null) { const then = result.then; if (typeof then === 'function') { - emitThenableDeprecationWarning(); then.call( result, function() { @@ -291,7 +289,6 @@ function constructNT(stream) { if (result != null) { const then = result.then; if (typeof then === 'function') { - emitThenableDeprecationWarning(); then.call( result, function() { diff --git a/lib/internal/streams/readable.js b/lib/internal/streams/readable.js index 29a694db2e763d..d0125386c8ae8e 100644 --- a/lib/internal/streams/readable.js +++ b/lib/internal/streams/readable.js @@ -73,7 +73,6 @@ const kPaused = Symbol('kPaused'); const { StringDecoder } = require('string_decoder'); const from = require('internal/streams/from'); -const { emitThenableDeprecationWarning } = require('internal/streams/utils'); ObjectSetPrototypeOf(Readable.prototype, Stream.prototype); ObjectSetPrototypeOf(Readable, Stream); @@ -498,7 +497,6 @@ Readable.prototype.read = function(n) { if (result != null) { const then = result.then; if (typeof then === 'function') { - emitThenableDeprecationWarning(); then.call( result, nop, diff --git a/lib/internal/streams/transform.js b/lib/internal/streams/transform.js index 58aa37f26aa1c2..26e0b07c2956c8 100644 --- a/lib/internal/streams/transform.js +++ b/lib/internal/streams/transform.js @@ -73,7 +73,6 @@ const { ERR_METHOD_NOT_IMPLEMENTED } = require('internal/errors').codes; const Duplex = require('internal/streams/duplex'); -const { emitThenableDeprecationWarning } = require('internal/streams/utils'); ObjectSetPrototypeOf(Transform.prototype, Duplex.prototype); ObjectSetPrototypeOf(Transform, Duplex); @@ -133,7 +132,6 @@ function final(cb) { try { const then = result.then; if (typeof then === 'function') { - emitThenableDeprecationWarning(); then.call( result, (data) => { @@ -209,7 +207,6 @@ Transform.prototype._write = function(chunk, encoding, callback) { try { const then = result.then; if (typeof then === 'function') { - emitThenableDeprecationWarning(); then.call( result, (val) => { diff --git a/lib/internal/streams/utils.js b/lib/internal/streams/utils.js index 1c7570f193a08a..854daae9de41b8 100644 --- a/lib/internal/streams/utils.js +++ b/lib/internal/streams/utils.js @@ -241,20 +241,7 @@ function isDisturbed(stream) { )); } -let thenableDeprecationWarningEmitted = false; -function emitThenableDeprecationWarning() { - if (!thenableDeprecationWarningEmitted) { - process.emitWarning( - 'Returning a thenable is deprecated, use callbacks instead.', - 'DeprecationWarning', - 'DEP0157' - ); - thenableDeprecationWarningEmitted = true; - } -} - module.exports = { - emitThenableDeprecationWarning, kDestroyed, isDisturbed, kIsDisturbed, diff --git a/lib/internal/streams/writable.js b/lib/internal/streams/writable.js index 3186bb47176c3a..7c082c075642eb 100644 --- a/lib/internal/streams/writable.js +++ b/lib/internal/streams/writable.js @@ -44,7 +44,6 @@ const EE = require('events'); const Stream = require('internal/streams/legacy').Stream; const { Buffer } = require('buffer'); const destroyImpl = require('internal/streams/destroy'); -const { emitThenableDeprecationWarning } = require('internal/streams/utils'); const { addAbortSignal, @@ -697,7 +696,6 @@ function callFinal(stream, state) { if (result != null) { const then = result.then; if (typeof then === 'function') { - emitThenableDeprecationWarning(); then.call( result, function() { diff --git a/test/parallel/test-stream-construct-async-error.js b/test/parallel/test-stream-construct-async-error.js index ffa001ec660b55..ea2d8740e29c94 100644 --- a/test/parallel/test-stream-construct-async-error.js +++ b/test/parallel/test-stream-construct-async-error.js @@ -9,12 +9,6 @@ const { const { setTimeout } = require('timers/promises'); const assert = require('assert'); -common.expectWarning( - 'DeprecationWarning', - 'Returning a thenable is deprecated, use callbacks instead.', - 'DEP0157' -); - { class Foo extends Duplex { async _destroy(err, cb) { From 8750b92565ea6188dc83a65a909130571fd5ad7a Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 22 Nov 2021 08:48:57 +0100 Subject: [PATCH 3/6] Update doc/api/deprecations.md Co-authored-by: Matteo Collina --- doc/api/deprecations.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index f3b571f5baa3a9..f9dfeaedecf645 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -3031,7 +3031,9 @@ changes: Type: Documentation-only An undocumented feature of Node.js streams was to support thenables in -implementation methods. This is now deprecated, instead use callbacks. +implementation methods. This feature was causing stream instability when using +async functions as stream methods. This is now deprecated, instead use +callbacks. [Legacy URL API]: url.md#legacy-url-api [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf From e847e1b0707f156de40d7aa7c9a70c1db104f731 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Mon, 22 Nov 2021 13:06:25 +0100 Subject: [PATCH 4/6] squash! Update doc/api/deprecations.md Co-authored-by: Robert Nagy --- doc/api/deprecations.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index f9dfeaedecf645..768ffa0d97a103 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -3035,6 +3035,18 @@ implementation methods. This feature was causing stream instability when using async functions as stream methods. This is now deprecated, instead use callbacks. +This feature caused users to encounter unexpected problems where the user +implements the function in callback style but uses e.g. an async method which +would cause an error since mixing promise and callback semantics is not valid. + +```js +const w = new Writable({ + async final (callback) { + await someOp(); + callback(); // Not valid + } +}); +``` [Legacy URL API]: url.md#legacy-url-api [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf [RFC 6066]: https://tools.ietf.org/html/rfc6066#section-3 From 002896ab99251abf960bd9b38b009b0bce29559b Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Mon, 22 Nov 2021 14:23:13 +0100 Subject: [PATCH 5/6] squash! Update doc/api/deprecations.md --- doc/api/deprecations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 768ffa0d97a103..501471ffd5e19f 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -3043,7 +3043,7 @@ would cause an error since mixing promise and callback semantics is not valid. const w = new Writable({ async final (callback) { await someOp(); - callback(); // Not valid + callback(); } }); ``` From fa5281d0405c4b5277fbe2e96f6fcf594320707e Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 23 Nov 2021 01:19:02 +0100 Subject: [PATCH 6/6] fix linter and improve wording --- doc/api/deprecations.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 501471ffd5e19f..05bb12b65a2b15 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -3031,9 +3031,8 @@ changes: Type: Documentation-only An undocumented feature of Node.js streams was to support thenables in -implementation methods. This feature was causing stream instability when using -async functions as stream methods. This is now deprecated, instead use -callbacks. +implementation methods. This is now deprecated, use callbacks instead and avoid +use of async function for streams implementation methods. This feature caused users to encounter unexpected problems where the user implements the function in callback style but uses e.g. an async method which @@ -3041,12 +3040,13 @@ would cause an error since mixing promise and callback semantics is not valid. ```js const w = new Writable({ - async final (callback) { + async final(callback) { await someOp(); callback(); } }); ``` + [Legacy URL API]: url.md#legacy-url-api [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf [RFC 6066]: https://tools.ietf.org/html/rfc6066#section-3