Skip to content
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: avoid race condition in tracing code #25624

Closed
wants to merge 2 commits into from
Closed

src: avoid race condition in tracing code #25624

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

json_trace_writer_ is protected by stream_mutex_,
but one access to it was not guarded by a lock on said mutex.

Refs: #25512

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@addaleax addaleax added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Jan 21, 2019
@nodejs-github-bot
Copy link
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2331/pipeline

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 21, 2019
@addaleax addaleax mentioned this pull request Jan 21, 2019
Closed
@addaleax
Copy link
Member Author

Stress test CI for parallel/test-trace-events-fs-sync with this PR: https://ci.nodejs.org/job/node-stress-single-test/2142/
Regular CI: https://ci.nodejs.org/job/node-test-pull-request/20250/

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 21, 2019
bnoordhuis
bnoordhuis reviewed Jan 22, 2019
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Aside: is the call to WriteFile() on line 136 sound? FlushPrivate() reads num_write_requests_ while holding the lock, then passes that value to WriteFile() after dropping the lock. It looks like you could observe duplicate request ids.

src/tracing/node_trace_writer.cc
if (!json_trace_writer_) {
return;
{
Mutex::ScopedLock stream_mutex_lock(stream_mutex_);
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to nest locks?1 And if so, can you perhaps add a comment explaining why? It's currently kind of hard to eyeball whether this is correct or a deadlock waiting to happen.

It would be nice to have an abstraction that enforces that locks are only taken out in a specific order so you don't run into AB-BA deadlocks.

1 I think the answer is 'yes' because otherwise there's a race window.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it necessary to nest locks?1

I think so, yes, for the reason you mentioned. But I am not a hundred percent sure either. I have a hard time figuring out this code myself as well.

And if so, can you perhaps add a comment explaining why? It's currently kind of hard to eyeball whether this is correct or a deadlock waiting to happen.

Added a comment; I did check that the locks are never required in another order, so this shouldn’t be making anything worse…

@addaleax
src: avoid race condition in tracing code
9d65ad3 
`json_trace_writer_` is protected by `stream_mutex_`,
but one access to it was not guarded by a lock on said mutex.

Refs: nodejs#25512
@addaleax
fixup! src: avoid race condition in tracing code
acabd3e
@addaleax
Copy link
Member Author

is the call to WriteFile() on line 136 sound? FlushPrivate() reads num_write_requests_ while holding the lock, then passes that value to WriteFile() after dropping the lock. It looks like you could observe duplicate request ids.

I can’t say for sure. /cc @nodejs/trace-events

@addaleax
Copy link
Member Author

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

@addaleax
Copy link
Member Author

Landed in 06da364

@addaleax addaleax closed this Jan 27, 2019
@addaleax addaleax deleted the tracing-race branch January 27, 2019 19:44
addaleax added a commit that referenced this pull request Jan 27, 2019
@addaleax
src: avoid race condition in tracing code
06da364 
`json_trace_writer_` is protected by `stream_mutex_`,
but one access to it was not guarded by a lock on said mutex.

Refs: #25512

PR-URL: #25624
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
addaleax added a commit that referenced this pull request Jan 28, 2019
@addaleax
src: avoid race condition in tracing code
797111a 
`json_trace_writer_` is protected by `stream_mutex_`,
but one access to it was not guarded by a lock on said mutex.

Refs: #25512

PR-URL: #25624
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@targos targos mentioned this pull request Jan 29, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 16, 2019
@addaleax @BethGriggs
src: avoid race condition in tracing code
5cd96b3 
`json_trace_writer_` is protected by `stream_mutex_`,
but one access to it was not guarded by a lock on said mutex.

Refs: #25512

PR-URL: #25624
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@lundibundi lundibundi lundibundi approved these changes

@richardlau richardlau richardlau approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@addaleax @nodejs-github-bot @bnoordhuis @cjihrig @richardlau @lundibundi