src: use RAII for mutexes and condition variables#7334
Uh oh!
There was an error while loading. Please reload this page.
Conversation
bnoordhuis added c++ 
src/inspector_agent.cc Outdated
There was a problem hiding this comment.
@pavelfeldman Can you comment on whether the changes to this function look correct to you? ISTM it should be holding the lock when signalling the condition variable.
addaleax commented Jun 18, 2016
@bnoordhuis The next time you rebase this against master, could you include addaleax/node@d16e607? Sorry it took me so long to get #6635 landed. |
bnoordhuis commented Jun 18, 2016
Rebased, with workaround for compilers that don't understand constexpr function pointers: https://ci.nodejs.org/job/node-test-pull-request/3024/ |
bnoordhuis commented Jun 19, 2016
Green except for flaky test parallel/test-vm-timeout, tracked in #6727. |
jasnell commented Jun 20, 2016
nice. LGTM |
cjihrig commented Jun 20, 2016
LGTM |
src/node_mutex.h Outdated
There was a problem hiding this comment.
reason for adding the argument name scoped_lock in Wait() but not in Broadcast() or Signal()?
There was a problem hiding this comment.
Wait() uses it, the other two just take it to prove ownership (i.e., you can't signal or broadcast if you don't hold the lock.)
trevnorris commented Jun 20, 2016
Thanks for answering the questions. Makes sense. LGTM. |
cpplint gets too easily confused by C++ constructs that look like function declarations but aren't. Furthermore, it's arguably a bad rule that conflicts with gcc's -Wunused-parameter flag. PR-URL: nodejs#7334 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
We will be introducing many more critical sections in the upcoming multi-isolate changes, so let's make manual synchronization a thing of the past. PR-URL: nodejs#7334 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
cpplint gets too easily confused by C++ constructs that look like function declarations but aren't. Furthermore, it's arguably a bad rule that conflicts with gcc's -Wunused-parameter flag. PR-URL: #7334 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
Fishrock123 commented Jun 27, 2016
This touches the inspector. Marking as |
Fishrock123 commented Jul 5, 2016
@bnoordhuis781713d applies poorly on v6 but as far as I can tell the other commit is required to not have more conflicts on the inspector... If possible, separating out non-backportable work would be greatly appreciated by us releasers! |
We will be introducing many more critical sections in the upcoming multi-isolate changes, so let's make manual synchronization a thing of the past. PR-URL: #7334 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
MylesBorins commented Jul 11, 2016
@bnoordhuis should this be backported? |
bnoordhuis commented Jul 12, 2016
@thealphanerd It's not necessary per se but it would be good. You'll have to drop the changes to src/inspector_agent.cc. |
bnoordhuis commented Jul 13, 2016
#7715 - v4.x back-port |
cjihrig commented Aug 10, 2016
@bnoordhuis this doesn't seem to apply cleanly to v6.x either, and #7715 has 213 commits. Could you backport this? |
We will be introducing many more critical sections in the upcoming multi-isolate changes, so let's make manual synchronization a thing of the past. PR-URL: nodejs#7334 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
bnoordhuis commented Sep 5, 2016
For posterity, this landed in v6.x in commit 0593351. |
We will be introducing many more critical sections in the upcoming multi-isolate changes, so let's make manual synchronization a thing of the past. PR-URL: #7334 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
We will be introducing many more critical sections in the upcoming multi-isolate changes, so let's make manual synchronization a thing of the past. PR-URL: #7334 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
We will be introducing many more critical sections in the upcoming multi-isolate changes, so let's make manual synchronization a thing of the past. PR-URL: #7334 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
We will be introducing many more critical sections in the upcoming multi-isolate changes, so let's make manual synchronization a thing of the past. PR-URL: #7334 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
7 participants
We will be introducing many more critical sections in the upcoming
multi-isolate changes, so let's make manual synchronization a thing
of the past.
CI: https://ci.nodejs.org/job/node-test-pull-request/3022/