Skip to content

worker: avoid potential deadlock on NearHeapLimit - #38403

Closed
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:santi/fix_38208
Closed

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

Conversation

@santigimeno

@santigimenosantigimeno 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.

@santigimenosantigimeno self-assigned this Apr 25, 2021
@github-actionsgithub-actionsBot 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
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Trott
Trott reviewed Apr 29, 2021
View reviewed changes
Comment threadtest/parallel/test-worker-nearheaplimit-deadlock.js Outdated
@Trott

Trott commented Apr 29, 2021

Copy link
Copy Markdown
Member

Test failure on Windows is relevant.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@santigimeno

santigimeno commented May 7, 2021

Copy link
Copy Markdown
MemberAuthor

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

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@santigimeno
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
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Sep 10, 2021

Copy link
Copy Markdown
Collaborator

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

@juanarboljuanarbol added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 10, 2021
juanarbol pushed a commit to juanarbol/node that referenced this pull request Sep 11, 2021
@santigimeno@juanarbol
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

juanarbol commented Sep 11, 2021

Copy link
Copy Markdown
Member

Landed in 540f9d9 🎉

This was referenced Sep 14, 2021
Open
Open
Open
Open
Open
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
@santigimeno@BethGriggs
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
@santigimeno@BethGriggs
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>
@BethGriggsBethGriggs mentioned this pull request Sep 21, 2021
Merged
1 task
richardlau pushed a commit that referenced this pull request Nov 26, 2021
@santigimeno@richardlau
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>
@richardlaurichardlau mentioned this pull request Nov 26, 2021
Merged
richardlau pushed a commit that referenced this pull request Dec 14, 2021
@santigimeno@richardlau
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>
@richardlaurichardlau mentioned this pull request Dec 15, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@cjihrigcjihrigcjihrig approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes
@juanarboljuanarboljuanarbol approved these changes

Assignees

@santigimenosantigimeno

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.workerIssues 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

@santigimeno@nodejs-github-bot@Trott@juanarbol@jasnell@addaleax@cjihrig@joyeecheung@richardlau