GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: use RAII for mutexes and condition variables by bnoordhuis · Pull Request #7334 · nodejs/node

Conversation

@bnoordhuis

@bnoordhuis bnoordhuis commented Jun 18, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fishrock123 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bnoordhuis bnoordhuis added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Jun 18, 2016
bnoordhuis
bnoordhuis reviewed Jun 18, 2016
View reviewed changes
Comment thread src/inspector_agent.cc Outdated

@bnoordhuis bnoordhuis Jun 18, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@addaleax

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis
bnoordhuis force-pushed the raii-lock branch from bed77d5 to 5546140 Compare June 18, 2016 19:38
@bnoordhuis

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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis

bnoordhuis commented Jun 19, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Jun 20, 2016

Copy link
Copy Markdown
Member

nice. LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Jun 20, 2016

Copy link
Copy Markdown
Contributor

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
trevnorris
trevnorris reviewed Jun 20, 2016
View reviewed changes
Comment thread src/node_mutex.h Outdated

@trevnorris trevnorris Jun 20, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bnoordhuis bnoordhuis Jun 20, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@trevnorris

trevnorris commented Jun 20, 2016

Copy link
Copy Markdown
Contributor

Thanks for answering the questions. Makes sense. LGTM.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis bnoordhuis closed this Jun 21, 2016
@bnoordhuis
bnoordhuis force-pushed the raii-lock branch from 5546140 to 781713d Compare June 21, 2016 08:31
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jun 21, 2016
@bnoordhuis
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jun 21, 2016
@bnoordhuis
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@bnoordhuis
bnoordhuis deleted the raii-lock branch June 21, 2016 08:31
@bnoordhuis
bnoordhuis merged commit 781713d into nodejs:master Jun 21, 2016
evanlucas pushed a commit that referenced this pull request Jun 27, 2016
@bnoordhuis @evanlucas
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@Fishrock123 Fishrock123 added the dont-land-on-v6.x label Jun 27, 2016
@Fishrock123

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123 Fishrock123 removed the dont-land-on-v6.x label Jul 5, 2016
@Fishrock123

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
@bnoordhuis @Fishrock123
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
Propose v6.3.0 (v2) #7550
Merged
@MylesBorins MylesBorins mentioned this pull request Jul 11, 2016
Audit commits not found on v4.x-staging #7661
Closed
@MylesBorins MylesBorins added the lts-watch-v4.x label Jul 11, 2016
@MylesBorins

MylesBorins commented Jul 11, 2016

Copy link
Copy Markdown
Contributor

@bnoordhuis should this be backported?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis

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.
addaleax reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis

bnoordhuis commented Jul 13, 2016

Copy link
Copy Markdown
Member Author

#7715 - v4.x back-port
MylesBorins reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Jul 13, 2016
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
v6.4.0 Release Proposal #8020
Closed
@cjihrig

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Sep 5, 2016
@bnoordhuis
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@bnoordhuis

bnoordhuis commented Sep 5, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123 Fishrock123 added the dont-land-on-v6.x label Sep 6, 2016
MylesBorins pushed a commit that referenced this pull request Sep 7, 2016
@bnoordhuis Myles Borins
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2016
@bnoordhuis Myles Borins
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
@bnoordhuis @rvagg
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
@bnoordhuis @MylesBorins
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
V4.6.2 proposal #9298
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Auditing for 6.11.1 nodejs/Release#230
Closed
2 of 3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@bnoordhuis @addaleax @jasnell @cjihrig @trevnorris @Fishrock123 @MylesBorins
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.