src: trace_events: fix race with metadata events - #25235
Conversation
Sorry, something went wrong.
Multiple threads may be concurrently adding to the metadata_events list. Protect access with a mutex. Fixes: nodejs#24129
|
@ofrobots build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2074/pipeline |
Sorry, something went wrong.
|
Has anyone gotten TSAN/ASAN builds working? They might help catch issues like this easier. |
Sorry, something went wrong.
|
@ofrobots - presence of mutexes are found to be problematic (ref: #25007) . While there is no specific issue as such to this PR, increase in the number of mutextes increases the chance of crashes at the non-cordinated process exit route; unless we arrive at a plan to control those (conditions such as attempting to lock a mutex that is already destroyed, attempting to destroy a mutex that is locked, etc). thoughts? |
Sorry, something went wrong.
| std::set<AsyncTraceWriter*> to_be_initialized_; | ||
|
|
||
| Mutex metadata_events_mutex_; | ||
| std::list<std::unique_ptr<TraceObject>> metadata_events_; |
There was a problem hiding this comment.
Maybe a dumb question, but when does this list get cleared? Does it ever?
Sorry, something went wrong.
There was a problem hiding this comment.
It doesn't and shouldn't really. These need to be cached and re-emitted whenever the trace file rolls over to a new file. There should only ever be a limited number of these.
Sorry, something went wrong.
There was a problem hiding this comment.
Right. Metadata events are meta information about the process and environment rather than being events at particular points in time. In production scenarios, the trace events may be streamed across the network where only a trailing buffer is retained. The idea is that these metadata events should be periodically remitted to ensure that they are still available even if the start of the log is truncated.
Sorry, something went wrong.
Sorry, something went wrong.
|
@addaleax thanks for launching the CI. I had started one earlier as well: https://ci.nodejs.org/job/node-test-pull-request/19975/ which is unfinished but well-poised right. |
Sorry, something went wrong.
Multiple threads may be concurrently adding to the metadata_events list. Protect access with a mutex. Fixes: nodejs#24129 PR-URL: nodejs#25235 Reviewed-By: Anna Henningsen <anna@addaleax.net>
Multiple threads may be concurrently adding to the metadata_events list. Protect access with a mutex. Fixes: nodejs#24129 PR-URL: nodejs#25235 Reviewed-By: Anna Henningsen <anna@addaleax.net>
Multiple threads may be concurrently adding to the metadata_events list. Protect access with a mutex. Fixes: nodejs#24129 PR-URL: nodejs#25235 Reviewed-By: Anna Henningsen <anna@addaleax.net>
Multiple threads may be concurrently adding to the metadata_events list.
Protect access with a mutex.
Fixes: #24129
Checklist
CI: https://ci.nodejs.org/job/node-test-pull-request/19827/ https://ci.nodejs.org/job/node-test-pull-request/19975/