lib: implement SafeThenable #36326

aduh95 · 2020-11-30T14:23:25Z

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.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

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.

aduh95 · 2020-11-30T14:26:52Z

Linter failure should be fixed by #36321.

jasnell · 2020-11-30T15:32:25Z

lib/events.js

+      // The callback is called with nextTick to avoid a follow-up
+      // rejection from this promise.
+      process.nextTick(emitUnhandledRejectionOrErr, that, err, type, args);
+    });


We should run benchmarks on this before landing

jasnell · 2020-11-30T15:35:43Z

lib/internal/per_context/safethenable.js

+  #cachedThen;
+  #hasAlreadyAccessedThen;
+
+  constructor(thenable, makeUnsafeCalls = false) {


A comment here describing the intent of makeUnsafeCalls would be good.

Also, I generally prefer avoiding boolean arguments in favor of named flags or an options object. That is, either new SafeThenable(promise, kMakeUnsafeCalls) where kMakeUnsafeCalls === 1 (allows additional flags to be added later using xor) or new SafeThenable(promise, { unsafeCalls: true }).

jasnell · 2020-11-30T16:03:18Z

lib/internal/per_context/safethenable.js

+    this.#makeUnsafeCalls = makeUnsafeCalls;
+  }
+
+  get #then() {


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.

jasnell · 2020-11-30T16:04:06Z

lib/internal/per_context/safethenable.js

+  #makeUnsafeCalls;
+  #target;
+  #cachedThen;
+  #hasAlreadyAccessedThen;


there are still a number of performance concerns around using private fields (unfortunately). These should likely be non-exported Symbols instead.

jasnell · 2020-11-30T16:05:04Z

lib/internal/streams/destroy.js

-          result,
+      if (!SafeThenable)
+        SafeThenable = require('internal/per_context/safethenable');
+      new SafeThenable(result)


Should wrap this in a lazySafeThenable() function to avoid the code duplication

benjamingr · 2020-11-30T17:13:13Z

lib/internal/per_context/safethenable.js

+      this.then(undefined, onError);
+  }
+
+  then(...args) {


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

This is intended to be used with objects for which only .then method is called. It's just a wrapper for the pattern that shows several times in core:

const then = obj?.then; if(typeof then === function) { FunctionPrototypeCall(then, obj, onSuccess, onError); }

I think .finally is a bit off-topic, we should always prefer PromisePrototypeFinally anyway.

benjamingr · 2020-11-30T17:14:41Z

lib/internal/per_context/safethenable.js

+  }
+
+  get #then() {
+    // Handle Promises/A+ spec, `then` could be a getter


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.

Yes, it is not intended to be used with genuine Promise object, it's just for API that have to deal with user-provided thenable objects (E.G.: util.callbackify).

benjamingr · 2020-11-30T17:15:29Z

lib/internal/event_target.js

