Skip to content

src: avoid race condition in tracing code #25624

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

Closed
addaleax wants to merge 2 commits into from
Closed

src: avoid race condition in tracing code #25624

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d65ad3
src: avoid race condition in tracing code
addaleax Jan 21, 2019
acabd3e
fixup! src: avoid race condition in tracing code
addaleax Jan 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
src: avoid race condition in tracing code 
`json_trace_writer_` is protected by `stream_mutex_`,
but one access to it was not guarded by a lock on said mutex.

Refs: #25512
  • Loading branch information
@addaleax
addaleax committed Jan 27, 2019
commit 9d65ad3ef4884cb888fd83fabdcc2276554742e3
6 changes: 4 additions & 2 deletions src/tracing/node_trace_writer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,10 @@ void NodeTraceWriter::FlushPrivate() {

void NodeTraceWriter::Flush(bool blocking) {
Mutex::ScopedLock scoped_lock(request_mutex_);
if (!json_trace_writer_) {
return;
{
Mutex::ScopedLock stream_mutex_lock(stream_mutex_);
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Jan 22, 2019

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
Copy Markdown
Member Author

@addaleax addaleax Jan 27, 2019

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…

if (!json_trace_writer_)
return;
}
int request_id = ++num_write_requests_;
int err = uv_async_send(&flush_signal_);
Expand Down
3 changes: 2 additions & 1 deletion src/tracing/node_trace_writer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ class NodeTraceWriter : public AsyncTraceWriter {
// Triggers callback to close async objects, ending the tracing thread.
uv_async_t exit_signal_;
// Prevents concurrent R/W on state related to serialized trace data
// before it's written to disk, namely stream_ and total_traces_.
// before it's written to disk, namely stream_ and total_traces_
// as well as json_trace_writer_.
Mutex stream_mutex_;
// Prevents concurrent R/W on state related to write requests.
Mutex request_mutex_;
Expand Down