Add workarounds to TestPromiseOrder#582
Merged
Merged
Conversation
When handling a Return message, one of the actions required by the CapNProto protocol is to send a corresponding Finish message, to let the remote vat know that no more calls will reference the answered question. However, there is currently a race condition due to lack of proper synchronization between application code calling a function to a promised answer (i.e. a question) and the library sending the Finish message. In particular, as the TestPromiseOrdering test exposes, due to timing or system loading, application code that has a reference to the promised answer may be in the process of queuing a Call message, but that goroutine may get preempted by the one that is queueing the Finish message, such that the Finish is sent before the Call. This causes the remote host to fail the Call, because it would have already removed its corresponding answer entry (due to Finish arriving first). This, in turn, causes the aforementioned test to fail. While this commit does not decisively solve the issue, it adds a stop gap measure to work around the issue and adds comments explaining it. The work around is a delay between processing the Return and sending the Finish, which allows any concurrent Call to be queued before the Finish. Other calls won't go through the same path, because the Return will cause the path to be shortened. Note that this issue does not entirely depend on path shortening and could happen without it: if a promise is resolved to an import, there's also the issue with lack of locking that could cause a call to be sent to the promise (that is being finished) instead of the import. This will have to be addressed in a more systematic way and this stopgap work around is being used only in order to improve reliance on the CI by making the TestPromiseOrdering less flaky.
Due to lack of proper synchronization, path shortening may cause a local call to be executed before a remote call. And the TestPromiseOrdering explicitly asserts on the server that the order of calls is coming in correctly, thus under load, this test can flake. Note that the _reception_ order is correct: the futures that correspond to the calls end up fulfilled with the correct number, but the _server_ may see the calls out of order. This may be an issue with embargoes not working correctly or it may be a misunderstanding of what assumptions can be relied on by application code using the CapNProto RPC system. In any case, in order to momentarily improve the reliability of the CI, we fix the issue by ensuring the path shortening completes before proceding with future calls.
Contributor
Author
|
For completeness, I should add that an alternative is to disable (via |
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.
These are merely workarounds and not a full solution to the underlying issues, but they (hopefully) allow the CI to stop failing spuriously and allow us to continue developing and improving the library with greater confidence on the test's reliability.