Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib: make safe primordials Promise methods #38650

Closed
wants to merge 3 commits into from
Closed

lib: make safe primordials Promise methods #38650

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
06e59ff
lib: make primordials Promise methods safe
aduh95 May 12, 2021
41a3924
fixup! lib: make primordials Promise methods safe
aduh95 May 18, 2021
57b14cf
fixup! fixup! lib: make primordials Promise methods safe
aduh95 May 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions lib/internal/per_context/primordials.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,8 @@ const {
Map,
ObjectFreeze,
ObjectSetPrototypeOf,
Promise,
PromisePrototypeThen,
Set,
SymbolIterator,
WeakMap,
Expand Down Expand Up @@ -384,5 +386,23 @@ primordials.SafeWeakRef = makeSafe(
}
);

const SafePromise = makeSafe(
Promise,
class SafePromise extends Promise {
// eslint-disable-next-line no-useless-constructor
constructor(executor) { super(executor); }
}
);

primordials.PromisePrototypeCatch = (thisPromise, onRejected) =>
PromisePrototypeThen(thisPromise, undefined, onRejected);

primordials.PromisePrototypeFinally = (thisPromise, onFinally) =>
new Promise((a, b) =>
new SafePromise((a, b) => PromisePrototypeThen(thisPromise, a, b))
.finally(onFinally)
.then(a, b)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this creates too many promises to be usable in any hot code paths. I would recommend not adding this.

Copy link
Contributor Author

@aduh95 aduh95 May 18, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't promises antithetical to hot code path since they are asynchronous? For info, PromisePrototypeFinally is only used in fs/promises, timers/promises, and run_main currently. Would you prefer if I added a lint rule to discourage its use so it doesn't end up in a hot code path?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this allocates 3 or 4 promises instead of 1.
Using this specific code will only create problems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also be more in favor of discouraging the use of the catch and finally methods in core:

  • catch can be easily replaced with then(undefined, fn)
  • finally may be replaceable with async functions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed used of PromisePrototypeFinally where it was possible, and rename the function to SafePromisePrototypeFinally to highlight the fact it is not a simple bridge to the actual primordial method. I plan to add it to the list of "problematic" primordials to avoid in hot code path in #38635.

);

ObjectSetPrototypeOf(primordials, null);
ObjectFreeze(primordials);
38 changes: 38 additions & 0 deletions test/parallel/test-primordials-promise.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Flags: --expose-internals
'use strict';

const common = require('../common');
const assert = require('assert');

const {
PromisePrototypeCatch,
PromisePrototypeFinally,
PromisePrototypeThen,
} = require('internal/test/binding').primordials;

Promise.prototype.catch = common.mustNotCall();
Promise.prototype.finally = common.mustNotCall();
Promise.prototype.then = common.mustNotCall();

assertIsPromise(PromisePrototypeCatch(test(), common.mustNotCall()));
assertIsPromise(PromisePrototypeFinally(test(), common.mustCall()));
assertIsPromise(PromisePrototypeThen(test(), common.mustCall()));

async function test() {
const catchFn = common.mustCall();
const finallyFn = common.mustCall();

try {
await Promise.reject();
} catch {
catchFn();
} finally {
finallyFn();
}
}

function assertIsPromise(promise) {
// Make sure the returned promise is a genuine %Promise% object and not a
// subclass instance.
assert.strictEqual(Object.getPrototypeOf(promise), Promise.prototype);
}