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

domain AsyncHook TypeError: Cannot read properties of undefined (again) #40999

Closed
cemerick opened this issue Nov 28, 2021 · 7 comments · Fixed by #43556
Closed

domain AsyncHook TypeError: Cannot read properties of undefined (again) #40999

cemerick opened this issue Nov 28, 2021 · 7 comments · Fixed by #43556
Labels
async_hooks Issues and PRs related to the async hooks subsystem. confirmed-bug Issues with confirmed bugs. domain Issues and PRs related to the domain subsystem.

Comments

@cemerick
Copy link

Version

v16.13.0

Platform

Linux t440p 5.4.0-89-generic #100-Ubuntu SMP Fri Sep 24 14:50:10 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

domain

What steps will reproduce the bug?

Fairly minimal reproduction, eval.js:

let createDomain = require("domain").create;
let vm = require("vm");

let context = vm.createContext({});
function eval(code) {
    let domain = createDomain();
    domain.run(() => {
      vm.runInContext(code, context)()
        .then(v => { console.log(v) })
        .catch(console.error);
    });
      
}
for (i = 0; i < 1000; i++) {
  eval("async () => null");
}
$ node --abort-on-uncaught-exception eval.js 
TypeError: Cannot read properties of undefined (reading 'enter')
    at AsyncHook.before (node:domain:97:20)
    at emitHook (node:internal/async_hooks:237:38)
    at emitBeforeScript (node:internal/async_hooks:503:5)
    at promiseBeforeHook (node:internal/async_hooks:347:3)
 1: 0xb02ec0 node::Abort() [node]
 2: 0xb76589  [node]
 3: 0xd4a18e  [node]
 4: 0xd4b5af v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [node]
 5: 0x15e7959  [node]
Aborted (core dumped)

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

The script runs without fault in node v12.22.7, but provokes failure reliably on stock node (as delivered by nvm) in versions:

  • 14.18.0
  • 16.13.0
  • 17.1.0

Note that failure is only sporadic if the number of evaluations scheduled (the upper bound of the for loop) is cut to, say, 100 or 500, and the script runs without fault when the number is set to something very small like 10.

What is the expected behavior?

No failure, as in node v12.x.

What do you see instead?

No response

Additional information

A very similar-looking issue was reported in #30122, but the script provided here does work as expected in the failing version of node reported there (12.13.0).

@VoltrexKeyva VoltrexKeyva added the domain Issues and PRs related to the domain subsystem. label Nov 28, 2021
@targos
Copy link
Member

targos commented Nov 29, 2021

@nodejs/async_hooks

@vdeturckheim
Copy link
Member

It seems to be a recurring issue of vm and async hooks not playing well together. I believe @Qard noticed it too.

@Flarna Flarna added the async_hooks Issues and PRs related to the async hooks subsystem. label Dec 6, 2021
@Qard
Copy link
Member

Qard commented Dec 9, 2021

As far as I can tell it's less an async_hooks issue and more an issue with domains, vm contexts, and gc timing. I don't really have the time to investigate properly right now though. 😕

@cemerick
Copy link
Author

Having seen this failure a couple of times in production (and not wanting to use node v12.x pending an actual fix), I tinkered a bit, and found what seems like a reliable workaround: setting a dummy timeout prior to using any newly-created domain. Here's a revision of the minimal repro from the original issue description, with such a timeout added:

let createDomain = require("domain").create;
let vm = require("vm");

let context = vm.createContext({});
function eval(code) {
  let domain = createDomain();
  setTimeout(_ => {
    domain.run(() => {
      vm.runInContext(code, context)()
        .catch(console.error);
    })
  }, 0);
}

for (i = 0; i < 10000; i++) {
  eval("async () => null");
}

Whether this works because it's helping the program dodge a timing bug or not is definitely above my pay grade here, but maybe someone will find it helpful.

(Interestingly, v12 runs the above in half the time as v16. Obviously it's a pathological bit of code, but it seems notable.)

@Mesteery Mesteery added the confirmed-bug Issues with confirmed bugs. label Dec 29, 2021
MrJithil added a commit to MrJithil/node that referenced this issue Jan 15, 2022
MrJithil added a commit to MrJithil/node that referenced this issue Jan 15, 2022
MrJithil added a commit to MrJithil/node that referenced this issue Jan 15, 2022
fix: uncaught exception on domain
Fixes: nodejs#40999
MrJithil added a commit to MrJithil/node that referenced this issue Jan 15, 2022
fix uncaught exception on domain
Fixes: nodejs#40999
MrJithil added a commit to MrJithil/node that referenced this issue Jan 15, 2022
fix uncaught exception on domain
Fixes: nodejs#40999
MrJithil added a commit to MrJithil/node that referenced this issue Jan 15, 2022
fix uncaught exception on domain
Fixes: nodejs#40999
MrJithil added a commit to MrJithil/node that referenced this issue Jan 16, 2022
fix uncaught exception on domain
Fixes: nodejs#40999
@bagelbits
Copy link

bagelbits commented Mar 8, 2022

I've noticed this is not an issue in 14.15, but is still an issue in 14.19.

@bagelbits
Copy link

Anecdotally, this issue is also not present in 14.17.

@yocontra
Copy link

yocontra commented Jun 5, 2022

Chiming in - I get this issue consistently with code like this on the latest node 16:

const transform = (...args) =>
  new Promise((resolve, reject) => {
    const internalDomain = domains.create()
     internalDomain.on('error', reject) // report async errors
     let out
     internalDomain.run(() => {
       out = fn(...args)
     })
     resolve(out)
  })

This is creating a domain within a promise, then running a function created using the VM2 module inside of the domain. This consistently reproduces for me even with 1 or 2 invocations, usually on the first invocation but every so often it will continue on for a bit before failing. Wrapping each domain.run call with a setTimeout(0) or a nextTick does not seem to help as suggested above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. confirmed-bug Issues with confirmed bugs. domain Issues and PRs related to the domain subsystem.
Projects
None yet
9 participants