New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: wrap HostPort in ExclusiveAccess #31717

Closed

bnoordhuis wants to merge 2 commits into nodejs:master from bnoordhuis:exclusive-access

Member

bnoordhuis commented Feb 10, 2020

I found it exceedingly hard to figure out if there is a race condition
where one thread reads the inspector agent's HostPort's properties while
another modifies them concurrently.

I think the answer is "no, there isn't" but with this commit use sites
are forced to unwrap the object (and acquire the mutex in the process),
making it a great deal easier to reason about correctness.

(The first commit introduces said ExclusiveAccess class.)

nodejs-github-bot added c++ lib / src labels

Collaborator

nodejs-github-bot commented Feb 10, 2020

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

jasnell approved these changes

View reviewed changes

cjihrig approved these changes

View reviewed changes

addaleax approved these changes

View reviewed changes

src/node_mutex.h Outdated Show resolved Hide resolved

tniessen approved these changes

View reviewed changes

devnexen approved these changes

View reviewed changes

bnoordhuis added 2 commits

February 13, 2020 16:36


          src: add ExclusiveAccess class

8a362d0

Similar to Rust's `std::sync::Mutex` in that it requires one to unwrap
the object before accessing it, acquiring the mutex in the process.


          src: wrap HostPort in ExclusiveAccess

18b5694

I found it exceedingly hard to figure out if there is a race condition
where one thread reads the inspector agent's HostPort's properties while
another modifies them concurrently.

I think the answer is "no, there isn't" but with this commit use sites
are forced to unwrap the object (and acquire the mutex in the process),
making it a great deal easier to reason about correctness.

bnoordhuis force-pushed the exclusive-access branch from 24ef127 to 18b5694 Compare

February 13, 2020 15:37

Collaborator

nodejs-github-bot commented Feb 13, 2020

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

addaleax added the author ready label

Collaborator

nodejs-github-bot commented Feb 13, 2020

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

Member

addaleax commented Feb 13, 2020

Landed in d63bcdd...303e2fd

addaleax closed this

addaleax pushed a commit that referenced this pull request


          src: add ExclusiveAccess class

b32fa7b

Similar to Rust's `std::sync::Mutex` in that it requires one to unwrap
the object before accessing it, acquiring the mutex in the process.

PR-URL: #31717
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: David Carlier <[email protected]>

addaleax pushed a commit that referenced this pull request


          src: wrap HostPort in ExclusiveAccess

303e2fd

I found it exceedingly hard to figure out if there is a race condition
where one thread reads the inspector agent's HostPort's properties while
another modifies them concurrently.

I think the answer is "no, there isn't" but with this commit use sites
are forced to unwrap the object (and acquire the mutex in the process),
making it a great deal easier to reason about correctness.

PR-URL: #31717
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: David Carlier <[email protected]>

codebytere pushed a commit that referenced this pull request


          src: add ExclusiveAccess class

e23023d

Similar to Rust's `std::sync::Mutex` in that it requires one to unwrap
the object before accessing it, acquiring the mutex in the process.

PR-URL: #31717
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: David Carlier <[email protected]>

codebytere pushed a commit that referenced this pull request


          src: wrap HostPort in ExclusiveAccess

1dec9d1

I found it exceedingly hard to figure out if there is a race condition
where one thread reads the inspector agent's HostPort's properties while
another modifies them concurrently.

I think the answer is "no, there isn't" but with this commit use sites
are forced to unwrap the object (and acquire the mutex in the process),
making it a great deal easier to reason about correctness.

PR-URL: #31717
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: David Carlier <[email protected]>

codebytere mentioned this pull request

v13.9.0 proposal #31837

Merged

codebytere pushed a commit that referenced this pull request


          src: add ExclusiveAccess class

7d2fe7d

Similar to Rust's `std::sync::Mutex` in that it requires one to unwrap
the object before accessing it, acquiring the mutex in the process.

PR-URL: #31717
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: David Carlier <[email protected]>

codebytere pushed a commit that referenced this pull request


          src: wrap HostPort in ExclusiveAccess

a686550

I found it exceedingly hard to figure out if there is a race condition
where one thread reads the inspector agent's HostPort's properties while
another modifies them concurrently.

I think the answer is "no, there isn't" but with this commit use sites
are forced to unwrap the object (and acquire the mutex in the process),
making it a great deal easier to reason about correctness.

PR-URL: #31717
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: David Carlier <[email protected]>

codebytere pushed a commit that referenced this pull request


          src: add ExclusiveAccess class

182b06c

Similar to Rust's `std::sync::Mutex` in that it requires one to unwrap
the object before accessing it, acquiring the mutex in the process.

PR-URL: #31717
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: David Carlier <[email protected]>

codebytere pushed a commit that referenced this pull request


          src: wrap HostPort in ExclusiveAccess

76edc7c

I found it exceedingly hard to figure out if there is a race condition
where one thread reads the inspector agent's HostPort's properties while
another modifies them concurrently.

I think the answer is "no, there isn't" but with this commit use sites
are forced to unwrap the object (and acquire the mutex in the process),
making it a great deal easier to reason about correctness.

PR-URL: #31717
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: David Carlier <[email protected]>

codebytere mentioned this pull request

v12.16.2 proposal #32313

Merged

codebytere pushed a commit that referenced this pull request


          src: add ExclusiveAccess class

01da656

Similar to Rust's `std::sync::Mutex` in that it requires one to unwrap
the object before accessing it, acquiring the mutex in the process.

PR-URL: #31717
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: David Carlier <[email protected]>

codebytere pushed a commit that referenced this pull request


          src: wrap HostPort in ExclusiveAccess

I found it exceedingly hard to figure out if there is a race condition
where one thread reads the inspector agent's HostPort's properties while
another modifies them concurrently.

I think the answer is "no, there isn't" but with this commit use sites
are forced to unwrap the object (and acquire the mutex in the process),
making it a great deal easier to reason about correctness.

PR-URL: #31717
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: David Carlier <[email protected]>

addaleax mentioned this pull request

report: add missing locks for report_on_fatalerror accessors #32535

Closed

2 tasks

codebytere added a commit to electron/electron that referenced this pull request


          Wrap HostPort with ExclusiveAccess

55818fd

nodejs/node#31717

codebytere added a commit to electron/electron that referenced this pull request


          Wrap HostPort with ExclusiveAccess

6ab649f

nodejs/node#31717

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes

addaleax addaleax approved these changes

cjihrig cjihrig approved these changes

tniessen tniessen approved these changes

devnexen devnexen approved these changes

Labels

author ready c++ lib / src