Fix lost stream-update notifications and blocked freezes in the C++ client#974
Merged
Conversation
The ProcedureCall invoke overload built the request by copying the call field by field, which silently drops any field ProcedureCall gains. The call arrives by const reference and a Request owns its calls, so a copy is unavoidable here; use CopyFrom, which is correct and future-proof.
The _client and _id fields are public because the encoder and decoder need them and cannot be given access with 'friend' without a circular dependency between the headers. State that as a constraint rather than a TODO to make them private.
The stream update thread notified the update condition without holding its mutex, so a notification could fire between a waiter checking the stream value and entering its wait and be lost, leaving the waiter asleep through the update it was waiting for. Notify while holding the mutex, matching the Python client. Re-enables the three wait_for_stream_update tests, in the shape of the Python client's equivalents: the stream is started while the update condition is held and the first wait synchronizes on an update message boundary before the baseline count is read, so each subsequent wait corresponds to exactly one update message. Passes repeated runs of the full client suite.
freeze_streams blocked until the next stream update message arrived, because the update thread only checked the freeze flag after applying a message. With no started streams, or no stream whose value is changing, no message ever arrives and the call blocked forever. The update thread now also acknowledges a freeze from the receive loop's timeout path, between messages, so freezing is bounded by the receive poll interval. Re-enables test_stream_stop_while_frozen, which freezes with a stream that is not producing updates and tears down while frozen. It dates from when streams started at creation; deferred start turned its stream into one that never produces updates, hitting the blocked-freeze case.
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
1 participant
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.
Two long-standing bugs in the C++ client's stream machinery, found by re-enabling tests disabled as flaky in 2018.
Lost notifications. The stream update thread called
notify_allon the update condition without holding its mutex, so a notification could fire between a waiter checking the stream value and entering its wait, and be lost — leavingwait_for_stream_updatesleeping through the update it was waiting for. This is why the threewait_for_stream_updatetests could never be stabilised. The update thread now notifies while holding the condition mutex, matching the Python client, and the tests are re-enabled in the shape of the Python client's equivalents (the stream is started while the condition is held, and the first wait synchronises on an update-message boundary before the baseline count is read).Blocked freezes.
freeze_streamswas only acknowledged by the update thread after applying an update message, so with no started stream — or no stream whose value is changing — it blocked forever. The disabledtest_stream_stop_while_frozenpredates deferred stream start; when streams stopped auto-starting on creation, its stream stopped producing updates and the freeze hung, matching the date it was disabled. The update thread now also acknowledges a freeze from the receive loop's timeout path, between messages, so freezing is bounded by the receive poll interval. The test is re-enabled verbatim.Also copies the call into stream requests with
CopyFromrather than field-by-field, so future fields are not dropped, and documents why theObjectfields are public.