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: trace_events: fix race with metadata events #25235

Closed
wants to merge 1 commit into from
Closed

src: trace_events: fix race with metadata events #25235

wants to merge 1 commit into from

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Dec 27, 2018 •
edited
Loading

Multiple threads may be concurrently adding to the metadata_events list.
Protect access with a mutex.

Fixes: #24129

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

CI: https://ci.nodejs.org/job/node-test-pull-request/19827/ https://ci.nodejs.org/job/node-test-pull-request/19975/

@ofrobots
src: trace_events: fix race with metadata events
73859b6 
Multiple threads may be concurrently adding to the metadata_events list.
Protect access with a mutex.

Fixes: nodejs#24129
@nodejs-github-bot
Copy link
Collaborator

@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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 27, 2018
@ofrobots ofrobots mentioned this pull request Dec 27, 2018
Closed
@ofrobots
Copy link
Contributor Author

Has anyone gotten TSAN/ASAN builds working? They might help catch issues like this easier.

@ofrobots ofrobots added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Dec 27, 2018
@gireeshpunathil
Copy link
Member

@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?

addaleax
addaleax approved these changes Dec 27, 2018
View reviewed changes
src/tracing/agent.h
@@ -145,6 +143,8 @@ class Agent {
ConditionVariable initialize_writer_condvar_;
uv_async_t initialize_writer_async_;
std::set<AsyncTraceWriter*> to_be_initialized_;

Mutex metadata_events_mutex_;
std::list<std::unique_ptr<TraceObject>> metadata_events_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a dumb question, but when does this list get cleared? Does it ever?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gireeshpunathil
Copy link
Member

@ofrobots - pls disregard my previous comment / question; thinking on it agin, that question does not make any sense here at all. In addition, I saw your comment in #25007, clarifying some outstanding questions there.

@addaleax
Copy link
Member

addaleax commented Jan 7, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/19981/

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 7, 2019
@ofrobots
Copy link
Contributor Author

ofrobots commented Jan 7, 2019

@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.

@addaleax
Copy link
Member

addaleax commented Jan 7, 2019

Landed in f39b3e3

@addaleax addaleax closed this Jan 7, 2019
pull bot pushed a commit to SimenB/node that referenced this pull request Jan 7, 2019
@ofrobots @addaleax
src: trace_events: fix race with metadata events
f39b3e3 
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>
addaleax pushed a commit that referenced this pull request Jan 9, 2019
@ofrobots @addaleax
src: trace_events: fix race with metadata events
807e732 
Multiple threads may be concurrently adding to the metadata_events list.
Protect access with a mutex.

Fixes: #24129

PR-URL: #25235
Reviewed-By: Anna Henningsen <anna@addaleax.net>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@ofrobots @refack
src: trace_events: fix race with metadata events
cb82a04 
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>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@ofrobots @BridgeAR
src: trace_events: fix race with metadata events
939f4ad 
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>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
@ofrobots @BethGriggs
src: trace_events: fix race with metadata events
2c2d71c 
Multiple threads may be concurrently adding to the metadata_events list.
Protect access with a mutex.

Fixes: #24129

PR-URL: #25235
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
@ofrobots @BethGriggs
src: trace_events: fix race with metadata events
6381452 
Multiple threads may be concurrently adding to the metadata_events list.
Protect access with a mutex.

Fixes: #24129

PR-URL: #25235
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@ofrobots @MylesBorins
src: trace_events: fix race with metadata events
883d61c 
Multiple threads may be concurrently adding to the metadata_events list.
Protect access with a mutex.

Fixes: #24129

PR-URL: #25235
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ofrobots @nodejs-github-bot @gireeshpunathil @addaleax @jasnell