Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix room membership race with PREPARED event #2613

Merged
merged 3 commits into from
Aug 23, 2022
Merged

Conversation

jotto
Copy link
Contributor

@jotto jotto commented Aug 23, 2022

See the call site of the original triggering event of this function:

Object.keys(resp.rooms).forEach((roomId) => {
this.invokeRoomDataListeners(
roomId,
resp.rooms[roomId],
);
});
const listIndexesWithUpdates: Set<number> = new Set();
if (!doNotUpdateList) {
resp.lists.forEach((list, listIndex) => {
list.ops = list.ops || [];
if (list.ops.length > 0) {
listIndexesWithUpdates.add(listIndex);
}
this.processListOps(list, listIndex);
});
}
this.invokeLifecycleListeners(SlidingSyncState.Complete, resp);

I think the bug is current code assumes downstream event listeners of SlidingSyncEvent.RoomData have synchronous execution so that by the time it emits SlidingSyncState.Complete, and eventually SyncState.Prepared the room state is correct. But since SlidingSyncSdk's processRoomData is async, and the membership field was being set after the async, it looks like SlidingSyncState.Complete was being fired before the membership field was set.

Signed-off-by: Jonathan Otto jonathan.otto@gmail.com


Here's what your changelog entry will look like:

🐛 Bug Fixes

  • Fix room membership race with PREPARED event (#2613). Contributed by @jotto.

See the call site of the original triggering event of this function: https://github.com/matrix-org/matrix-js-sdk/blob/b265d795a427c6d30ccdf279a09f7836509df863/src/sliding-sync.ts#L789-L806

I think the bug is current code assumes downstream event listeners of `SlidingSyncEvent.RoomData` have synchronous execution so that by the time it emits `SlidingSyncState.Complete`, and eventually `SyncState.Prepared` the room state is correct. But since SlidingSyncSdk's `processRoomData` is async, and the membership field was being set after the async, it looks like `SlidingSyncState.Complete` was being fired before the membership field was set.
@jotto jotto requested a review from a team as a code owner August 23, 2022 01:13
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Aug 23, 2022
@turt2live turt2live requested review from t3chguy and kegsay and removed request for turt2live, weeman1337 and duxovni August 23, 2022 04:29
Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Thanks for this. I thought I did basically what sync.ts did here, but this makes sense to set it prior to invoking listeners. Needs linting fixes and a regression test though ideally.

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Thanks for this

@t3chguy t3chguy enabled auto-merge (squash) August 23, 2022 14:54
@t3chguy t3chguy merged commit 5e4474b into matrix-org:develop Aug 23, 2022
@jotto jotto deleted the patch-1 branch August 23, 2022 15:01
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request Sep 1, 2022
* Re-emit room state events on rooms ([\matrix-org#2607](matrix-org#2607)).
* Add ability to override built in room name generator for an i18n'able one ([\matrix-org#2609](matrix-org#2609)).
* Add txn_id support to sliding sync ([\matrix-org#2567](matrix-org#2567)).
* Refactor Sync and fix `initialSyncLimit` ([\matrix-org#2587](matrix-org#2587)).
* Use deep equality comparisons when searching for outgoing key requests by target ([\matrix-org#2623](matrix-org#2623)). Contributed by @duxovni.
* Fix room membership race with PREPARED event ([\matrix-org#2613](matrix-org#2613)). Contributed by @jotto.
* fixed a sliding sync bug which could cause the `roomIndexToRoomId` map to be incorrect when a new room is added in the middle of the list or when an existing room is deleted from the middle of the list. ([\matrix-org#2610](matrix-org#2610)).
* Fix: Handle parsing of a beacon info event without asset ([\matrix-org#2591](matrix-org#2591)). Fixes element-hq/element-web#23078. Contributed by @kerryarchibald.
* Fix finding event read up to if stable private read receipts is missing ([\matrix-org#2585](matrix-org#2585)). Fixes element-hq/element-web#23027.
* fixed a sliding sync issue where history could be interpreted as live events. ([\matrix-org#2583](matrix-org#2583)).
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request Sep 15, 2022
* Fix bug in deepCompare which would incorrectly return objects with disjoint keys as equal ([\matrix-org#2586](matrix-org#2586)). Contributed by @3nprob.
* Refactor Sync and fix `initialSyncLimit` ([\matrix-org#2587](matrix-org#2587)).
* Use deep equality comparisons when searching for outgoing key requests by target ([\matrix-org#2623](matrix-org#2623)). Contributed by @duxovni.
* Fix room membership race with PREPARED event ([\matrix-org#2613](matrix-org#2613)). Contributed by @jotto.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants