Async hooks cause programs with frozen promises to crash #42229
Comments
|
See also |
|
Attn @bmeck |
|
See also microsoft/vscode#144493 |
The commit was landed manually, see #36394 (comment). |
|
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 |
VoltrexKeyva
added
the
async_hooks
|
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 |
|
The two lines you mention const ah = require("async_hooks");
ah.createHook({ init: () => {}}).enable(); // remove this line and it works for youdo not appear in my source code. Where would I remove them from? Put another way, how can I omit Node's async_hooks? |
|
If I understand your initial posting correct you use repl to execute your code. So even the code is not visible it's executed as your setup depends on it. Edit: As far as I know I don't think there is any switch to remove existence of domain, async_hooks and repl from node. |
|
I'm still trying to track down what entrains
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 Regardless of what triggers the creation of the hooks, I believe this is a bug in the |
|
I think the VS code debugger uses async_hooks internally.
I guess performance but not 100% sure if this is the only reason. Adding a try/catch in |
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. |
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.
That is not my observation. If nothing triggers |
|
I think for the VS code debugger it's possible to disable use of async hooks by adding |
|
Thanks, that indeed does it for VS Code! I would like to re-iterate that:
I'll rename the issue to capture this better. Of course even better would be for |
erights
changed the title
|
Yes, I'm sure about the versions. I just double checked:With 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);And VS Code's {
"version": "0.2.0",
"configurations": [
{
"type": "node",
"request": "attach",
"name": "Attach Inspector",
"skipFiles": [],
"address": "127.0.0.1",
"port": 9231,
"protocol": "inspector",
"showAsyncStacks": true
}
]
}
|
|
Hmm I just realized that #36394 landed in 36b948560c, so would be in |
|
#39135 seems like the likely culprit. Landing in both v14.18 and v16.6 |
|
I think I understand the full picture now:
The inspector uses a destroy hook but my sample above does not. |
|
Thanks for clarifying the picture. What is the way forward on this? While we have a workaround for now (disabling async stack traces in the debugger), we'd like to be able to restore that debugging information. I understand the changes in async_hooks were performance motivated, but they came at the cost of breaking the ability to debug our environment. I assume moving to WeakMap instead of attaching symbols would alleviate this, but I understand this is a possibly large change, which might have a negative performance impact. Is that the only solution? |
|
Just to throw another option out here, I don't know what the performance characteristics would be compared to weakkmaps but private symbols (a v8 hack/feature) are not effected by freezing. https://github.com/devsnek/private-symbol/blob/master/private.cc |
|
Seems like it'd be fairly easy to try? However I'm not sure of how closely the current symbols are held inside node_internal, and if there'd be a risk to leak out this private symbol. |
|
Sigh. This terrible "private symbol" kludge took all the effort recapitulates the runtime arrangement of an efficient weakmap implementation, i.e., one using the transposed representation. All the effort and none of the safety. Perhaps, it seems possible this mechanism could be encapsulated in a shim that presents exactly the API and semantics of weakmaps, without the cost of v8 (non-transposed) weakmaps. |
|
i had a feeling you might say something like that mark :) i think as long as this is only used for internal implementation of the runtime and not more generally it should be alright. that's also assuming that it is an improvement over actual weakmaps. |
Could anyone measure? Please? |
|
Are these private symbols visible anywhere? e.g. in special in debugger it's quite important to see them otherwise working with them would be quite a pain. The JS language is still a source of surprises for me. On the one hand monkeypatching, adding this and that property at any time and at any place (e.g. mixins),... are used widely. But on the other hand there are restrictions where not even debugger or other tooling can do anything (e.g. ECMA script modules, private symbols). |
|
To be clear, these private symbols are outside JS. There is no reason for JS tooling to do anything reasonable with them. The only reason anyone would be tempted to use them is that they're much more efficient than weakmaps. If that's the case, we can encapsulate this kludgy mechanism within a shim of an efficient weakmap. We can then pressure v8 to make the actual weakmaps competitive with the shim. Since this thread on the node rather than v8, you should simply use the actual weakmaps. Does anyone have any measurements to indicate whether there actually is a performance issue? |
|
After chatting with @bmeck, it looks like private symbols wouldn't be efficient enough for this use case, and a |
|
I'm not aware of any actual measurements. As far as I remember the memory overhead of WeakMaps was quite bad a few years ago but that's too long ago to count as source of truth. But if it goes towards overhead it's needed to look at CPU and Memory overhead. Edit: found the issue where this is mentioned: nodejs/diagnostics#188 |
|
Just took a look at what adding a fallback mechanism would look like (which is basically a targeted refactor to get/set helpers just for promises), and I'm running into a complication as it seems the native side is directly accessing these symbols as well: Lines 116 to 151 in e8ac3b4 I think this is where my abilities stop. Does anyone have suggestions on how best to proceed? |
|
you could modify |
|
I'm sure someone could, but I'm not able to myself (it's been 15 years since I touched C code) |
|
These get measured various times all over every year, most recent one on social media was : https://github.com/RafaelGSS/nodejs-bench-operations/blob/main/bench/private-property.js some tweaks to add WeakMap backed checks w/ grouping to ease cost of lookup for example give my local machine: Node.js Benchmark Operations
private-property.jsNewer V8 has fixes for private fields: Node.js Benchmark Operations
private-property.jsWeakMaps are > 600 times slower even with grouping to reduce cost and Return Override is also hellish at ~15 times slower. I don't think they should be used in normal operation. |
|
I'll just leave this (monkeypatch) workaround here. No need to tell the developer to turn off async stacktraces manually const a = require('async_hooks');
const bkp = a.createHook
const noop = _ => { }
a.createHook = function () {
//process._rawDebug(Error().stack)
if (Error().stack.includes('node:internal/inspector_async_hook')) {
return { enable: noop, disable: noop }
}
return bkp.apply(this, arguments)
} |
FWIW, this is ridiculous. We should explain the brilliant weakmap representation and algorithm Moddable recently invented and implemented, which avoids the difficult choice between transposed and non-transposed representation. With one adjustment, it could provide reasonable efficiency and complexity measure under all scenarios. Having thought about ephemeron algorithms for a long time, the Moddable solution is by far the best solution I've seen. We should encourage v8 to adopt it. @phoddie @patrick-soquet , we should talk about publishing your weakmap gc algorithm! |
|
Our implementation of weakmaps in XS is in our repository for all to see. It could be a little challenging to infer the details from the code though. Perhaps we should write something about that. |
|
@phoddie to see if i'm understanding this correctly, |
|
Here's my first step to understanding it. Prior to any form of weakness, normal gc only has OR joins: If A points at C and B points at C then C is reachable if either A is reachable OR B is reachable. From an API and authority perspective, WeakMaps are asymmetric. But from a GC algorithm perspective, WeakMaps introduce symmetric AND joins into GC. After The Moddable GC algorithm might notice first either that reachable M at K points at V or that reachable K at M points at V. Whichever it notices first, it then transitions into a representation such that iff it notices the other one is also reachable, then it decides that V is reachable. |
|
While I find the discussions regarding WeakMap algorithms here really interesting I doubt that this will result in a fast fix. May I ask what are the exact requirements for SES? Please note that async hooks expose quite some internals in general. On every init hook you get a reference to the object created. Some of them are node internal objects which end user may never see via the public APIs (e.g. an HTTP Parser,...). This is one of the main reasons why async hooks never exited experimental status. Therefore I wonder if SES can accept the existence of an installed async hook at all - independent of implementation details like WeakMap/private symbols,... |
|
@Flarna SES guarantees could be broken by the program running in a hardened compartment accessing hooks. That won't be allowed. The issue here is developers using SES should still be able to use devtools/debugger. |
|
To expand on @naugtur's point:
In the case of async hooks triggered by debugging, the hooks themselves are never exposed to the program, so I believe there is no endowment issue. Also the action of debugging can only be taken if you are already in a privileged position. I have some workarounds we can put in place to mitigate this debugging use case, avoiding crashes on frozen promises, while maintaining debugger async traces. However they are not optimal, and I would like to know if some of the following could be considered:
I'd still like to find a solution to avoid leaking the async ids for all promise objects, or at least an option to put the async hooks into a mode where it doesn't create those props, but as I mentioned, there is the issue of native code requiring these symbols's presence. |
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.
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:
That last line will terminate the debug session, with the following error appearing on vscode's JavaScript Debug Terminal
Putting the same code in a .js file and executing it non-interactively does not trigger the bug. The
namesarray 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?
namesshould be empty.qshould be defined as a promise that eventually fulfills to p's fulfillment, 8.What do you see instead?
As presented above:
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.
The text was updated successfully, but these errors were encountered: