src: avoid race condition in tracing code #25624
Conversation
|
Stress test CI for |
Aside: is the call to WriteFile() on line 136 sound? FlushPrivate() reads num_write_requests_ while holding the lock, then passes that value to WriteFile() after dropping the lock. It looks like you could observe duplicate request ids.
| if (!json_trace_writer_) { | ||
| return; | ||
| { | ||
| Mutex::ScopedLock stream_mutex_lock(stream_mutex_); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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…
`json_trace_writer_` is protected by `stream_mutex_`, but one access to it was not guarded by a lock on said mutex. Refs: nodejs#25512
I can’t say for sure. /cc @nodejs/trace-events |
|
Landed in 06da364 |
json_trace_writer_is protected bystream_mutex_,but one access to it was not guarded by a lock on said mutex.
Refs: #25512
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes