src: fix race condition in ~NodeTraceBuffer

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]>

Author: addaleax

src/tracing/node_trace_buffer.cc

@@ -1,4 +1,5 @@
 #include"tracing/node_trace_buffer.h"
+#include"util-inl.h"
 
 namespacenode{
 namespacetracing{
@@ -170,15 +171,25 @@ void NodeTraceBuffer::NonBlockingFlushSignalCb(uv_async_t* signal){
 
 // static
 voidNodeTraceBuffer::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);
+ });
  });
 }
 

src/tracing/node_trace_writer.cc

@@ -218,18 +218,23 @@ void NodeTraceWriter::AfterWrite(){
 voidNodeTraceWriter::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