Fix multiple boosts of a same toot erroneously appearing in TL #14759
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
Instead of using ZREVRANK to determine whether a reblog is a new reblog or not, use ZADD's NX option to perform the check/addition option atomically.
ClearlyClaire
changed the title
ClearlyClaire
changed the title
Gargron
approved these changes
Sep 7, 2020
thenameisnigel-old
pushed a commit
to ChatterlyOSE/Chatterly
that referenced
this pull request
Sep 9, 2020
…don#14759) * Check for and record reblog info atomically Instead of using ZREVRANK to determine whether a reblog is a new reblog or not, use ZADD's NX option to perform the check/addition option atomically. * Replace ZREVRANK call with ZSCORE key which is more efficient * Make tests a bit stricter * Fix off-by-one
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.
I have seen and been reported (very rare) cases of multiple consecutive boosts of a same toot appearing in a timeline despite the the “aggregate reblogs” option being checked.
The only possible cause I have identified is the check for a toot having been recently boosted and adding that toot to the set of recently-boosted toots. I am unconvinced this would be the cause, as the time window for such a race condition is extremely narrow, but it would be it is a possible cause, consistent with the reports. I have fixed this race condition.
This is done by using the
NXoption of Redis'ZADD. I am unsure about the performance implications, but as Redis' documentation does not mention anything related to performances, and that the check was formerly made with a Redis call as complex asZADDitself (ZREVRANK), I think performances should be better when the posted item is a new reblog, and roughly the same otherwise.Compatibility-wise,
ZADD'sNXoption has been added in Redis 3.0.2, and we currently require Redis 4 because of Sidekiq, so it should be fine to use.This PR also includes minor fixups/cleanups/performance improvements.