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.

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

lib: make safe primordials Promise methods #38650

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ const {
MathMin,
NumberIsSafeInteger,
Promise,
PromisePrototypeFinally,
PromisePrototypeThen,
PromiseResolve,
PromiseReject,
SafeArrayIterator,
SafePromisePrototypeFinally,
Symbol,
Uint8Array,
} = primordials;
Expand Down Expand Up @@ -188,12 +188,12 @@ class FileHandle extends EventEmitterMixin(JSTransferable) {
this[kRefs]--;
if (this[kRefs] === 0) {
this[kFd] = -1;
this[kClosePromise] = PromisePrototypeFinally(
this[kClosePromise] = SafePromisePrototypeFinally(
this[kHandle].close(),
() => { this[kClosePromise] = undefined; }
);
} else {
this[kClosePromise] = PromisePrototypeFinally(
this[kClosePromise] = SafePromisePrototypeFinally(
new Promise((resolve, reject) => {
this[kCloseResolve] = resolve;
this[kCloseReject] = reject;
Expand Down
9 changes: 6 additions & 3 deletions lib/internal/modules/run_main.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
'use strict';

const {
PromisePrototypeFinally,
StringPrototypeEndsWith,
} = primordials;
const CJSLoader = require('internal/modules/cjs/loader');
Expand Down Expand Up @@ -51,7 +50,7 @@ function runMainESM(mainPath) {
}));
}

function handleMainPromise(promise) {
async function handleMainPromise(promise) {
// Handle a Promise from running code that potentially does Top-Level Await.
// In that case, it makes sense to set the exit code to a specific non-zero
// value if the main code never finishes running.
Expand All @@ -60,7 +59,11 @@ function handleMainPromise(promise) {
process.exitCode = 13;
}
process.on('exit', handler);
return PromisePrototypeFinally(promise, () => process.off('exit', handler));
try {
return await promise;
} finally {
process.off('exit', handler);
}
}

// For backwards compatibility, we have to run a bunch of
Expand Down
31 changes: 31 additions & 0 deletions lib/internal/per_context/primordials.js
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,34 @@ 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);

/**
* Attaches a callback that is invoked when the Promise is settled (fulfilled or
* rejected). The resolved value cannot be modified from the callback.
* Prefer using async functions when possible.
* @param {Promise<any>} thisPromise
* @param {() => void) | undefined | null} onFinally The callback to execute
* when the Promise is settled (fulfilled or rejected).
* @returns A Promise for the completion of the callback.
*/
primordials.SafePromisePrototypeFinally = (thisPromise, onFinally) =>
// Wrapping on a new Promise is necessary to not expose the SafePromise
// prototype to user-land.
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

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);
6 changes: 3 additions & 3 deletions lib/timers/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
const {
FunctionPrototypeBind,
Promise,
PromisePrototypeFinally,
PromiseReject,
SafePromisePrototypeFinally,
} = primordials;

const {
Expand Down Expand Up @@ -71,7 +71,7 @@ function setTimeout(after, value, options = {}) {
}
});
return oncancel !== undefined ?
PromisePrototypeFinally(
SafePromisePrototypeFinally(
ret,
() => signal.removeEventListener('abort', oncancel)) : ret;
}
Expand Down Expand Up @@ -115,7 +115,7 @@ function setImmediate(value, options = {}) {
}
});
return oncancel !== undefined ?
PromisePrototypeFinally(
SafePromisePrototypeFinally(
ret,
() => signal.removeEventListener('abort', oncancel)) : ret;
}
Expand Down
1 change: 1 addition & 0 deletions test/message/esm_display_syntax_error_import.out
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ SyntaxError: The requested module '../fixtures/es-module-loaders/module-named-ex
at async ModuleJob.run (node:internal/modules/esm/module_job:*:*)
at async Loader.import (node:internal/modules/esm/loader:*:*)
at async Object.loadESM (node:internal/process/esm_loader:*:*)
at async handleMainPromise (node:internal/modules/run_main:*:*)
1 change: 1 addition & 0 deletions test/message/esm_display_syntax_error_import_module.out
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ SyntaxError: The requested module './module-named-exports.mjs' does not provide
at async ModuleJob.run (node:internal/modules/esm/module_job:*:*)
at async Loader.import (node:internal/modules/esm/loader:*:*)
at async Object.loadESM (node:internal/process/esm_loader:*:*)
at async handleMainPromise (node:internal/modules/run_main:*:*)
38 changes: 38 additions & 0 deletions test/parallel/test-primordials-promise.js
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,
PromisePrototypeThen,
SafePromisePrototypeFinally,
} = require('internal/test/binding').primordials;

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

assertIsPromise(PromisePrototypeCatch(Promise.reject(), common.mustCall()));
assertIsPromise(PromisePrototypeThen(test(), common.mustCall()));
assertIsPromise(SafePromisePrototypeFinally(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);
}