lib: implement SafeThenable #36326
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
|
There was a problem hiding this comment. there are still a number of performance concerns around using private fields (unfortunately). These should likely be non-exported Symbols instead. |
||
|
|
||
| constructor(thenable, makeUnsafeCalls = false) { | ||
|
There was a problem hiding this comment. A comment here describing the intent of Also, I generally prefer avoiding boolean arguments in favor of named flags or an options object. That is, either |
||
| this.#target = thenable; | ||
| this.#is_promise = isPromise(thenable); | ||
| this.#makeUnsafeCalls = makeUnsafeCalls; | ||
| } | ||
|
|
||
| get #then() { | ||
|
There was a problem hiding this comment. While I'm fine with using this syntax in general, we should benchmark this extensively before landing. Last I checked, v8 was not yet optimizing private accessors that well and since we're using this in EventEmitter (one of the most performance sensitive bits of code we have in core) it's good to be careful here. |
||
| // Handle Promises/A+ spec, `then` could be a getter | ||
|
There was a problem hiding this comment. Why do we care about the Promises/A+ spec in the first place? The only spec that we care about is the JavaScript spec - if we got a native promise this should never happen right? The "double getteR" thing is an issue (thanks jQuery!) with assimilation mostly. There was a problem hiding this comment. Yes, it is not intended to be used with genuine |
||
| // 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) { | ||
|
There was a problem hiding this comment. This is terrifying, even if we ignore things like "the returned thenable is missing finally" or "if you do `.constructor you don't get all the promise statics. This is still terrifying ^^ I am -0.5, if others feel strongly then maybe There was a problem hiding this comment. This is intended to be used with objects for which only const then = obj?.then;
if(typeof then === function) {
FunctionPrototypeCall(then, obj, onSuccess, onError);
}I think |
||
| 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; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should run benchmarks on this before landing