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

CLS store gets reset on the first callback in http scenario #32060

Closed
HarshithaKP opened this issue Mar 3, 2020 · 7 comments
Closed

CLS store gets reset on the first callback in http scenario #32060

HarshithaKP opened this issue Mar 3, 2020 · 7 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. confirmed-bug Issues with confirmed bugs.

Comments

@HarshithaKP
Copy link
Member

  • Version: master
  • Platform: Linux
  • Subsystem: async_hooks

What steps will reproduce the bug?

I have been trying to write some new scenarios for AsyncLocalStorage class, as mentioned in #31978, and came across this issue:

const { AsyncLocalStorage } = require('async_hooks');
const http = require('http')
const cls = new AsyncLocalStorage();

const server = http.createServer((req, res) => {
  res.write('hello')
  setTimeout(() => {
    res.end(' world!')
  }, 1000)
})

server.listen(12000, () => {
  cls.run(() => {
    const req = http.get('http://localhost:12000', (res) => {
      const store = cls.getStore()
      store.set('foo', '')
      res.on('data', (d) => { console.log(cls.getStore()) });
    })
    req.end()
  })
})

In this simple client-server program, the server is sending two chunks of data, forcing the ondata handler to be invoked twice.

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

every time

What is the expected behavior?

I get an empty Map every time in the ondata callback

What do you see instead?

Unfortunately, the store is undefined after the first invocation:

$ node foo.js
Map(1) { 'foo' => '' }
undefined
^C
$

Additional information

If I replace this with a simple timer code, this logic works fine:

const { AsyncLocalStorage } = require('async_hooks');
const cls = new AsyncLocalStorage();

cls.run(() => {
  const store = cls.getStore()
  store.set('foo', 'bar')
  setInterval(() => {
    console.log(cls.getStore().get('foo'))
  }, 1000)
}) 

$ node timer.js

bar
bar
bar
bar
^C

Am I missing something?

Ping @vdeturckheim

@vdeturckheim
Copy link
Member

Thanks @HarshithaKP , it's weird. I'll take a look. But It seems you are using an older version of the API. Did you compile Node.js from sources or did you use the userland module I created to test the API?

@vdeturckheim vdeturckheim added the async_hooks Issues and PRs related to the async hooks subsystem. label Mar 3, 2020
@HarshithaKP
Copy link
Member Author

@vdeturckheim, thanks for the quick response. I am on Node.js master built from source. My HEAD is at d4e4480093319f6d8f3a26be6aad8c02eb7d589a, were there recent changes went it ?

@vdeturckheim
Copy link
Member

vdeturckheim commented Mar 3, 2020

