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

Handle symbols installed on Promise by Node's async_hooks #1115

Merged
merged 12 commits into from
Mar 21, 2022

Conversation

mhofman
Copy link
Contributor

@mhofman mhofman commented Mar 15, 2022

Closes #1105
Layers on top of #1114

Node 14.18 and 16.61 introduced a change (nodejs/node#39135) to async_hooks that enabled the fast-path for promise hooks even in the presence of a destroy hook. The fast-path logic removes the wrapper object which was previously used to hold the async hooks metadata for each promise instance, such as async ids and a property bag related to the liveness of the promise instance, causing these to be attached directly to the promise instance as own symbols.

When a debugger enables async stack traces (Debugger.setAsyncCallStackDepth({maxDepth}) with maxDepth > 0), node's inspector support internally enables async_hooks with a destroy hook 2 3 4, causing all promises to gain these symbols.

The init promise hooks is the main source of symbol attachments. It runs synchronously right after the constructor returns and before the caller gets the reference. While that would normally cause all promise instances to gain these symbols as own property before the program gets a chance to freeze them, other promise hooks enforce the presence of async ids on existing promise objects. If async_hooks is enabled after a promise has already been created and frozen, for example by attaching the debugger later, it will fail to attach these symbols, throwing a TypeError synchronously.

Until node removes these symbols as own property, or handles assignment failure (see nodejs/node#42229), we workaround the issue by installing accessors for these symbols on the Promise.prototype, which defines them as an own property (non-enumerable to avoid polluting the console output), and falls-back to a WeakMap for non-extensible promise instances.

The accessors handle both async ids, and the destroyed symbol. While the latter is not strictly necessary as it's always installed during init and never late, it's more consistent to handle it similarly to async ids, and has the added benefit of removing enumerable pollution.

The async_hooks logic is added to @endo/init because of the dependency on a node import. lockdown is updated to whitelist these symbols on the promise prototype. It would be better to have lockdown remove these by default and require a taming option to leave them in place, but I didn't find a way to thread that into the whitelist logic to make it conditional.

Footnotes

  1. https://github.com/nodejs/node/issues/42229#issuecomment-1062145546

  2. https://github.com/nodejs/node/blob/d4d0b09122d91478881ee7fd36eb3c9f78f8452d/src/inspector_agent.cc#L411-L424

  3. https://github.com/nodejs/node/blob/d4d0b09122d91478881ee7fd36eb3c9f78f8452d/lib/internal/bootstrap/pre_execution.js#L303-L316

  4. https://github.com/nodejs/node/blob/master/lib/internal/inspector_async_hook.js

@mhofman mhofman linked an issue Mar 15, 2022 that may be closed by this pull request
@mhofman mhofman force-pushed the mhofman/handle-async-hooks-promise branch from 958be31 to 4dbf59d Compare March 15, 2022 19:14
};

const setAsyncIdFallback = (promise, symbol, value) => {
const state = getAsyncHookFallbackState(promise, true);
Copy link
Member

Choose a reason for hiding this comment

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

(nitpick) 3 function signatures here have boolean arguments, each meaning different things. using an options object where the boolean is named in the caller as well would limit the lookups one has to make while reading the body of the current function to remember what the boolean did.

bootstrapData.length = 0;
const { length } = promisesData;
if (length > 1) {
// console.warn('Found multiple potential candidates');
Copy link
Member

Choose a reason for hiding this comment

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

Are you aware of a situation when this would happen? AFAIR it shouldn't be possible.

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 did see it happen at one point when I was exploring, and then couldn't reproduce. not sure why exactly, but I kept the check around.

Copy link
Member

@naugtur naugtur Mar 15, 2022

Choose a reason for hiding this comment

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

It would mean the init hook was called twice. I'd consider that a bug in Node.

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 think it created multiple promise instances for some reason.

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

LGTM modulo some nits

packages/init/node-async_hooks.js Outdated Show resolved Hide resolved
packages/init/node-async_hooks.js Outdated Show resolved Hide resolved
packages/init/test/test-async_hooks.js Outdated Show resolved Hide resolved
packages/init/test/test-async_hooks.js Outdated Show resolved Hide resolved
packages/init/test/test-async_hooks.js Outdated Show resolved Hide resolved
packages/ses/src/whitelist.js Outdated Show resolved Hide resolved
Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

I am uncomfortable making these weird symbols into apparent well-known symbols by adding them as statics to the Symbol constructor. Let's discuss before we go forward with this.

if (!wellKnownName) {
throw new Error('Unknown symbol');
} else if (!Symbol[wellKnownName]) {
Symbol[wellKnownName] = symbol;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am uncomfortable making these weird symbols into apparent well-known symbols by adding them as statics to the Symbol constructor. Let's discuss before we go forward with this.

@mhofman mhofman force-pushed the mhofman/fix-well-known-symbol-whitelist branch 2 times, most recently from 9d2b638 to 6f6fffb Compare March 16, 2022 01:59
@mhofman mhofman force-pushed the mhofman/handle-async-hooks-promise branch from 7764ea1 to d62e8cf Compare March 16, 2022 02:40
@mhofman mhofman requested a review from erights March 16, 2022 02:41
@mhofman
Copy link
Contributor Author

mhofman commented Mar 16, 2022

@erights I switched away from making the async hooks symbols registered and using the new ability to name unique symbols in the whitelist. PTAL

@mhofman mhofman force-pushed the mhofman/handle-async-hooks-promise branch from d62e8cf to 02d90b9 Compare March 16, 2022 03:09
@mhofman
Copy link
Contributor Author

mhofman commented Mar 16, 2022

It was brought up that @endo/init may be used in non-Node environments, in which case we'll need either different entrypoints for each environment, or a shim for async_hooks, with the following shape:

export class AsyncResource {};
export const createHook = () => ({ enable() {}, disable() {} });

@mhofman mhofman force-pushed the mhofman/fix-well-known-symbol-whitelist branch 2 times, most recently from 899a957 to 5139dad Compare March 16, 2022 23:46
Base automatically changed from mhofman/fix-well-known-symbol-whitelist to master March 16, 2022 23:55
@mhofman mhofman force-pushed the mhofman/handle-async-hooks-promise branch from 02d90b9 to 65c52e3 Compare March 17, 2022 00:12
@mhofman
Copy link
Contributor Author

mhofman commented Mar 17, 2022

I've updated the PR to only include the async_hooks patch on node based on conditional exports, and included a test, but I'm getting an apparently unrelated failure:

Cannot read property 'buildError' of undefined

attn @kriskowal

@kriskowal
Copy link
Member

@mhofman I’ve added a commit that should unblock the build with makeArchive.

@@ -35,11 +35,11 @@
"types": "./index.d.ts",
"exports": {
".": {
"import": "./dist/ses.umd.js",
Copy link
Contributor Author

@mhofman mhofman Mar 17, 2022

Choose a reason for hiding this comment

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

Should there be a "browser" fallback keeping UMD for the browser use case (after import and require) ?

@mhofman
Copy link
Contributor Author

mhofman commented Mar 17, 2022

@mhofman I’ve added a commit that should unblock the build with makeArchive.

Well except I've taken a reliance on self references. I think I'll move the async hooks logic in a separate taming package that endo/init takes a dependency on for now?

@mhofman mhofman force-pushed the mhofman/handle-async-hooks-promise branch from 1b224d9 to 4b73064 Compare March 17, 2022 18:55
@mhofman
Copy link
Contributor Author

mhofman commented Mar 17, 2022

@erights PTAL

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

I just noticed that all the normal source files of this init package are at top level in the package, rather than nested in a src directory. Please move the normal source files into a src directory.

Other than that, my review is still in progress, but looking good to me so far.

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

Review done. Everything I understood LGTM except the directory structure. I did look at all the node/async_hook manipulation code, and it seemed fine to the extent I understood it. Mostly I was amazed at how many cases you had to wrestle with!

@mhofman
Copy link
Contributor Author

mhofman commented Mar 19, 2022

I just noticed that all the normal source files of this init package are at top level in the package, rather than nested in a src directory. Please move the normal source files into a src directory.

I believe the files were in the root because they are all entrypoints. While package.json "exports" now allows us to remap this anywhere, I think it's best to keep the regular entrypoints in place for tools which don't support remapping. I have however moved all other/new files into src. @kriskowal does the structure look good by you?

@kriskowal
Copy link
Member

kriskowal commented Mar 21, 2022

I just noticed that all the normal source files of this init package are at top level in the package, rather than nested in a src directory. Please move the normal source files into a src directory.

I believe the files were in the root because they are all entrypoints. While package.json "exports" now allows us to remap this anywhere, I think it's best to keep the regular entrypoints in place for tools which don't support remapping. I have however moved all other/new files into src. @kriskowal does the structure look good by you?

Yes, @michaelfig and I have agreed that, due to limitations of interoperability between Node.js versions that recognize exports and other systems that do not yet, we will place all entry-point modules (modules that are public) at the top level, and any other module under src as an indication that it is private.

For the same reason, we determined that the exports needs to just alias these entry-modules in-place, including the .js extension.

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

I approve in terms of general structure and willingness to maintain. I don’t begin to grasp the implications of async_hooks, but the effect of this change seems good to me.

packages/init/node-async_hooks.js Outdated Show resolved Hide resolved
packages/init/pre-remoting.js Show resolved Hide resolved
packages/init/package.json Show resolved Hide resolved
@mhofman mhofman merged commit 06827b9 into master Mar 21, 2022
@mhofman mhofman deleted the mhofman/handle-async-hooks-promise branch March 21, 2022 21:24
@mhofman mhofman mentioned this pull request Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node bug causes failures within the vscode Debug Console
5 participants