-  }
+  if (!SafeThenable)
+    SafeThenable = require('internal/per_context/safethenable');
+  new SafeThenable(promise).catch(function(err) {


What advantage does this have over { await thenable } catch (err) { ...?

This is not awaiting the thenable at all – it attaches a catch handler in case the event handler returns a thenable. I'm pretty sure that's in the spec, although I haven't checked.

benjamingr

Ok sorry, I hate blocking things but I'm starting to feel strongly there are footguns here.

I'd prefer an approach refactoring unguarded thens we are concerned about to awaits with native async await which is always safe anyway.

aduh95 · 2020-12-01T00:14:17Z

there are footguns here

@benjamingr What footguns are thinking about? Do you think we could avoid that by changing the name of the class and its method? new MaybeThenable(target).maybeThen()?

I'd prefer an approach refactoring unguarded thens we are concerned about to awaits with native async await which is always safe anyway

You mean something like:

const createSafeThenable = (target) => new Promise(async (_) => _(await target));

The issue I can think of with this code is that it creates another Promise object, but you're right it's worth the try. I'll run some benchmarks to see if one approach performs better than the other.

benjamingr · 2020-12-01T12:15:09Z

const createSafeThenable = (target) => new Promise(async (_) => _(await target));

That can just be createSafeThenable = PromiseResolve though no?

aduh95 · 2020-12-01T12:58:30Z

That can just be createSafeThenable = PromiseResolve though no?

If the target .then accessor throws (because it was accessed before maybe), it would throw the error synchronously instead of rejecting the promise. That would indeed be more inline how we handle those object, so yes.

The issue with createSafeThenable is that sometimes, we need to know IF an object is a thenable (E.G. https://github.com/nodejs/node/pull/36326/files#diff-95c23fb6674af85f6f23a374851af6cccf58fb4617d73897df5078ba537a862cR230). Wrapping it with createSafeThenable would not be helpful.

@benjamingr could you please express what are the footguns you think the current implementation could create? I'm willing to try to fix those if that's possible.

benjamingr · 2020-12-01T17:49:09Z

If the target .then accessor throws (because it was accessed before maybe), it would throw the error synchronously instead of rejecting the promise. That would indeed be more inline how we handle those object, so yes.

FWIW: I raised this issue in Node (the double get thing from the A+ spec) once as a rhetorical tool to illustrate there are many edge cases in the spec. Since then I keep seeing this as an issue here but it's really not a big deal in real life and I wouldn't worry about it. There was one version of jQuery like... 10 years ago that had a bug and assimilating promises caused issues.

I am not aware of any real thenable implementation with this bug.

The issue with createSafeThenable is that sometimes, we need to know IF an object is a thenable

That's fair, the common pattern is indeed typeof then === 'function' like we currently do. I thin wrapping things in a "promise like compliant wrapper" is kind of confusing.

You can extract that check into a helper if you think it repeats a lot I guess?

function callThenIfPromise(maybeThenable, onFulfilled) {
  const then = maybeThenable?.then; 
  if(typeof then === 'function')
    FunctionPrototypeCall(then, maybeThenable, onFulfilled); // Or PromisePrototypeThen if you prefer
}

Which is much simpler (right?). The code after this PR isn't actually shorter or clearer from what I can tell.

could you please express what are the footguns you think the current implementation could create?

Sure, always happy to talk about promise footguns, none of these are huge but they were pretty quick to spot:

You are probably doing async_hooks incorrectly here? (not sure honestly)
Certain API methods are simply missing (e.g) finally which is fine as long as the SafeThenable is never ever exposed anywhere, I am not sure people will be vigilant and will never return it.
It doesn't work with promise subclasses that override then (there is a boolean trap for that I guess?)
What if then is added on an object later on? Now that we keep state in a SafeThenable the API might tempt users to convert something to a SafeThenable which checks if it has a then in the constructor and not when you invoke it.
The API surface looks like a promise but isn't a promise and can't be used in certain places promises can.

If the goal here is to make code safer I think it's better to call the then of the passed in object - then is all about duck typing so I think calling the passed then and not PromisePrototypeThen makes sense from the API contract perspective).

If you want to make sure Promise.prototype.then is called - then I would Promise.resolve it which would call then internally (why though? even more chance to mess with async_hooks that way).

aduh95 · 2020-12-01T19:46:31Z

Fair points, thanks for the explanation.

lib: implement SafeThenable

6844ebf

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.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 30, 2020

aduh95 added blocked PRs that are blocked by other issues or PRs. dont-land-on-v10.x labels Nov 30, 2020

jasnell reviewed Nov 30, 2020

View reviewed changes

benjamingr reviewed Nov 30, 2020

View reviewed changes

benjamingr requested changes Nov 30, 2020

View reviewed changes

aduh95 closed this Dec 1, 2020

aduh95 deleted the safe-thenable branch December 1, 2020 19:46

aduh95 mentioned this pull request Dec 6, 2020

lib: make safe primordials safe to iterate #36391

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: implement SafeThenable #36326

lib: implement SafeThenable #36326

aduh95 commented Nov 30, 2020

aduh95 commented Nov 30, 2020

jasnell Nov 30, 2020

jasnell Nov 30, 2020

jasnell Nov 30, 2020

jasnell Nov 30, 2020

jasnell Nov 30, 2020

benjamingr Nov 30, 2020

aduh95 Nov 30, 2020

benjamingr Nov 30, 2020

aduh95 Nov 30, 2020

benjamingr Nov 30, 2020

aduh95 Nov 30, 2020

benjamingr left a comment

aduh95 commented Dec 1, 2020

benjamingr commented Dec 1, 2020

aduh95 commented Dec 1, 2020

benjamingr commented Dec 1, 2020

aduh95 commented Dec 1, 2020

lib: implement SafeThenable #36326

lib: implement SafeThenable #36326

Conversation

aduh95 commented Nov 30, 2020

Checklist

aduh95 commented Nov 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

aduh95 commented Dec 1, 2020

benjamingr commented Dec 1, 2020

aduh95 commented Dec 1, 2020

benjamingr commented Dec 1, 2020

aduh95 commented Dec 1, 2020