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

Show file tree
Hide file tree
Changes from all commits
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
9 changes: 7 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,13 @@ void NodeTraceWriter::FlushPrivate() {

void NodeTraceWriter::Flush(bool blocking) {
Mutex::ScopedLock scoped_lock(request_mutex_);
if (!json_trace_writer_) {
return;
{
// 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
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…

if (!json_trace_writer_)
return;
}
int request_id = ++num_write_requests_;
int err = uv_async_send(&flush_signal_);
Expand Down
4 changes: 3 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,9 +45,11 @@ 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.
// 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