From 6844ebf88ce57f645c3744550013e68ba03b7aa3 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Mon, 30 Nov 2020 00:41:48 +0100 Subject: [PATCH] lib: implement SafeThenable Adds an internal API to handle promises/A+ spec in a consistent way. It uses the built-in Promise.prototype methods when an actual `Promise` instance is given, or lookup and cache the `then` method in the prototype chain otherwise. --- lib/events.js | 19 ++--- lib/internal/event_target.js | 16 ++-- lib/internal/per_context/safethenable.js | 71 +++++++++++++++++ lib/internal/streams/destroy.js | 19 +++-- lib/internal/streams/pipeline.js | 14 ++-- lib/internal/streams/transform.js | 19 +++-- lib/internal/streams/writable.js | 10 +-- lib/util.js | 5 +- node.gyp | 1 + test/parallel/test-safethenable.js | 97 ++++++++++++++++++++++++ 10 files changed, 219 insertions(+), 52 deletions(-) create mode 100644 lib/internal/per_context/safethenable.js create mode 100644 test/parallel/test-safethenable.js diff --git a/lib/events.js b/lib/events.js index dc08042578bc7a..2cac17a1c95ec1 100644 --- a/lib/events.js +++ b/lib/events.js @@ -65,6 +65,7 @@ const { const { validateAbortSignal } = require('internal/validators'); +let SafeThenable; const kCapture = Symbol('kCapture'); const kErrorMonitor = Symbol('events.errorMonitor'); @@ -218,18 +219,14 @@ function addCatch(that, promise, type, args) { return; } - // Handle Promises/A+ spec, then could be a getter - // that throws on second use. try { - const then = promise.then; - - if (typeof then === 'function') { - then.call(promise, undefined, function(err) { - // The callback is called with nextTick to avoid a follow-up - // rejection from this promise. - process.nextTick(emitUnhandledRejectionOrErr, that, err, type, args); - }); - } + if (!SafeThenable) + SafeThenable = require('internal/per_context/safethenable'); + new SafeThenable(promise).catch(function(err) { + // The callback is called with nextTick to avoid a follow-up + // rejection from this promise. + process.nextTick(emitUnhandledRejectionOrErr, that, err, type, args); + }); } catch (err) { that.emit('error', err); } diff --git a/lib/internal/event_target.js b/lib/internal/event_target.js index 1f6ae1c17c8317..3d10d431d618c3 100644 --- a/lib/internal/event_target.js +++ b/lib/internal/event_target.js @@ -17,6 +17,7 @@ const { SymbolToStringTag, SafeWeakSet, } = primordials; +let SafeThenable; const { codes: { @@ -564,14 +565,13 @@ function isEventTarget(obj) { } function addCatch(that, promise, event) { - const then = promise.then; - if (typeof then === 'function') { - then.call(promise, undefined, function(err) { - // The callback is called with nextTick to avoid a follow-up - // rejection from this promise. - process.nextTick(emitUnhandledRejectionOrErr, that, err, event); - }); - } + if (!SafeThenable) + SafeThenable = require('internal/per_context/safethenable'); + new SafeThenable(promise).catch(function(err) { + // The callback is called with nextTick to avoid a follow-up + // rejection from this promise. + process.nextTick(emitUnhandledRejectionOrErr, that, err, event); + }); } function emitUnhandledRejectionOrErr(that, err, event) { diff --git a/lib/internal/per_context/safethenable.js b/lib/internal/per_context/safethenable.js new file mode 100644 index 00000000000000..40b9f8a1ebb026 --- /dev/null +++ b/lib/internal/per_context/safethenable.js @@ -0,0 +1,71 @@ +'use strict'; + +const { + PromisePrototypeCatch, + PromisePrototypeThen, + ReflectApply, + SafeWeakMap, +} = primordials; + +const { isPromise } = require('internal/util/types'); + +const cache = new SafeWeakMap(); + +// `SafeThenable` should be used when dealing with user provided `Promise`-like +// instances. It provides two methods `.then` and `.catch`. +// Wrapping uses of `SafeThenable` in `try`/`catch` may be useful if the +// accessing `.then` property of the provided object throws. +class SafeThenable { + #is_promise; + #makeUnsafeCalls; + #target; + #cachedThen; + #hasAlreadyAccessedThen; + + constructor(thenable, makeUnsafeCalls = false) { + this.#target = thenable; + this.#is_promise = isPromise(thenable); + this.#makeUnsafeCalls = makeUnsafeCalls; + } + + get #then() { + // Handle Promises/A+ spec, `then` could be a getter + // that throws on second access. + if (this.#hasAlreadyAccessedThen === undefined) { + this.#hasAlreadyAccessedThen = cache.has(this.#target); + if (this.#hasAlreadyAccessedThen) { + this.#cachedThen = cache.get(this.#target); + } + } + if (!this.#hasAlreadyAccessedThen) { + this.#cachedThen = this.#target?.then; + this.#hasAlreadyAccessedThen = true; + if (typeof this.#target === 'object' && this.#target !== null) { + cache.set(this.#target, this.#cachedThen); + } + } + + return this.#cachedThen; + } + + get isThenable() { + return this.#is_promise || typeof this.#then === 'function'; + } + + catch(onError) { + return this.#is_promise ? + PromisePrototypeCatch(this.#target, onError) : + this.then(undefined, onError); + } + + then(...args) { + if (this.#is_promise) { + return PromisePrototypeThen(this.#target, ...args); + } else if (this.#makeUnsafeCalls || this.isThenable) { + return ReflectApply(this.#then, this.#target, args); + } + } + +} + +module.exports = SafeThenable; diff --git a/lib/internal/streams/destroy.js b/lib/internal/streams/destroy.js index 7477ec97379ff9..29ce8dc0a9111d 100644 --- a/lib/internal/streams/destroy.js +++ b/lib/internal/streams/destroy.js @@ -4,6 +4,7 @@ const { ERR_MULTIPLE_CALLBACK } = require('internal/errors').codes; const { Symbol } = primordials; +let SafeThenable; // lazy loaded const kDestroy = Symbol('kDestroy'); const kConstruct = Symbol('kConstruct'); @@ -91,10 +92,10 @@ function _destroy(self, err, cb) { }); if (result !== undefined && result !== null) { try { - const then = result.then; - if (typeof then === 'function') { - then.call( - result, + if (!SafeThenable) + SafeThenable = require('internal/per_context/safethenable'); + new SafeThenable(result) + .then( function() { if (called) return; @@ -142,7 +143,6 @@ function _destroy(self, err, cb) { process.nextTick(emitErrorCloseNT, self, err); }); - } } catch (err) { process.nextTick(emitErrorNT, self, err); } @@ -309,10 +309,10 @@ function constructNT(stream) { }); if (result !== undefined && result !== null) { try { - const then = result.then; - if (typeof then === 'function') { - then.call( - result, + if (!SafeThenable) + SafeThenable = require('internal/per_context/safethenable'); + new SafeThenable(result) + .then( function() { // If the callback was invoked, do nothing further. if (called) @@ -343,7 +343,6 @@ function constructNT(stream) { process.nextTick(errorOrDestroy, stream, err); } }); - } } catch (err) { process.nextTick(emitErrorNT, stream, err); } diff --git a/lib/internal/streams/pipeline.js b/lib/internal/streams/pipeline.js index b23f05ca1f206a..82e54a6e2769e1 100644 --- a/lib/internal/streams/pipeline.js +++ b/lib/internal/streams/pipeline.js @@ -5,10 +5,10 @@ const { ArrayIsArray, - ReflectApply, SymbolAsyncIterator, SymbolIterator, } = primordials; +let SafeThenable; let eos; @@ -224,18 +224,18 @@ function pipeline(...streams) { objectMode: true }); - // Handle Promises/A+ spec, `then` could be a getter that throws on - // second use. - const then = ret?.then; - if (typeof then === 'function') { - ReflectApply(then, ret, [ + if (!SafeThenable) + SafeThenable = require('internal/per_context/safethenable'); + const maybeThenable = new SafeThenable(ret); + if (maybeThenable.isThenable) { + maybeThenable.then( (val) => { value = val; pt.end(val); }, (err) => { pt.destroy(err); } - ]); + ); } else if (isIterable(ret, true)) { finishCount++; pump(ret, pt, finish); diff --git a/lib/internal/streams/transform.js b/lib/internal/streams/transform.js index 26e0b07c2956c8..1bc73f5cd30c6f 100644 --- a/lib/internal/streams/transform.js +++ b/lib/internal/streams/transform.js @@ -67,6 +67,7 @@ const { ObjectSetPrototypeOf, Symbol } = primordials; +let SafeThenable; module.exports = Transform; const { @@ -130,10 +131,10 @@ function final(cb) { }); if (result !== undefined && result !== null) { try { - const then = result.then; - if (typeof then === 'function') { - then.call( - result, + if (!SafeThenable) + SafeThenable = require('internal/per_context/safethenable'); + new SafeThenable(result) + .then( (data) => { if (called) return; @@ -150,7 +151,6 @@ function final(cb) { process.nextTick(() => this.destroy(err)); } }); - } } catch (err) { process.nextTick(() => this.destroy(err)); } @@ -205,10 +205,10 @@ Transform.prototype._write = function(chunk, encoding, callback) { }); if (result !== undefined && result != null) { try { - const then = result.then; - if (typeof then === 'function') { - then.call( - result, + if (!SafeThenable) + SafeThenable = require('internal/per_context/safethenable'); + new SafeThenable(result) + .then( (val) => { if (called) return; @@ -230,7 +230,6 @@ Transform.prototype._write = function(chunk, encoding, callback) { (err) => { process.nextTick(callback, err); }); - } } catch (err) { process.nextTick(callback, err); } diff --git a/lib/internal/streams/writable.js b/lib/internal/streams/writable.js index 104a3c43452931..aeab6bde012fbe 100644 --- a/lib/internal/streams/writable.js +++ b/lib/internal/streams/writable.js @@ -33,6 +33,7 @@ const { Symbol, SymbolHasInstance, } = primordials; +let SafeThenable; module.exports = Writable; Writable.WritableState = WritableState; @@ -667,10 +668,10 @@ function callFinal(stream, state) { }); if (result !== undefined && result !== null) { try { - const then = result.then; - if (typeof then === 'function') { - then.call( - result, + if (!SafeThenable) + SafeThenable = require('internal/per_context/safethenable'); + new SafeThenable(result) + .then( function() { if (state.prefinished) return; @@ -685,7 +686,6 @@ function callFinal(stream, state) { } process.nextTick(errorOrDestroy, stream, err, state.sync); }); - } } catch (err) { process.nextTick(errorOrDestroy, stream, err, state.sync); } diff --git a/lib/util.js b/lib/util.js index 2d8687ac9c569a..4a83a5e9593de0 100644 --- a/lib/util.js +++ b/lib/util.js @@ -34,6 +34,7 @@ const { ObjectSetPrototypeOf, ReflectApply, } = primordials; +let SafeThenable; const { codes: { @@ -208,7 +209,9 @@ function callbackify(original) { const cb = (...args) => { ReflectApply(maybeCb, this, args); }; // In true node style we process the callback on `nextTick` with all the // implications (stack, `uncaughtException`, `async_hooks`) - ReflectApply(original, this, args) + if (!SafeThenable) + SafeThenable = require('internal/per_context/safethenable'); + new SafeThenable(ReflectApply(original, this, args), true) .then((ret) => process.nextTick(cb, null, ret), (rej) => process.nextTick(callbackifyOnRejected, rej, cb)); } diff --git a/node.gyp b/node.gyp index 643c8a43c6ff9c..37f0cfbbbb6b16 100644 --- a/node.gyp +++ b/node.gyp @@ -42,6 +42,7 @@ 'lib/internal/per_context/primordials.js', 'lib/internal/per_context/domexception.js', 'lib/internal/per_context/messageport.js', + 'lib/internal/per_context/safethenable.js', 'lib/async_hooks.js', 'lib/assert.js', 'lib/assert/strict.js', diff --git a/test/parallel/test-safethenable.js b/test/parallel/test-safethenable.js new file mode 100644 index 00000000000000..1f5f96dcd2a4d9 --- /dev/null +++ b/test/parallel/test-safethenable.js @@ -0,0 +1,97 @@ +'use strict'; + +// Flags: --expose-internals --no-warnings + +const common = require('../common'); + +const SafeThenable = require('internal/per_context/safethenable'); +const assert = require('assert'); + +{ + new SafeThenable(Promise.resolve(1)).then( + common.mustCall((t) => assert.strictEqual(t, 1)) + ).then(common.mustCall()); +} +{ + new SafeThenable(Promise.reject(1)).catch( + common.mustCall((t) => assert.strictEqual(t, 1)) + ); +} +{ + new SafeThenable(Promise.reject(1)).then( + common.mustNotCall(), + common.mustCall((t) => assert.strictEqual(t, 1)) + ); +} +{ + // Store original value to restore it later. + const originalCatch = Promise.prototype.catch; + + Promise.prototype.catch = common.mustNotCall(function() { + // User mutations of Promise prototype should not interfere. + return this; + }); + new SafeThenable(Promise.reject(1)).catch( + common.mustCall((t) => assert.strictEqual(t, 1)) + ).then(common.mustCall()); + + // Restore the original value. + Promise.prototype.catch = originalCatch; +} +{ + assert.rejects( + new SafeThenable(Promise.reject(new Error('Test error'))).then( + common.mustNotCall() + ), + /Test error/ + ).then(common.mustCall()); +} +{ + let wasAccessed = false; + const thenable = { + get then() { + if (wasAccessed) throw new Error('Cannot access this method twice'); + wasAccessed = true; + return function then(onSuccess, onError) { + try { + onSuccess(); + } catch (e) { + onError(e); + } + return this; + }; + }, + }; + + assert(new SafeThenable(thenable).isThenable); + + assert.throws( + () => typeof thenable.then === 'function', + /Cannot access this method twice/ + ); + + // SafeThenable allows to call then method even from new instances + new SafeThenable(thenable).then(common.mustCall()); + new SafeThenable(thenable).catch(common.mustCall()); +} +{ + [true, 1, 4n, 'string', Symbol(), function() {}, class {}, new class {}(), + null, undefined, {}].forEach((val) => { + assert(!new SafeThenable(val).isThenable); + new SafeThenable(val).then(common.mustNotCall()); + assert.throws(() => new SafeThenable(val, true).then(common.mustNotCall()), + /not a function/); + }); +} +{ + Number.prototype.then = function then(onSuccess, onError) { + try { + onSuccess(); + } catch (e) { + onError(e); + } + }; + assert(new SafeThenable(1).isThenable); + new SafeThenable(1).then(common.mustCall()); + delete Number.prototype.then; +}