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

src: use RAII for mutexes and condition variables by bnoordhuis · Pull Request #7334 · nodejs/node · GitHub

/ node Public

src: use RAII for mutexes and condition variables - #7334

Merged
bnoordhuis merged 0 commit into
nodejs:masterfrom
bnoordhuis:raii-lock
Jun 21, 2016
ConversationCommits (0)ChecksFiles changed
Merged

src: use RAII for mutexes and condition variables#7334
bnoordhuis merged 0 commit into
nodejs:masterfrom
bnoordhuis:raii-lock

Conversation

bnoordhuis commented Jun 18, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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/

bnoordhuis added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jun 18, 2016
bnoordhuis reviewed Jun 18, 2016
View reviewed changes
Comment thread src/inspector_agent.cc Outdated

bnoordhuis Jun 18, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

Green except for flaky test parallel/test-vm-timeout, tracked in #6727.

jasnell commented Jun 20, 2016

Copy link
Copy Markdown
Member

nice. LGTM

cjihrig commented Jun 20, 2016

Copy link
Copy Markdown
Contributor

LGTM

trevnorris reviewed Jun 20, 2016
View reviewed changes
Comment thread src/node_mutex.h Outdated

trevnorris Jun 20, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

reason for adding the argument name scoped_lock in Wait() but not in Broadcast() or Signal()?

bnoordhuis Jun 20, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor

Thanks for answering the questions. Makes sense. LGTM.

bnoordhuis closed this Jun 21, 2016
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jun 21, 2016
tools: disable readability/function cpplint rule
bb33c28 
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jun 21, 2016
src: use RAII for mutexes and condition variables
d7087df 
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
bnoordhuis deleted the raii-lock branch June 21, 2016 08:31
bnoordhuis merged commit 781713d into nodejs:master Jun 21, 2016
evanlucas pushed a commit that referenced this pull request Jun 27, 2016
tools: disable readability/function cpplint rule
bd8c951 
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Fishrock123 commented Jun 27, 2016

Copy link
Copy Markdown
Contributor

This touches the inspector. Marking as dont-land-on-v6. (for now, idk until we resolve that)

Fishrock123 commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

@bnoordhuis 781713d 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!

Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
src: use RAII for mutexes and condition variables
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Fishrock123 mentioned this pull request Jul 5, 2016
Merged
MylesBorins mentioned this pull request Jul 11, 2016
Closed

MylesBorins commented Jul 11, 2016

Copy link
Copy Markdown
Contributor

@bnoordhuis should this be backported?

bnoordhuis commented Jul 12, 2016

Copy link
Copy Markdown
Member Author

@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

Copy link
Copy Markdown
Member Author

#7715 - v4.x back-port

cjihrig mentioned this pull request Aug 8, 2016
Closed

cjihrig commented Aug 10, 2016

Copy link
Copy Markdown
Contributor

@bnoordhuis this doesn't seem to apply cleanly to v6.x either, and #7715 has 213 commits. Could you backport this?

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Sep 5, 2016
src: use RAII for mutexes and condition variables
ab9e3aa 
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

bnoordhuis commented Sep 5, 2016

Copy link
Copy Markdown
Member Author

For posterity, this landed in v6.x in commit 0593351.

MylesBorins pushed a commit that referenced this pull request Sep 7, 2016
src: use RAII for mutexes and condition variables
efc77ec 
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2016
src: use RAII for mutexes and condition variables
401b4ff 
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
src: use RAII for mutexes and condition variables
32dd2f5 
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
src: use RAII for mutexes and condition variables
a5b6c2c 
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
MylesBorins mentioned this pull request Oct 26, 2016
Closed
gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
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

No reviews

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL