Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

possible fix for queue drop deadlock #3702

Merged
merged 5 commits into from
Dec 5, 2016
Merged

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Dec 2, 2016

Fix in accordance with my theory in #3686 although this is the kind of thing that's very hard to test -- it's more or less at the whim of the OS scheduler :)

Wait conditions are now checked under locks and in a documented ordering.

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Dec 2, 2016
@rphmeier
Copy link
Contributor Author

rphmeier commented Dec 2, 2016

Simplified the conclusion logic (as the deleting flag was shared anyway it didn't need reproducing) and no longer wait for threads to conclude. Stuck verifier threads in the future will now just lead to leaked resources rather than a full deadlock.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 85.987% when pulling 765e1db on fix-queue-drop-deadlock into 3837114 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 85.95% when pulling 7cb9c37 on fix-queue-drop-deadlock into 3837114 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 85.991% when pulling 7cb9c37 on fix-queue-drop-deadlock into 3837114 on master.

@rphmeier rphmeier added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 5, 2016
@rphmeier
Copy link
Contributor Author

rphmeier commented Dec 5, 2016

Will take this opportunity to change how the check is done after discussion with @arkpar

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 5, 2016
@rphmeier
Copy link
Contributor Author

rphmeier commented Dec 5, 2016

Now alters the verification loop to use a state machine which should be resistant to all races.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 85.909% when pulling e7a5363 on fix-queue-drop-deadlock into 3837114 on master.

self.more_to_verify.notify_all();

// second pass to join.
for handle in verifiers.drain(..) {
handle.join();
Copy link
Collaborator

Choose a reason for hiding this comment

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

The queue still needs to wait for all threads to finish

@arkpar arkpar added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 5, 2016
@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Dec 5, 2016
@rphmeier
Copy link
Contributor Author

rphmeier commented Dec 5, 2016

Issues addressed.

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 5, 2016
@arkpar arkpar merged commit 1b6ebe1 into master Dec 5, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 85.901% when pulling 34c670d on fix-queue-drop-deadlock into 3837114 on master.

@arkpar arkpar deleted the fix-queue-drop-deadlock branch January 10, 2017 11:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants