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

TypeError: Method Map.prototype.set called on incompatible receiver #<AsyncContextFrame> #54503

Closed
bengl opened this issue Aug 22, 2024 · 2 comments · Fixed by #54510
Closed
Labels
async_hooks Issues and PRs related to the async hooks subsystem.

Comments

@bengl
Copy link
Member

bengl commented Aug 22, 2024

Version

v22.7.0

Platform

Darwin poutine-mac 23.6.0 Darwin Kernel Version 23.6.0: Fri Jul  5 17:56:41 PDT 2024; root:xnu-10063.141.1~2/RELEASE_ARM64_T6000 arm64 arm Darwin

Subsystem

async_hooks

What steps will reproduce the bug?

node --experimental-async-context-frame -e "new async_hooks.AsyncLocalStorage().run({}, () => {})"

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

100% always happens.

What is the expected behavior? Why is that the expected behavior?

The above command should just exit cleanly with no output.

What do you see instead?

node:internal/async_context_frame:56
    this.set(store, data);
         ^

TypeError: Method Map.prototype.set called on incompatible receiver #<AsyncContextFrame>
    at AsyncContextFrame.set (<anonymous>)
    at new AsyncContextFrame (node:internal/async_context_frame:56:10)
    at AsyncLocalStorage.enterWith (node:internal/async_local_storage/async_context_frame:24:19)
    at AsyncLocalStorage.run (node:internal/async_local_storage/async_context_frame:30:10)
    at [eval]:1:37
    at runScriptInThisContext (node:internal/vm:209:10)
    at node:internal/process/execution:118:14
    at [eval]-wrapper:6:24
    at runScript (node:internal/process/execution:101:62)
    at evalScript (node:internal/process/execution:136:3)

Node.js v22.7.0

Additional information

While the test suite includes the running of async-local-storage tests with the flag enabled, it doesn't actually check whether those files actually passed or failed. It just runs them. That's why this wasn't caught.

I think I have a fix ready. I'm just waiting on a compile to verify.

@RedYetiDev RedYetiDev added the async_hooks Issues and PRs related to the async hooks subsystem. label Aug 22, 2024
bengl added a commit to bengl/node that referenced this issue Aug 22, 2024
This gives a class prototype for AsyncContextFrame that contains the
required methods, so that when we swap the prototype,
ActiveAsyncContextFrame methods are used instead. Previously, the
methods were defined in AsyncContextFrame, so swapping the prototype
didn't swap those static methods.

Also, make the ActiveAsyncContextFrame extend from Map.

Fixes: nodejs#54503
@andreialecu
Copy link

I was excited about trying --experimental-async-context-frame to check if it helps with open telemetry tracing overhead, and I just ran into this issue.

CleanShot 2024-08-23 at 15 39 05@2x

@RedYetiDev
Copy link
Member

$ node --experimental-async-context-frame -e "new async_hooks.AsyncLocalStorage().run({}, () => {})" 
node:internal/async_context_frame:56
    this.set(store, data);
         ^

TypeError: Method Map.prototype.set called on incompatible receiver #<AsyncContextFrame>
    at AsyncContextFrame.set (<anonymous>)
    at new AsyncContextFrame (node:internal/async_context_frame:56:10)
    at AsyncLocalStorage.enterWith (node:internal/async_local_storage/async_context_frame:24:19)
    at AsyncLocalStorage.run (node:internal/async_local_storage/async_context_frame:30:10)
    at [eval]:1:37
    at runScriptInThisContext (node:internal/vm:209:10)
    at node:internal/process/execution:118:14
    at [eval]-wrapper:6:24
    at runScript (node:internal/process/execution:101:62)
    at evalScript (node:internal/process/execution:136:3)

Node.js v22.7.0

sendoru pushed a commit to sendoru/node that referenced this issue Sep 1, 2024
This gives a class prototype for AsyncContextFrame that contains the
required methods, so that when we swap the prototype,
ActiveAsyncContextFrame methods are used instead. Previously, the
methods were defined in AsyncContextFrame, so swapping the prototype
didn't swap those static methods.

Also, make the ActiveAsyncContextFrame extend from Map.

Fixes: nodejs#54503
PR-URL: nodejs#54510
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
aduh95 pushed a commit that referenced this issue Sep 12, 2024
This gives a class prototype for AsyncContextFrame that contains the
required methods, so that when we swap the prototype,
ActiveAsyncContextFrame methods are used instead. Previously, the
methods were defined in AsyncContextFrame, so swapping the prototype
didn't swap those static methods.

Also, make the ActiveAsyncContextFrame extend from Map.

Fixes: #54503
PR-URL: #54510
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
This gives a class prototype for AsyncContextFrame that contains the
required methods, so that when we swap the prototype,
ActiveAsyncContextFrame methods are used instead. Previously, the
methods were defined in AsyncContextFrame, so swapping the prototype
didn't swap those static methods.

Also, make the ActiveAsyncContextFrame extend from Map.

Fixes: nodejs#54503
PR-URL: nodejs#54510
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
tpoisseau pushed a commit to tpoisseau/node that referenced this issue Nov 21, 2024
This gives a class prototype for AsyncContextFrame that contains the
required methods, so that when we swap the prototype,
ActiveAsyncContextFrame methods are used instead. Previously, the
methods were defined in AsyncContextFrame, so swapping the prototype
didn't swap those static methods.

Also, make the ActiveAsyncContextFrame extend from Map.

Fixes: nodejs#54503
PR-URL: nodejs#54510
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Gerhard Stöbich <[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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants