Fix Effect.throttle data races and scheduler value return
#669
Conversation
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
The `Effect.throttle` operator had multiple data races when updating `throttleTimes` and `throttleValues` shared state, because: 1. `Effect.throttle` can be called from any scheduler. 2. The internal `flatMap` runs on the current chain scheduler, but the throttled (delayed) value runs on the passed in `scheduler` parameter (which can be different). By protecting shared state with a lock (similar to the cancellables'), these data races should be addressed. Additionally, the `Effect.throttle` should return all values in the `scheduler` passed in, so that the API contract is honored and values come from where callers expect them to. ## Changes - Add new `throttleLock` `recursive lock to protect `throttleTimes` and `throttleValues` shared state in `Effect.throttle` operator. - Ensure all values in `Effect.throttle` come from the passed in `scheduler`.
Member
|
@p4checo Thanks for the PR! Looks like the changes are breaking our test expectations. Can you look into what may have changed? |
p4checo
commented
Jul 22, 2021
stephencelis
approved these changes
Jul 22, 2021
p4checo
deleted the
fix-effect-throttle-data-races-and-scheduler-value-return
branch
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.
The
Effect.throttleoperator had multiple data races when updatingthrottleTimesandthrottleValuesshared state, because:Effect.throttlecan be called from any scheduler.flatMapruns on the current chain scheduler, but the throttled (delayed) value runs on the passed inschedulerparameter (which can be different).By protecting shared state with a lock (similar to the cancellables'), these data races should be addressed.
Additionally, the
Effect.throttleshould return all values in theschedulerpassed in, so that the API contract is honored and values come from where callers expect them to.Changes
Add new
throttleLockrecursive lock to protectthrottleTimesandthrottleValuesshared state inEffect.throttle` operator.Ensure all values in
Effect.throttlecome from the passed inscheduler.