Refuse splicing the live timeline into a broken position #873
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
Credit to Matthew for basically solving this. Theoretically fixes spontaneous timeline corruption: element-hq/element-web#8593 When the live timeline ends up in a position where it can no longer be live (such as becoming the second timeline in the set, rather than the first) we end up getting neighbouring timeline errors. By refusing to splice the live timeline into such a position, we hopefully keep the live timeline in a position of still being live for when it is next used. The running theory that leads to this fix is multiple limited syncs coming in, causing holes in the timeline. When trying to patch up the holes, the timeline set would end up splicing all over the place, leading to potentially splicing the live timeline into a broken position.
ara4n
reviewed
Mar 29, 2019
| @@ -408,8 +408,21 @@ EventTimelineSet.prototype.addEventsToTimeline = function(events, toStartOfTimel | |||
| console.info("Already have timeline for " + eventId + | |||
| " - joining timeline " + timeline + " to " + | |||
| existingTimeline); | |||
| timeline.setNeighbouringTimeline(existingTimeline, direction); | |||
| existingTimeline.setNeighbouringTimeline(timeline, inverseDirection); | |||
|
|
|||
There was a problem hiding this comment.
the warnings are a bit cryptic here, to the point of being very confusing for the 2nd one. how about:
"Refusing to set a preceding existingTimeLine on our timeline as the existingTimeLine is live"
and
"Refusing to set our preceding timeline on a existingTimeLine as our timeline is live"
perhaps?
|
lgtm. only other change would be to log the timeline ids so we can do forensics later |
turt2live
added a commit
that referenced
this pull request
Apr 2, 2019
See element-hq/element-web#8593 (comment) Previously (#873) we allowed half-linking timelines to each other if they satisfy the conditions, however this appears to not be helping. Instead, it seems like the timelines are getting stuck in a position where one direction is spliced but the other is broken. To avoid this case, we'll just avoid splicing in both directions when one of the directions is invalid.
bwindels
pushed a commit
that referenced
this pull request
Apr 8, 2019
See element-hq/element-web#8593 (comment) Previously (#873) we allowed half-linking timelines to each other if they satisfy the conditions, however this appears to not be helping. Instead, it seems like the timelines are getting stuck in a position where one direction is spliced but the other is broken. To avoid this case, we'll just avoid splicing in both directions when one of the directions is invalid.
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.
Credit to Matthew for basically solving this.
Theoretically fixes spontaneous timeline corruption: element-hq/element-web#8593
When the live timeline ends up in a position where it can no longer be live (such as becoming the second timeline in the set, rather than the first) we end up getting neighbouring timeline errors. By refusing to splice the live timeline into such a position, we hopefully keep the live timeline in a position of still being live for when it is next used.
The running theory that leads to this fix is multiple limited syncs coming in, causing holes in the timeline. When trying to patch up the holes, the timeline set would end up splicing all over the place, leading to potentially splicing the live timeline into a broken position.
Disclaimer: I've done light testing on Riot to make sure it doesn't fail in miserable ways, but have not reproduced the case which causes the issue in the first place. The issue is a bit complex to test, hence the word "theory" being littered above.