Fix deadlock#316
Merged
Merged
Conversation
It is illegal to resolve promises while holding connection locks, and I was seeing TestHandleReturn_regression deadlock every 6th run or so; changing this fixes it. Note: since this makes the resolution itself and the manipulation of the questions table non-atomic, the ordering is critical: 1. First we remove the entry from the table. This means that if the remote vat sends us messages pertaining to that question id, they will simply be rejected instead being acted on. This shouldn't happen normally, but maybe a malicious vat could get us to panic or something. 2. Second, resolve the promise. After this is complete, we can be sure the client won't be used. 3. Then, and only then, we can return the question id to the pool for re-use.
Unlike the previous commit, I haven't observed the consequences of these, but the same logic & ordering constraints apply.
Merged
lthibault
approved these changes
Oct 1, 2022
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
2 participants
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.
See the first commit message. The second commit fixes similar logic errors, though I haven't seen consequences of those errors.
There are some other tests that occasionally deadlock, but I haven't tracked down the cause yet.