Async hooks cause programs with frozen promises to crash

[erights]

Version

v16.13.1

Platform

Darwin MacBook-Pro 21.3.0 Darwin Kernel Version 21.3.0: Wed Jan 5 21:37:58 PST 2022; root:xnu-8019.80.24~20/RELEASE_ARM64_T6000 arm64

Subsystem

No response

What steps will reproduce the bug?

Type the following interactively at the Node shell.

~$ node
Welcome to Node.js v16.13.1.
Type ".help" for more information.
> const p = Promise.resolve(8);
undefined
> const names = Reflect.ownKeys(p);
undefined
> names
[
  'domain',
  Symbol(async_id_symbol),
  Symbol(trigger_async_id_symbol),
  Symbol(destroyed)
]
> for (const name of names) {
...   delete p[name];
... }
true
> Object.freeze(p);
Promise { 8 }
> const q = p.then(x => console.log(x));
Uncaught:
TypeError: Cannot add property Symbol(async_id_symbol), object is not extensible
    at getOrSetAsyncId (node:internal/async_hooks:424:34)
    at trackPromise (node:internal/async_hooks:314:35)
    at promiseInitHook (node:internal/async_hooks:322:3)
    at promiseInitHookWithDestroyTracking (node:internal/async_hooks:329:3)
    at Promise.then (<anonymous>)
> undefined
> 8

The 'domain' property is not the main concern here. Rather it is the other three symbols. Hardened JS (aka SES) avoid loading the module that would add 'domain'. However, at the vscode debug terminal, we get a result much like the above but without 'domain' showing up in the list of names. Other differences between the following and the previous section are probably due to the differences caused by Hardened JS and might or might now be relevant. Other than the expected absence of 'domain', the underlying Node bug is the same:

In the debug console:

p = Promise.resolve(8);
Promise {[[PromiseState]]: 'fulfilled', [[PromiseResult]]: 8, Symbol(async_id_symbol): 3543, Symbol(trigger_async_id_symbol): 1783, Symbol(destroyed): {…}}
names = Reflect.ownKeys(p);
(3) [Symbol(async_id_symbol), Symbol(trigger_async_id_symbol), Symbol(destroyed)]
for (const name of names) {
  delete p[name];
}
true
Object.freeze(p);
Promise {[[PromiseState]]: 'fulfilled', [[PromiseResult]]: 8}
q = p.then(x => console.log(x));

That last line will terminate the debug session, with the following error appearing on vscode's JavaScript Debug Terminal

Debugger attached.
  - range queries

  Uncaught exception in test/test-rankOrder.js

  TypeError: Cannot add property Symbol(async_id_symbol), object is not extensible
    at Promise.then (<anonymous>)
    at eval (eval at <anonymous> (packages/store/test/test-rankOrder.js:94:7), <anonymous>:1:13)
    at packages/store/test/test-rankOrder.js:94:7
    at packages/store/test/test-rankOrder.js:90:12
    at async Promise.all (index 7)

  › Promise.then (<anonymous>)
  › eval (eval at <anonymous> (packages/store/test/test-rankOrder.js:94:7), <anonymous>:1:13)
  › packages/store/test/test-rankOrder.js:94:7
  › packages/store/test/test-rankOrder.js:90:12

(TypeError#1)
Waiting for the debugger to disconnect...
TypeError#1: Cannot add property Symbol(async_id_symbol), object is not extensible
  at Promise.then (<anonymous>)
  at eval (eval at <anonymous> (packages/store/test/test-rankOrder.js:94:7), <anonymous>:1:13)
  at packages/store/test/test-rankOrder.js:94:7
  at packages/store/test/test-rankOrder.js:90:12
  at async Promise.all (index 7)

  ✖ test/test-rankOrder.js exited with a non-zero exit code: 1
  ─

  1 test skipped
  1 uncaught exception
Waiting for the debugger to disconnect...
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Waiting for the debugger to disconnect...
Waiting for the debugger to disconnect...

Putting the same code in a .js file and executing it non-interactively does not trigger the bug. The names array in that case is empty, as it should be.

How often does it reproduce? Is there a required condition?

It reproduces reliably. The explanation above should be adequate to reliably reproduce it. Please let me know if you have any trouble reproducing it.

What is the expected behavior?

names should be empty.
q should be defined as a promise that eventually fulfills to p's fulfillment, 8.

What do you see instead?

As presented above:

~$ node
Welcome to Node.js v16.13.1.
Type ".help" for more information.
> const p = Promise.resolve(8);
undefined
> const names = Reflect.ownKeys(p);
undefined
> names
[
  'domain',
  Symbol(async_id_symbol),
  Symbol(trigger_async_id_symbol),
  Symbol(destroyed)
]
> for (const name of names) {
...   delete p[name];
... }
true
> Object.freeze(p);
Promise { 8 }
> const q = p.then(x => console.log(x));
Uncaught:
TypeError: Cannot add property Symbol(async_id_symbol), object is not extensible
    at getOrSetAsyncId (node:internal/async_hooks:424:34)
    at trackPromise (node:internal/async_hooks:314:35)
    at promiseInitHook (node:internal/async_hooks:322:3)
    at promiseInitHookWithDestroyTracking (node:internal/async_hooks:329:3)
    at Promise.then (<anonymous>)
> undefined
> 8

Additional information

Googling led me to
https://stackoverflow.com/questions/70742387/where-can-i-find-any-docs-on-why-node-14-changed-promise-into-promise-undefi
which led me to
f37c26b
and
#36394

However, that PR shows Closed rather than Merged, so it may not actually be the source of the bug.

[erights]

See also
endojs/endo#627
and
endojs/endo#126

[erights]

Attn @bmeck

[erights]

See also microsoft/vscode#144493

[RaisinTen]

However, that PR shows Closed rather than Merged, so it may not actually be the source of the bug.

The commit was landed manually, see #36394 (comment).

[Divine-Cipher-Cells]

Hello I am new to open source but familiar with Mern stack and thus would be really helpful if you provide me resources to contribute to this bug

[Flarna]

I'm not really sure why this should be a bug. If you use async hooks (which happens behind the scenes here by using domain) but tinker with the objects in a way that async hooks can't work anymore it's reasonable that a crash happens.

If you don't use async hooks then it's fine:

const ah = require("async_hooks");

ah.createHook({ init: () => {}}).enable(); // remove this line and it works for you

const p = Promise.resolve(8);
const names = Reflect.ownKeys(p);
console.log(names);
for (const name of names) {
  delete p[name];
}
Object.freeze(p);
const q = p.then(x => console.log(x));

There are a lot other cases where tinkering with objects result in crashes, e.g see following http sample:

const http = require("http");

const server = http.createServer((req, res) => {});
Object.freeze(server);
server.listen();

results in

node:net:1317
    this._handle = rval;
                 ^

TypeError: Cannot assign to read only property '_handle' of object '#<Server>'
    at Server.setupListenHandle [as _listen2] (node:net:1317:18)
    at listenInCluster (node:net:1378:12)
    at Server.listen (node:net:1465:7)
    at Object.<anonymous> (C:\work\node-42229\http.js:5:8)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47

[erights]

The two lines you mention

const ah = require("async_hooks");

ah.createHook({ init: () => {}}).enable(); // remove this line and it works for you

do not appear in my source code. Where would I remove them from? Put another way, how can I omit Node's async_hooks?

[Flarna]

If I understand your initial posting correct you use repl to execute your code.
repl uses domain module see here
domain uses async hooks see here

So even the code is not visible it's executed as your setup depends on it.

Edit: As far as I know domain is the only module within nodejs itself which uses async-hooks (besides AsyncLocalStorage which is part of async_hooks module so it's quite oblivious that is it used there).
Additionally I think repl is the only internal module which uses domain. So don't use them and you should be fine.

I don't think there is any switch to remove existence of domain, async_hooks and repl from node.
Clearly once you use any 3rd party package you have to look if they rely on domain and/or async_hooks.

[mhofman]

I'm still trying to track down what entrains async_hook, but from what I've gathered so far:

• Connecting with Chrome devtools or VS Code JS debugger does trigger this behavior
• Connecting with node inspect 127.0.0.1:9229 does not

Here is the test program that I'm using:

const p = Promise.resolve(8);
const names = Reflect.ownKeys(p);
console.log("p own keys", names);
for (const name of names) {
  delete p[name];
}
Object.freeze(p);
debugger;
const q = p.then((x) => console.log(x));
console.log(q);

If the devtool/VSCode are attached either at start, or as late as the debugger statement, then the following line will throw during the .then with the stack reported above.

Regardless of what triggers the creation of the hooks, I believe this is a bug in the async_hooks implementation which should be resilient to the extensibility state of objects it's attempting to modify. In particular here getOrSetAsyncId should not perform an assignment without try/catch. There may be similar cases. Is there actually a reason async_hooks tries to modify objects instead of using a WeakMap to store its internal information?

[Flarna]

I think the VS code debugger uses async_hooks internally.

There may be similar cases. Is there actually a reason async_hooks tries to modify objects instead of using a WeakMap to store its internal information?

I guess performance but not 100% sure if this is the only reason.

Adding a try/catch in getOrSetAsyncId would avoid the exception above but it would break the underlying functionality. I think you should not use async hooks if you don't want them.

[jasnell]

Regardless of what triggers the creation of the hooks, I believe this is a bug in the async_hooks implementation which should be resilient to the extensibility state of objects it's attempting to modify. In particular here getOrSetAsyncId should not perform an assignment without try/catch. There may be similar cases. Is there actually a reason async_hooks tries to modify objects instead of using a WeakMap to store its internal information?

Node.js will attach async context metadata to any object that can be tracked with async_hooks regardless of whether async_hooks is actively being used or not within code and yes, finding a way of tracking the information without modifying the objects themselves would be ideal. I'd certainly +1 a refactor on that -- though it's likely to be a non-trivial change.

[mhofman]

I think you should not use async hooks if you don't want them.

We do not want to use async_hooks. If I could prevent it from loading I would (is there a way without modifying node?). However, we and our users need to be able to attach a debugger, without it crashing the program being debugged.

Node.js will attach async context metadata to any object that can be tracked with async_hooks regardless of whether async_hooks is actively being used or not within code

That is not my observation. If nothing triggers async_hooks (like a debugger), promise objects are not modified and everything runs fine.