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

worker_threads.moveMessagePortToContext results in an abort #38499

Closed
zyscoder opened this issue May 1, 2021 · 1 comment
Closed

worker_threads.moveMessagePortToContext results in an abort #38499

zyscoder opened this issue May 1, 2021 · 1 comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. confirmed-bug Issues with confirmed bugs. good first issue Issues that are suitable for first-time contributors. worker Issues and PRs related to Worker support.

Comments

@zyscoder
Copy link

zyscoder commented May 1, 2021

What steps will reproduce the bug?

Setup a node instance,

» node

and run the following javascript code line by line.

worker_threads = require('worker_threads');msgChannel = new worker_threads.MessageChannel();
msgChannel.port2.close();
worker_threads.moveMessagePortToContext(msgChannel.port2,{});

Then an abort occurs.
Noticed that paste these codes to REPL all at once seems cannot trigger this abort. Run these codes line by line pls.

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

This problem can always be triggered following the steps above.

What is the expected behavior?

If any error occurs, an exception or other similar error-reporting stuff should be thrown. There is no reason to abort the whole node process.

What do you see instead?

» node
Welcome to Node.js v17.0.0-pre.
Type ".help" for more information.
> worker_threads = require('worker_threads');msgChannel = new worker_threads.MessageChannel();
MessageChannel {
  port1: MessagePort [EventTarget] {
    active: true,
    refed: false,
    [Symbol(kEvents)]: SafeMap(2) [Map] {
      'newListener' => [Object],
      'removeListener' => [Object]
    },
    [Symbol(events.maxEventTargetListeners)]: 10,
    [Symbol(events.maxEventTargetListenersWarned)]: false,
    [Symbol(kNewListener)]: [Function (anonymous)],
    [Symbol(kRemoveListener)]: [Function (anonymous)],
    [Symbol(nodejs.internal.kCurrentlyReceivingPorts)]: undefined
  },
  port2: MessagePort [EventTarget] {
    active: true,
    refed: false,
    [Symbol(kEvents)]: SafeMap(2) [Map] {
      'newListener' => [Object],
      'removeListener' => [Object]
    },
    [Symbol(events.maxEventTargetListeners)]: 10,
    [Symbol(events.maxEventTargetListenersWarned)]: false,
    [Symbol(kNewListener)]: [Function (anonymous)],
    [Symbol(kRemoveListener)]: [Function (anonymous)],
    [Symbol(nodejs.internal.kCurrentlyReceivingPorts)]: undefined
  }
}
> msgChannel.port2.close();
undefined
> worker_threads.moveMessagePortToContext(msgChannel.port2,{});
/home/zys/Toolchains/node/node[59534]: ../src/node_messaging.cc:1068:static void node::worker::MessagePort::MoveToContext(const FunctionCallbackInfo<v8::Value> &): Assertion `(port) != nullptr' failed.
 1: 0x3623e04 node::DumpBacktrace(_IO_FILE*) [/home/zys/Toolchains/node/node]
 2: 0x37bb92e node::Abort() [/home/zys/Toolchains/node/node]
 3: 0x37bb3f8  [/home/zys/Toolchains/node/node]
 4: 0x38c8dac node::worker::MessagePort::MoveToContext(v8::FunctionCallbackInfo<v8::Value> const&) [/home/zys/Toolchains/node/node]
 5: 0x3ebd0d8 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [/home/zys/Toolchains/node/node]
 6: 0x3ebadb9  [/home/zys/Toolchains/node/node]
 7: 0x3eb8d1d  [/home/zys/Toolchains/node/node]
 8: 0x5a08759  [/home/zys/Toolchains/node/node]
[1]    59534 abort (core dumped)  /home/zys/Toolchains/node/node

Additional information

@addaleax addaleax added worker Issues and PRs related to Worker support. c++ Issues and PRs that require attention from people who are familiar with C++. confirmed-bug Issues with confirmed bugs. good first issue Issues that are suitable for first-time contributors. labels May 1, 2021
@addaleax
Copy link
Member

addaleax commented May 1, 2021

This is the invalid CHECK:

CHECK_NOT_NULL(port);

Noticed that paste these codes to REPL all at once seems cannot trigger this abort. Run these codes line by line pls.

Fwiw, this is happening because MessagePorts, like all libuv handles, close asynchronously. Wrapping the crashing call in a setImmediate() or similar should make this consistently reproducible.

@jasnell jasnell closed this as completed in b373a2c May 6, 2021
jasnell pushed a commit that referenced this issue May 6, 2021
PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue May 17, 2021
This reverts commit 73370b4.

The unit test is preserved to make sure it does not break
#26463 again.

PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue May 17, 2021
PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue May 30, 2021
This reverts commit 73370b4.

The unit test is preserved to make sure it does not break
#26463 again.

PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue May 30, 2021
PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jun 5, 2021
This reverts commit 73370b4.

The unit test is preserved to make sure it does not break
#26463 again.

PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jun 5, 2021
PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jun 5, 2021
This reverts commit 73370b4.

The unit test is preserved to make sure it does not break
#26463 again.

PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jun 5, 2021
PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jun 11, 2021
This reverts commit 73370b4.

The unit test is preserved to make sure it does not break
#26463 again.

PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jun 11, 2021
PR-URL: #38510
Fixes: #38499
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[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
c++ Issues and PRs that require attention from people who are familiar with C++. confirmed-bug Issues with confirmed bugs. good first issue Issues that are suitable for first-time contributors. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants