Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix issue with room duplication caused by filtering and selecting room using keyboard #6389

Closed
wants to merge 1 commit into from

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Jul 16, 2021

Wrap sticky room updates in lock to prevent setStickyRoom running in middle of setKnownRooms

Fixes the most common reproductions of element-hq/element-web#14508 but likely not all


So I reproduced this 100% of the time when filtering room list and selecting the room using keyboard.
The difference this has is it also clears the filter at the same time.
This makes two events race in the RoomListStore:

  • Updating the sticky room to the one you selected
  • The setKnownRooms calculating values for the new (unfiltered) list

This issue came about from when the filtering was split into prefiltering and runtime filtering as prior to that the filters were applied later.

In theory a mutex around the critical path in setKnownRooms solves this.
I can no longer reproduce it using the steps which reproduced it 100% of the time for me.
This is much less invasive than #6024
But there is a significant likelihood that other reproduction methods for the same symptoms may still be possible given this is just one race and the RoomListStore is rather complex and has too much async logic.

…m using keyboard

Wrap sticky room updates in lock to prevent setStickyRoom running in middle of setKnownRooms
@t3chguy t3chguy added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Jul 16, 2021
@t3chguy t3chguy requested a review from a team July 16, 2021 07:55
@t3chguy
Copy link
Member Author

t3chguy commented Jul 16, 2021

Hmm, it looks like 75% of the async nature of the RLS is due to it making the SortAlgorithm async even though its probably fast enough to just be blocking, might make another PR with a different approach which is likely more bulletproof but might not scale to accounts like M's

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a sensible workaround but yeah, agreed - the fact that stuff is async for no immediate reason is a bit terrifying.

@t3chguy
Copy link
Member Author

t3chguy commented Jul 16, 2021

Succeeded by #6391

@t3chguy t3chguy closed this Jul 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants