worker: avoid potential deadlock on NearHeapLimit#38403
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
github-actionsbot added c++ 
addaleax approved these changes Apr 25, 2021
This comment has been minimized.
This comment has been minimized.
jasnell approved these changes Apr 26, 2021
cjihrig approved these changes Apr 27, 2021
joyeecheung approved these changes Apr 27, 2021
Trott approved these changes Apr 29, 2021
Trott reviewed Apr 29, 2021
Uh oh!
There was an error while loading. Please reload this page.
Member
Trott commented Apr 29, 2021
Test failure on Windows is relevant. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
MemberAuthor
santigimeno commented May 7, 2021
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 |
santigimenoforce-pushed the santi/fix_38208 branch from 750d62b to 6142adcCompare This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
santigimenoforce-pushed the santi/fix_38208 branch from 761bb60 to 73ac434Compare This comment has been minimized.
This comment has been minimized.
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
santigimenoforce-pushed the santi/fix_38208 branch from 73ac434 to 28414cbCompare This comment has been minimized.
This comment has been minimized.
Collaborator
nodejs-github-bot commented Sep 10, 2021
juanarbol approved these changes Sep 10, 2021
juanarbol added the author ready juanarbol pushed a commit to juanarbol/node that referenced this pull request Sep 11, 2021
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
Member
juanarbol commented Sep 11, 2021
Landed in 540f9d9 🎉 |
This was referenced Sep 14, 2021
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
BethGriggs pushed a commit that referenced this pull request Sep 21, 2021
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
richardlau pushed a commit that referenced this pull request Nov 26, 2021
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
Merged
richardlau pushed a commit that referenced this pull request Dec 14, 2021
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It can happen that the
NearHeapLimitcallback is called while callingthe
oninit()function onMessagePortconstruction causing a deadlockwhen the
Worker::Exit()method is called, as themutex_was alreadyheld on the
CreateEnvMessagePort()method. To fix it, just use themutex_to protect thechild_port_data_variable and avoid holding itwhen creating the
MessagePort.Also, return early from
Worker::Run()if the worker message portcould not be created.
Fixes: #38208.
A couple of things I'd like to point out:
v12.xbranch, but I think that potentially it could happen on newer branches too.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.