FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

worker: avoid potential deadlock on NearHeapLimit by santigimeno · Pull Request #38403 · nodejs/node · GitHub

/ node Public

worker: avoid potential deadlock on NearHeapLimit - #38403

Closed
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:santi/fix_38208
ConversationCommits (1)ChecksFiles changed
Closed

worker: avoid potential deadlock on NearHeapLimit#38403
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:santi/fix_38208

Conversation

santigimeno commented Apr 25, 2021

Copy link
Copy Markdown
Member

It can happen that the NearHeapLimit callback is called while calling
the oninit() function on MessagePort construction causing a deadlock
when the Worker::Exit() method is called, as the mutex_ was already
held on the CreateEnvMessagePort() method. To fix it, just use the
mutex_ to protect the child_port_data_ variable and avoid holding it
when creating the MessagePort.
Also, return early from Worker::Run() if the worker message port
could not be created.

Fixes: #38208.

A couple of things I'd like to point out:

  • I've only been able to reproduce the issue from the original report in the v12.x branch, but I think that potentially it could happen on newer branches too.
  • I've implemented this solution on the assumption that the mutex is only needed to protect child_port_data_ based on this comment, if that's not the case, it won't work. Another option I've tried that also works was using a recursive mutex, but I don't know if that solution would be acceptable.

santigimeno self-assigned this Apr 25, 2021
github-actions Bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels Apr 25, 2021
jasnell requested a review from joyeecheung April 25, 2021 21:06

This comment has been minimized.

Sign in to view

Trott commented Apr 29, 2021

Copy link
Copy Markdown
Member

Test failure on Windows is relevant.

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

santigimeno commented May 7, 2021

Copy link
Copy Markdown
Member Author

I think this is ready to go now, just in case anyone wants to check again. I have changed the test a bit so it passes on every platform while also keeps deadlocking in v12.x. Thanks

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view
worker: avoid potential deadlock on NearHeapLimit
28414cb 
It can happen that the `NearHeapLimit` callback is called while calling
the `oninit()` function on `MessagePort` construction causing a deadlock
when the `Worker::Exit()` method is called, as the `mutex_` was already
held on the `CreateEnvMessagePort()` method. To fix it, just use the
`mutex_` to protect the `child_port_data_` variable and avoid holding it
when creating the `MessagePort`.
Also, return early from `Worker::Run()` if the worker message port
could not be created.

Fixes: nodejs#38208

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Sep 10, 2021

Copy link
Copy Markdown
Collaborator

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

juanarbol added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 10, 2021
juanarbol pushed a commit to juanarbol/node that referenced this pull request Sep 11, 2021
worker: avoid potential deadlock on NearHeapLimit
540f9d9 
It can happen that the `NearHeapLimit` callback is called while calling
the `oninit()` function on `MessagePort` construction causing a deadlock
when the `Worker::Exit()` method is called, as the `mutex_` was already
held on the `CreateEnvMessagePort()` method. To fix it, just use the
`mutex_` to protect the `child_port_data_` variable and avoid holding it
when creating the `MessagePort`.
Also, return early from `Worker::Run()` if the worker message port
could not be created.

Fixes: nodejs#38208

PR-URL: nodejs#38403
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>

juanarbol commented Sep 11, 2021

Copy link
Copy Markdown
Member

Landed in 540f9d9 🎉

juanarbol closed this Sep 11, 2021
This was referenced Sep 14, 2021
Open
Open
Open
Open
Open
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
worker: avoid potential deadlock on NearHeapLimit
a562016 
It can happen that the `NearHeapLimit` callback is called while calling
the `oninit()` function on `MessagePort` construction causing a deadlock
when the `Worker::Exit()` method is called, as the `mutex_` was already
held on the `CreateEnvMessagePort()` method. To fix it, just use the
`mutex_` to protect the `child_port_data_` variable and avoid holding it
when creating the `MessagePort`.
Also, return early from `Worker::Run()` if the worker message port
could not be created.

Fixes: #38208

PR-URL: #38403
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
worker: avoid potential deadlock on NearHeapLimit
5983568 
It can happen that the `NearHeapLimit` callback is called while calling
the `oninit()` function on `MessagePort` construction causing a deadlock
when the `Worker::Exit()` method is called, as the `mutex_` was already
held on the `CreateEnvMessagePort()` method. To fix it, just use the
`mutex_` to protect the `child_port_data_` variable and avoid holding it
when creating the `MessagePort`.
Also, return early from `Worker::Run()` if the worker message port
could not be created.

Fixes: #38208

PR-URL: #38403
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
BethGriggs mentioned this pull request Sep 21, 2021
Merged
1 task
richardlau pushed a commit that referenced this pull request Nov 26, 2021
worker: avoid potential deadlock on NearHeapLimit
39583f7 
It can happen that the `NearHeapLimit` callback is called while calling
the `oninit()` function on `MessagePort` construction causing a deadlock
when the `Worker::Exit()` method is called, as the `mutex_` was already
held on the `CreateEnvMessagePort()` method. To fix it, just use the
`mutex_` to protect the `child_port_data_` variable and avoid holding it
when creating the `MessagePort`.
Also, return early from `Worker::Run()` if the worker message port
could not be created.

Fixes: #38208

PR-URL: #38403
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
richardlau mentioned this pull request Nov 26, 2021
Merged
richardlau pushed a commit that referenced this pull request Dec 14, 2021
worker: avoid potential deadlock on NearHeapLimit
f50b9c1 
It can happen that the `NearHeapLimit` callback is called while calling
the `oninit()` function on `MessagePort` construction causing a deadlock
when the `Worker::Exit()` method is called, as the `mutex_` was already
held on the `CreateEnvMessagePort()` method. To fix it, just use the
`mutex_` to protect the `child_port_data_` variable and avoid holding it
when creating the `MessagePort`.
Also, return early from `Worker::Run()` if the worker message port
could not be created.

Fixes: #38208

PR-URL: #38403
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
richardlau mentioned this pull request Dec 15, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
jasnell jasnell approved these changes
addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
joyeecheung joyeecheung approved these changes
juanarbol juanarbol approved these changes

Assignees

santigimeno

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

worker_threads.terminate results in no response on v12.x

9 participants

Back | FazBrowse Home | New Git URL