@HarshithaKP I managed to isolate the bug in executionAsyncResource() (which is the underlying API used by AsyncLocalStorage:

I updated the test test/async-hooks/test-async-exec-resource-http.js:

'use strict';

require('../common');
const assert = require('assert');
const {
  executionAsyncResource,
  executionAsyncId,
  createHook,
} = require('async_hooks');
const http = require('http');

const hooked = {};
createHook({
  init(asyncId, type, triggerAsyncId, resource) {
    hooked[asyncId] = resource;
  }
}).enable();

const server = http.createServer((req, res) => {
  res.write('hello');
  setTimeout(() => {
    res.end(' world!');
  }, 1000);
});

server.listen(0, () => {
  assert.strictEqual(executionAsyncResource(), hooked[executionAsyncId()]);
  http.get({ port: server.address().port }, (res) => {
    assert.strictEqual(executionAsyncResource(), hooked[executionAsyncId()]);
    res.on('data', () => {
      assert.strictEqual(executionAsyncResource(), hooked[executionAsyncId()]);
    });
    res.on('end', () => {
      assert.strictEqual(executionAsyncResource(), hooked[executionAsyncId()]);
      server.close();
    });
  });
});

and now it fails.
CC @mcollina @Qard and @Flarna who understand this mechanism better than me:
basically the resource in the second call to res.on('data', () => { does not seem to be gne through the hook even if it has the same id as the resource on the line before.

@vdeturckheim vdeturckheim added the confirmed-bug Issues with confirmed bugs. label Mar 3, 2020
@Flarna
Copy link
Member

Flarna commented Mar 3, 2020

I did a fast look and it seems there is still an issue related to reuse of HTTPParser. On my local machine above test fails on second data event because executionAsyncResource() returns an HTTPParser but hooked[executionAsyncId()] returns an instance of HTTPClientAsyncResource.

@addaleax
Copy link
Member

addaleax commented Mar 3, 2020

Pretty sure this is an oversight in the PR that added executionAsyncResource(). I’ll open a PR.

addaleax added a commit to addaleax/node that referenced this issue Mar 3, 2020
This was an oversight in 9fdb6e6.
Fixing this is necessary to make `executionAsyncResource()` work
as expected.

Refs: nodejs#30959
Fixes: nodejs#32060
@addaleax
Copy link
Member

addaleax commented Mar 3, 2020

#32063, with the test copied from above.

@vdeturckheim
Copy link
Member

Thanks a lot @addaleax !

HarshithaKP added a commit to HarshithaKP/node that referenced this issue Mar 4, 2020
Add a new scenario of multiple clients sharing a single data
callback function managing their response data through
AsyncLocalStorage APIs

Refs: nodejs#32063
Refs: nodejs#32060
Refs: nodejs#32062 (comment)

Co-authored-by: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 4, 2020
This was an oversight in 9fdb6e6.
Fixing this is necessary to make `executionAsyncResource()` work
as expected.

Refs: #30959
Fixes: #32060

PR-URL: #32063
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this issue Mar 13, 2020
Add a new scenario of multiple clients sharing a single data
callback function managing their response data through
AsyncLocalStorage APIs

Refs: #32063
Refs: #32060
Refs: #32062 (comment)

Co-authored-by: Gireesh Punathil <[email protected]>

PR-URL: #32082
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit that referenced this issue Mar 17, 2020
Add a new scenario of multiple clients sharing a single data
callback function managing their response data through
AsyncLocalStorage APIs

Refs: #32063
Refs: #32060
Refs: #32062 (comment)

Co-authored-by: Gireesh Punathil <[email protected]>

PR-URL: #32082
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 24, 2020
Add a new scenario of multiple clients sharing a single data
callback function managing their response data through
AsyncLocalStorage APIs

Refs: #32063
Refs: #32060
Refs: #32062 (comment)

Co-authored-by: Gireesh Punathil <[email protected]>

PR-URL: #32082
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
puzpuzpuz pushed a commit to puzpuzpuz/node that referenced this issue Apr 14, 2020
This was an oversight in 9fdb6e6.
Fixing this is necessary to make `executionAsyncResource()` work
as expected.

Refs: nodejs#30959
Fixes: nodejs#32060

PR-URL: nodejs#32063
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
puzpuzpuz pushed a commit to puzpuzpuz/node that referenced this issue Apr 14, 2020
This was an oversight in 9fdb6e6.
Fixing this is necessary to make `executionAsyncResource()` work
as expected.

Refs: nodejs#30959
Fixes: nodejs#32060

PR-URL: nodejs#32063
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
puzpuzpuz pushed a commit to puzpuzpuz/node that referenced this issue Apr 14, 2020
Add a new scenario of multiple clients sharing a single data
callback function managing their response data through
AsyncLocalStorage APIs

Refs: nodejs#32063
Refs: nodejs#32060
Refs: nodejs#32062 (comment)

Co-authored-by: Gireesh Punathil <[email protected]>

PR-URL: nodejs#32082
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this issue Apr 25, 2020
This was an oversight in 9fdb6e6.
Fixing this is necessary to make `executionAsyncResource()` work
as expected.

Refs: nodejs#30959
Fixes: nodejs#32060

PR-URL: nodejs#32063
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this issue Apr 25, 2020
Add a new scenario of multiple clients sharing a single data
callback function managing their response data through
AsyncLocalStorage APIs

Refs: nodejs#32063
Refs: nodejs#32060
Refs: nodejs#32062 (comment)

Co-authored-by: Gireesh Punathil <[email protected]>

PR-URL: nodejs#32082
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Apr 28, 2020
This was an oversight in 9fdb6e6.
Fixing this is necessary to make `executionAsyncResource()` work
as expected.

Refs: #30959
Fixes: #32060

PR-URL: #32063
Reviewed-By: Vladimir de Turckheim <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Apr 28, 2020
Add a new scenario of multiple clients sharing a single data
callback function managing their response data through
AsyncLocalStorage APIs

Refs: #32063
Refs: #32060
Refs: #32062 (comment)

Co-authored-by: Gireesh Punathil <[email protected]>

PR-URL: #32082
Reviewed-By: Anna Henningsen <[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. confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants