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
Prev Previous commit
fixup! src: avoid race condition in tracing code
  • Loading branch information
@addaleax
addaleax committed Jan 27, 2019
commit acabd3e937df0686c5a80cbd8e3fdd3c92178644
3 changes: 3 additions & 0 deletions src/tracing/node_trace_writer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ void NodeTraceWriter::FlushPrivate() {
void NodeTraceWriter::Flush(bool blocking) {
Mutex::ScopedLock scoped_lock(request_mutex_);
{
// We need to lock the mutexes here in a nested fashion; stream_mutex_
// protects json_trace_writer_, and without request_mutex_ there might be
// a time window in which the stream state changes?
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;
Expand Down
1 change: 1 addition & 0 deletions src/tracing/node_trace_writer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class NodeTraceWriter : public AsyncTraceWriter {
// as well as json_trace_writer_.
Mutex stream_mutex_;
// Prevents concurrent R/W on state related to write requests.
// If both mutexes are locked, request_mutex_ has to be locked first.
Mutex request_mutex_;
// Allows blocking calls to Flush() to wait on a condition for
// trace events to be written to disk.
Expand Down