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

Fix race condition when emitting stored diagnostics #57066

Merged
merged 1 commit into from Jan 24, 2019
Merged

Fix race condition when emitting stored diagnostics #57066

merged 1 commit into from Jan 24, 2019

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Dec 22, 2018

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 22, 2018
@bors
Copy link
Contributor

bors commented Dec 26, 2018

☔ The latest upstream changes (presumably #57108) made this pull request unmergeable. Please resolve the merge conflicts.

@estebank estebank added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2019
@michaelwoerister
Copy link
Member

Can you explain to me how this is racy? Loaded diagnostics are only emitted by the thread that actually allocates the dep-node and that is already synchronized.

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 8, 2019

try_mark_green is expected to emit diagnostics before returning, but that only happens if we are on the thread which did the dep-node allocation. Other threads may continue executing and emit their own diagnostics before the thread which did the dep-node allocation, giving a wrong order to the emitted diagnostics.

@michaelwoerister
Copy link
Member

Yeah, true. However, I'm not sure how much we can avoid diagnostics re-ordering in general with parallel queries. Or, put differently, we'll have to put some machinery in place to make things more deterministic and that might (1) clash with this incr.comp. specific approach and (2) it might handle this case with the extra logic.
I'd opt for putting this on hold until we figure out the concurrency&diagnostics story more generally.

@stokhos stokhos removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 14, 2019
@TimNN
Copy link
Contributor

TimNN commented Jan 22, 2019

Ping from triage @Zoxc / @michaelwoerister: What is the status of this PR? Is it blocked on something for the foreseeable future?

@michaelwoerister
Copy link
Member

I'm actually fine with merging it the way it is. The code will probably change in the medium term but it doesn't hurt to have this behaving correctly in the meantime.

r=me with the conflicts resolved.

@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 23, 2019

@bors r=michaelwoerister

@bors
Copy link
Contributor

bors commented Jan 23, 2019

📌 Commit b2dfd96 has been approved by michaelwoerister

@bors
Copy link
Contributor

bors commented Jan 23, 2019

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 23, 2019
Centril added a commit to Centril/rust that referenced this pull request Jan 24, 2019
@Centril
Rollup merge of rust-lang#57066 - Zoxc:graph-race, r=michaelwoerister
b3cefbc 
Fix race condition when emitting stored diagnostics

r? @michaelwoerister
@Centril Centril mentioned this pull request Jan 24, 2019
Closed
@bors
Copy link
Contributor

bors commented Jan 24, 2019

⌛ Testing commit b2dfd96 with merge e44e5415dbb2b07cdc332bf32437dac54cd5f70c...

@Centril
Copy link
Contributor

Centril commented Jan 24, 2019 •
edited

@bors retry

yielding prio to rollup in which this is included.

@bors
Copy link
Contributor

bors commented Jan 24, 2019

⌛ Testing commit b2dfd96 with merge f23b63c...

bors added a commit that referenced this pull request Jan 24, 2019
@bors
Auto merge of #57066 - Zoxc:graph-race, r=michaelwoerister
f23b63c 
Fix race condition when emitting stored diagnostics

r? @michaelwoerister
@bors
Copy link
Contributor

bors commented Jan 24, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: michaelwoerister
Pushing f23b63c to master...

@bors
Copy link
Contributor

bors commented Jan 24, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: michaelwoerister
Pushing f23b63c to master...

@bors bors merged commit b2dfd96 into rust-lang:master Jan 24, 2019
@Zoxc Zoxc deleted the graph-race branch January 24, 2019 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Zoxc @bors @michaelwoerister @TimNN @Centril @estebank @rust-highfive @stokhos