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

AsyncLocalStorage potential issue ? #33580

Closed
FranckFreiburger opened this issue May 27, 2020 · 4 comments
Closed

AsyncLocalStorage potential issue ? #33580

FranckFreiburger opened this issue May 27, 2020 · 4 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. doc Issues and PRs related to the documentations. question Issues that look for answers.

Comments

@FranckFreiburger
Copy link

resource[this.kResourceStore] = store;

You are using the resource object to store the store whereas the doc state :

In some cases the resource object is reused for performance reasons, it is thus not safe to use it as a key in a WeakMap or add properties to it.

I just wondering if it is ok.

@targos targos added the async_hooks Issues and PRs related to the async hooks subsystem. label May 27, 2020
@vdeturckheim
Copy link
Member

Hello @FranckFreiburger !

that's a good point. I remember we discussed it with @Qard , @puzpuzpuz and @Flarna on the PR but I can't find it back. There might actually be a memory leak in case of a reused resource.
I need to circle back in the discussions but this is a massive one and Github has a hard time with it(#26540).

@Qard
Copy link
Member

Qard commented May 27, 2020

All existing resource reuse should be using an AsyncResource for each reuse, so that should no longer be true. It would be a bug if anything doesn't use AsyncResource when reusing a resource now.

The line in the doc is not exactly true anymore, though I hesitate slightly to remove it as it's still possible for native modules to reuse a resource without an AsyncResource and there's not really anything we can do to stop them. I lean on the side of continuing to discourage people from relying on that capability.

@puzpuzpuz puzpuzpuz added question Issues that look for answers. doc Issues and PRs related to the documentations. labels May 28, 2020
@Flarna
Copy link
Member

Flarna commented May 28, 2020

There should be no reuse anymore within node core - at least not in the sense of the resource visible via async hooks.
HTTPParser objects are still reused and also the Socket in HTTP agent. But on every reuse a new "wrapper object" passed to AsyncHooks is created (even it's not of type AsyncResource).

see e.g

handle.asyncReset(new ReusedHandle(handle.getProviderType(), handle));
for resued socket handle in HTTP agent.

But there is no way to gurantee that some userland module reuses an resource object. And this potential reuse can be native or managed.

There is an open PR regarding potential wrong reuse via NAPI: #32930 But again it dependes on the actual native addon.

@puzpuzpuz
Copy link
Member

@FranckFreiburger
As the question seems to be answered, is it ok to close the issue? Please feel free to submit a PR that would add more context to the doc.

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. doc Issues and PRs related to the documentations. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

6 participants