Skip to content

Commit

Permalink
src: fix race condition in ~NodeTraceBuffer
Browse files Browse the repository at this point in the history
Libuv does not guarantee that handles have their close
callbacks called in the order in which they were added
(and in fact, currently calls them in reverse order).

This patch ensures that the `flush_signal_` handle
is no longer in use (i.e. its close callback has already
been run) when we signal to the main thread that
`~NodeTraceBuffer` may be destroyed.

The same applies for `~NodeTraceWriter`.

Credit for debugging goes to Gireesh Punathil.

Fixes: #25512
Co-authored-by: Gireesh Punathil <[email protected]>

PR-URL: #25896
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
  • Loading branch information
addaleax authored and MylesBorins committed May 16, 2019
1 parent 38d8562 commit bc5e04b
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 17 deletions.
25 changes: 18 additions & 7 deletions src/tracing/node_trace_buffer.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "tracing/node_trace_buffer.h"
#include "util-inl.h"

namespace node {
namespace tracing {
Expand Down Expand Up @@ -170,15 +171,25 @@ void NodeTraceBuffer::NonBlockingFlushSignalCb(uv_async_t* signal) {

// static
void NodeTraceBuffer::ExitSignalCb(uv_async_t* signal) {
NodeTraceBuffer* buffer = reinterpret_cast<NodeTraceBuffer*>(signal->data);
uv_close(reinterpret_cast<uv_handle_t*>(&buffer->flush_signal_), nullptr);
uv_close(reinterpret_cast<uv_handle_t*>(&buffer->exit_signal_),
NodeTraceBuffer* buffer =
ContainerOf(&NodeTraceBuffer::exit_signal_, signal);

// Close both flush_signal_ and exit_signal_.
uv_close(reinterpret_cast<uv_handle_t*>(&buffer->flush_signal_),
[](uv_handle_t* signal) {
NodeTraceBuffer* buffer =
ContainerOf(&NodeTraceBuffer::flush_signal_,
reinterpret_cast<uv_async_t*>(signal));

uv_close(reinterpret_cast<uv_handle_t*>(&buffer->exit_signal_),
[](uv_handle_t* signal) {
NodeTraceBuffer* buffer =
reinterpret_cast<NodeTraceBuffer*>(signal->data);
Mutex::ScopedLock scoped_lock(buffer->exit_mutex_);
buffer->exited_ = true;
buffer->exit_cond_.Signal(scoped_lock);
ContainerOf(&NodeTraceBuffer::exit_signal_,
reinterpret_cast<uv_async_t*>(signal));
Mutex::ScopedLock scoped_lock(buffer->exit_mutex_);
buffer->exited_ = true;
buffer->exit_cond_.Signal(scoped_lock);
});
});
}

Expand Down
25 changes: 15 additions & 10 deletions src/tracing/node_trace_writer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -218,18 +218,23 @@ void NodeTraceWriter::AfterWrite() {
void NodeTraceWriter::ExitSignalCb(uv_async_t* signal) {
NodeTraceWriter* trace_writer =
ContainerOf(&NodeTraceWriter::exit_signal_, signal);
// Close both flush_signal_ and exit_signal_.
uv_close(reinterpret_cast<uv_handle_t*>(&trace_writer->flush_signal_),
nullptr);
uv_close(reinterpret_cast<uv_handle_t*>(&trace_writer->exit_signal_),
[](uv_handle_t* signal) {
NodeTraceWriter* trace_writer =
ContainerOf(&NodeTraceWriter::exit_signal_,
reinterpret_cast<uv_async_t*>(signal));
Mutex::ScopedLock scoped_lock(trace_writer->request_mutex_);
trace_writer->exited_ = true;
trace_writer->exit_cond_.Signal(scoped_lock);
});
NodeTraceWriter* trace_writer =
ContainerOf(&NodeTraceWriter::flush_signal_,
reinterpret_cast<uv_async_t*>(signal));
uv_close(
reinterpret_cast<uv_handle_t*>(&trace_writer->exit_signal_),
[](uv_handle_t* signal) {
NodeTraceWriter* trace_writer =
ContainerOf(&NodeTraceWriter::exit_signal_,
reinterpret_cast<uv_async_t*>(signal));
Mutex::ScopedLock scoped_lock(trace_writer->request_mutex_);
trace_writer->exited_ = true;
trace_writer->exit_cond_.Signal(scoped_lock);
});
});
}

} // namespace tracing
} // namespace node

0 comments on commit bc5e04b

Please sign in to comment.