Fix #420 #424
Merged
Fix #420 #424
Conversation
This file contains 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
No functional change, but this is in preparation to address capnproto#420; we will have to separate these across two separate calls in Returner.
See the comments. No functional change yet, since all call sites currently call PrepareReturn and Return in immediate succession, but presently this separation will be used to fix a race condition.
(without re-introducing capnproto#394) 420 was a regression introduced by our fix for 394; if we Return() before we fulfill(), this can result in out of order message delivery, since incoming calls can be made directly on clients in the result before all queued calls are drained. Previously, if we fulfilled() before Return(), we would get a data race as fillPayloadCapTable modified the message while pipelined calls read it. Having split Return(error) into PrepareReturn(error) and Return(), it is now safe to move just the latter after the call to fulfill().
lthibault
previously approved these changes
Jan 13, 2023
lthibault
approved these changes
Jan 13, 2023
...based on Louis's feedback.
|
|
...that's one of the existing ones. |
|
Yeah, I thought so, but couldn't find an issue for it. The error in #377 is a different one. Should I open an issue for this? (Re-running CI in the meantime.) |
|
Yeah, why don't you open a separate one; I think this one's been around a while but I keep binning it with #377 |
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.
This fixes #420 (without re-introducing #394, or #349). Per the last commit:
I am now unable to reproduce any of the referenced issues.