[Metric] Fix crashed when register metric view in multithread#13485
Merged
jovany-wang merged 3 commits intoray-project:masterfrom Jan 25, 2021
Merged
[Metric] Fix crashed when register metric view in multithread#13485jovany-wang merged 3 commits intoray-project:masterfrom
jovany-wang merged 3 commits intoray-project:masterfrom
Conversation
Member
Author
simon-mo
approved these changes
Jan 15, 2021
rkooo567
reviewed
Jan 15, 2021
src/ray/stats/metric.h
Outdated
| std::unique_ptr<opencensus::stats::Measure<double>> measure_; | ||
|
|
||
| // For making sure thread-safe to all of metric registrations. | ||
| static absl::Mutex mutex_; |
Contributor
There was a problem hiding this comment.
Can you rename it to registration_mutex_?
Member
Author
There was a problem hiding this comment.
Can you rename it to registration_mutex_?
Nice catch.
Contributor
|
I guess this can be merged if CI passes. |
Contributor
|
Hi @ashione @zhongchun , should we hold this PR until we fix it totally? If so, I will mark the state to RequestChange. |
jovany-wang
requested changes
Jan 19, 2021
Contributor
jovany-wang
left a comment
There was a problem hiding this comment.
@ashione Please ping me if this get fixed totally~
jovany-wang
approved these changes
Jan 25, 2021
Contributor
jovany-wang
left a comment
There was a problem hiding this comment.
Since there is no failure any more, this can be merged now I think.
Member
Author
No related failures to this PR. |
fishbone
pushed a commit
to fishbone/ray
that referenced
this pull request
Feb 16, 2021
…oject#13485) * Fix crashed when register metric view in multithread * fix comments * fix
fishbone
added a commit
to fishbone/ray
that referenced
this pull request
Feb 16, 2021
…ray-project#13485)" This reverts commit 18216d2.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We fond there will be race while registering metric in multithread and process crash finally.
According to opencensus source code, it's known issue but not fixed yet.
So we'd better add lock for registering metric when first recording.
Why are these changes needed?
Related issue number
#13484
Checks
scripts/format.shto lint the changes in this PR.