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

mhofman · 2022-03-15T19:08:27Z

Closes #1105
Layers on top of #1114

Node 14.18 and 16.6¹ 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 ² ³ ⁴, 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.

packages/init/node-async_hooks.js

naugtur · 2022-03-15T19:42:51Z

packages/init/node-async_hooks.js

+};
+
+const setAsyncIdFallback = (promise, symbol, value) => {
+  const state = getAsyncHookFallbackState(promise, true);


(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.

naugtur · 2022-03-15T19:51:14Z

packages/init/node-async_hooks.js

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


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

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.

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

I think it created multiple promise instances for some reason.

gibson042

LGTM modulo some nits

packages/init/node-async_hooks.js

packages/init/test/test-async_hooks.js

packages/ses/src/whitelist.js

erights

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.

erights · 2022-03-16T00:05:06Z

packages/init/node-async_hooks.js

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


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 · 2022-03-16T02:42:58Z

@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 · 2022-03-16T18:23:00Z

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 · 2022-03-17T02:05:54Z

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

packages/init/pre-remoting.js

kriskowal · 2022-03-17T17:46:13Z

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

mhofman · 2022-03-17T17:48:24Z

packages/ses/package.json

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


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

mhofman · 2022-03-17T17:52:19Z

@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?

Co-authored-by: Richard Gibson <richard.gibson@gmail.com>

mhofman · 2022-03-17T19:27:46Z

@erights PTAL

erights

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.

erights

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 · 2022-03-19T13:39:49Z

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 · 2022-03-21T20:41:21Z

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.

kriskowal

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

packages/init/pre-remoting.js

packages/init/package.json

mhofman added endo ses labels Mar 15, 2022

mhofman requested review from kriskowal and erights March 15, 2022 19:08

mhofman linked an issue Mar 15, 2022 that may be closed by this pull request

Node bug causes failures within the vscode Debug Console #1105

Closed

mhofman force-pushed the mhofman/handle-async-hooks-promise branch from 958be31 to 4dbf59d Compare March 15, 2022 19:14

naugtur reviewed Mar 15, 2022

View reviewed changes

packages/init/node-async_hooks.js Outdated Show resolved Hide resolved

naugtur reviewed Mar 15, 2022

View reviewed changes

gibson042 approved these changes Mar 15, 2022

View reviewed changes

erights requested changes Mar 16, 2022

View reviewed changes

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 force-pushed the mhofman/handle-async-hooks-promise branch from 7764ea1 to d62e8cf Compare March 16, 2022 02:40

mhofman requested a review from erights March 16, 2022 02:41

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

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 force-pushed the mhofman/handle-async-hooks-promise branch from 02d90b9 to 65c52e3 Compare March 17, 2022 00:12

mhofman commented Mar 17, 2022

View reviewed changes

packages/init/pre-remoting.js Show resolved Hide resolved

mhofman commented Mar 17, 2022

View reviewed changes

mhofman added 3 commits March 17, 2022 18:53

feat: handle Promise symbols added by node async_hooks

63fc3f7

fix CI

228b6ba

Upgrade boolean args to prop bags

427b3ba

mhofman and others added 7 commits March 17, 2022 18:53

Avoid console calls in async_hooks code

3eb7f3b

Apply suggestions from code review

668c826

Co-authored-by: Richard Gibson <richard.gibson@gmail.com>

Split async hook bootstrap logic

cee6c5f

Do not register async hook symbols as well-known

1ae93f6

Add failing test for @endo/init compat with bundle

98d1c79

Use conditional exports to only perform async_hooks patch under node

f431f1d

Use package self-reference to reduce clunk of conditonal exports

4b73064

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

erights requested changes Mar 19, 2022

View reviewed changes

erights approved these changes Mar 19, 2022

View reviewed changes

erights mentioned this pull request Mar 19, 2022

fix: make Promise.resolve safe. #1126

Closed

Move node specific files into src

650c8d1

kriskowal approved these changes Mar 21, 2022

View reviewed changes

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

Comment copy

e24bfe0

mhofman merged commit 06827b9 into master Mar 21, 2022

mhofman deleted the mhofman/handle-async-hooks-promise branch March 21, 2022 21:24

mhofman mentioned this pull request Apr 20, 2022

fix(init): workaround node promise destroyed bug #1166

Merged

mhofman mentioned this pull request Aug 23, 2022

Fix safe promise checks #1264

Merged

mhofman mentioned this pull request Jul 20, 2023

Make async_hooks mitigation less intrusive in unsafe-fast #1688

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mhofman commented Mar 15, 2022 •

edited

Loading

naugtur Mar 15, 2022

naugtur Mar 15, 2022

mhofman Mar 15, 2022

naugtur Mar 15, 2022 •

edited

Loading

mhofman Mar 15, 2022

gibson042 left a comment

erights left a comment

erights Mar 16, 2022

mhofman commented Mar 16, 2022

mhofman commented Mar 16, 2022

mhofman commented Mar 17, 2022

kriskowal commented Mar 17, 2022

mhofman Mar 17, 2022 •

edited

Loading

mhofman commented Mar 17, 2022

mhofman commented Mar 17, 2022

erights left a comment

erights left a comment

mhofman commented Mar 19, 2022

kriskowal commented Mar 21, 2022 •

edited

Loading

kriskowal left a comment

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

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

Conversation

mhofman commented Mar 15, 2022 • edited Loading

Footnotes

naugtur Mar 15, 2022

Choose a reason for hiding this comment

naugtur Mar 15, 2022

Choose a reason for hiding this comment

mhofman Mar 15, 2022

Choose a reason for hiding this comment

naugtur Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

mhofman Mar 15, 2022

Choose a reason for hiding this comment

gibson042 left a comment

Choose a reason for hiding this comment

erights left a comment

Choose a reason for hiding this comment

erights Mar 16, 2022

Choose a reason for hiding this comment

mhofman commented Mar 16, 2022

mhofman commented Mar 16, 2022

mhofman commented Mar 17, 2022

kriskowal commented Mar 17, 2022

mhofman Mar 17, 2022 • edited Loading

Choose a reason for hiding this comment

mhofman commented Mar 17, 2022

mhofman commented Mar 17, 2022

erights left a comment

Choose a reason for hiding this comment

erights left a comment

Choose a reason for hiding this comment

mhofman commented Mar 19, 2022

kriskowal commented Mar 21, 2022 • edited Loading

kriskowal left a comment

Choose a reason for hiding this comment

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

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

mhofman commented Mar 15, 2022 •

edited

Loading

naugtur Mar 15, 2022 •

edited

Loading

mhofman Mar 17, 2022 •

edited

Loading

kriskowal commented Mar 21, 2022 •

edited

Loading