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

test: improve disable AsyncLocalStorage test #31998

Conversation

puzpuzpuz
Copy link
Member

Improves test-async-local-storage-enable-disable.js test, as it wasn't covering .disable() method's behavior as it should. See #31950 (comment) for more details.

cc @Qard @vdeturckheim

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Feb 28, 2020
@vdeturckheim
Copy link
Member

Can we fast-track this one? It just adds an assertion to a test.

@nodejs-github-bot
Copy link
Collaborator

@puzpuzpuz
Copy link
Member Author

Looks like tests are flaky. As far as I can see, failed tests are not related with this PR.

@nodejs-github-bot
Copy link
Collaborator

@vdeturckheim
Copy link
Member

@puzpuzpuz something went wrong in CI, I restarted it

@nodejs-github-bot
Copy link
Collaborator

@Qard
Copy link
Member

Qard commented Feb 29, 2020

There's actually a deeper issue than that.

'use strict';
require('../common');
const assert = require('assert');
const { AsyncLocalStorage } = require('async_hooks');

const asyncLocalStorage = new AsyncLocalStorage();
const store = {};

asyncLocalStorage.runSyncAndReturn(store, () => {
  process.nextTick(() => {
    // This will fail because the second `runSyncAndReturn` re-enabled the storage
    // between when it was disabled and when the `nextTick` callback runs.
    assert.strictEqual(asyncLocalStorage.getStore(), undefined);
  });
  asyncLocalStorage.disable();
});

asyncLocalStorage.runSyncAndReturn(store, () => {
    assert.strictEqual(asyncLocalStorage.getStore(), store);
});

In this example, they are in the same sync tick, but imagine two runs from two separate http requests with a less immediate async task like a fs.readFile(...). The first request does its run*(...) call, ending with disabling the storage. The next does a run*(...) call, re-enabling it. Then the fs.readFile(...) from the first request runs its callback in the context again because it was re-enabled by the second request. This is very strange and unexpected behaviour--the disable should persist.

@puzpuzpuz
Copy link
Member Author

puzpuzpuz commented Feb 29, 2020

@Qard

There's actually a deeper issue than that.

Your snippet shows expected behavior which is described in the doc: https://github.com/nodejs/node/blob/311e12b96201c01d6c66c800d8cfc59ebf9bc4ae/doc/api/async_hooks.md#asynclocalstoragedisable

This behavior (and the general approach for .disable() method) was discussed in #26540. At a certain degree, I agree with you and I'd prefer to have a terminal (non-reversible) action for such method, but the consensus was to go with the current approach.

In any case, this discussion seems to be unrelated with changes from this PR and should be moved into a GH issue or a separate PR. WDYT?

@Qard
Copy link
Member

Qard commented Feb 29, 2020

Ah, hmm...okay. I must have missed that. Seems like very strange behaviour to me though. It makes it impossible to actually fully disable the storage. 😕

Anyway, yes, we can take this concern elsewhere.

@nodejs-github-bot
Copy link
Collaborator

@puzpuzpuz
Copy link
Member Author

Another flaky CI run 😢. Could someone re-run it?

@nodejs-github-bot
Copy link
Collaborator

@puzpuzpuz puzpuzpuz force-pushed the async-local-storage-improve-disable-test branch from 6387b29 to d0f9175 Compare March 2, 2020 14:14
@puzpuzpuz
Copy link
Member Author

I have rebased over the latest master. Maybe those flaky tests are fixed and it would help CI build to pass.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29483/

@vdeturckheim vdeturckheim added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2020
vdeturckheim pushed a commit that referenced this pull request Mar 2, 2020
PR-URL: #31998
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@vdeturckheim
Copy link
Member

Landed in 68e36ad

@puzpuzpuz puzpuzpuz deleted the async-local-storage-improve-disable-test branch March 2, 2020 17:25
@codebytere codebytere mentioned this pull request Mar 3, 2020
codebytere pushed a commit that referenced this pull request Mar 3, 2020
PR-URL: #31998
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@puzpuzpuz
Copy link
Member Author

v12 backport: #32318

puzpuzpuz added a commit to puzpuzpuz/node that referenced this pull request Apr 14, 2020
PR-URL: nodejs#31998
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
PR-URL: nodejs#31998
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
PR-URL: #31998
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants