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

[Bug]: AsyncLocalStorage context loss when using enterWith #13653

Closed
jenseng opened this issue Dec 1, 2022 · 2 comments
Closed

[Bug]: AsyncLocalStorage context loss when using enterWith #13653

jenseng opened this issue Dec 1, 2022 · 2 comments

Comments

@jenseng
Copy link

jenseng commented Dec 1, 2022

Version

29.3.1

Steps to reproduce

  1. Clone https://github.com/jenseng/jest-als-bug
  2. Run npm install
  3. Run npm run test

Expected behavior

The tests should pass

Actual behavior

One or more tests will typically fail due to AsyncLocalStorage context loss

Additional context

This bug is similar to #11435 and nodejs/node#38781, but appears to have a different root cause, as it affects the latest versions of Jest and Node.js.

The tests in question (1, 2) set a store via AsyncLocalStorage#enterWith in a beforeEach, and attempt to retrieve it via AsyncLocalStorage#getStore during the test, e.g.

const store = new AsyncLocalStorage();
beforeEach(async () => {
  store.enterWith("one");
});

it("the store should still be set", () => {
  expect(store.getStore()).toEqual("one");
});

Observations:

  1. When run individually, the tests always pass. When run together, one will usually fail, regardless of settings like runInBand or maxWorkers (though in a real app's test suite it's much worse with maxWorkers). Based on my reading of the Jest codebase, the tests should pass, since the test execution follows the beforeEach hooks via a promise chain, and enterWith should "[persist] the store through any following asynchronous calls"
  2. Jest does not appear to have similar context loss issues when using AsyncLocalStorage#run, e.g. wrapping async operations with it works just fine
  3. I've also attempted to repro the issue outside of Jest while implementing roughly the same code path, but have so far been unsuccessful, leading me to conclude this must be something specific to Jest.

Based on the above, it seems the bug may be due to the interplay between Node.js's promise hooks and Jest's vm global/context injection (e.g. perhaps the Promise provided to the VM is not the "right" one, and thus the hooks don't work correctly when it is used?)

Environment

System:
    OS: macOS 12.5.1
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
  Binaries:
    Node: 18.12.1 - ~/.newt-cache/node-versions/n/versions/node/18.12.1/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v16.16.0/bin/yarn
    npm: 9.1.3 - ~/.newt-cache/npm-versions/9.1.3/bin/npm
  npmPackages:
    jest: ^29.3.1 => 29.3.1
@jenseng
Copy link
Author

jenseng commented Dec 13, 2022

It's looking like this is mainly a Node.js issue, and I've got a simple non-Jest repro 1. The problem can also go away in Jest if you comment out this line -- this allows the beforeEach ALS context setting to moves up the async stack to encompass the entire runTest, allowing it to apply to other async operations like _callCircusTest.

The tl;dr: is if you:

  1. Activate one ALS store (either via enterWith or run) (e.g. in one test's beforeEach)
  2. Activate another ALS store within an async/promise chain (e.g. in another test's beforeEach)
  3. Then the second store's context will be lost in later dependent async operations (the test). If you skip step 1 it works.

On the Node.js side this kind of feels like a documentation issue, but also a bug in the sense that it works some of the time (depending on other stores). Based on how/where the various async operations are created, it seems like setting ALS context in a beforeEach should never carry forward. Though from a Jest end-user perspective it would be nice if it always worked, or if Jest provided an aroundEach mechanism.

For anyone else who comes across this issue, if you want to have some automatic ALS setup for each test, beforeEach is probably a non-starter for now. More resilient approaches might be to 1. enterWith at the start of each test, or 2. roll your own test function that wraps your test in an ALS run.

Footnotes

  1. Simple non-Jest repro:

    const { AsyncLocalStorage } = require("async_hooks");
    const assert = require("assert");
    
    const store = new AsyncLocalStorage();
    
    const unrelatedStore = new AsyncLocalStorage();
    unrelatedStore.enterWith("value"); // it works if you comment this out
    
    function beforeEach() {
      store.enterWith("one");
    }
    
    function test() {
      assert.equal(store.getStore(), "one");
    }
    
    async function testHarness() {
      await Promise.resolve().then(beforeEach); // it also works if you just `await beforeEach()`
      await Promise.resolve().then(test);
    }
    testHarness();
    

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant