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.

Sign up for GitHub

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

Jump to bottom

Fix race condition with sliding sync extensions #4089

Merged
merged 3 commits into from
Mar 8, 2024
Merged

Fix race condition with sliding sync extensions #4089

merged 3 commits into from
Mar 8, 2024

Conversation

zzorba
Copy link
Contributor

@zzorba zzorba commented Feb 29, 2024

While debugging verification when using sliding sync, I encountered the same events being processed multiple times, causing the VerificationRequest state machine to break (some events are reentrant, but ready -> ready is a cancellable error).

I noticed that the onResponse for ExtensionToDevice was declared void but overridden as Promise<void>, with the nextBatch cursor getting updated at the END of the function (after an await). In local testing, it was observed that next onRequest would sometimes start before the previous onResponse was finished, leading to the same since parameter being sent, leading to the same events being processed multiple times.

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

@zzorba zzorba requested a review from a team as a code owner February 29, 2024 21:21
@zzorba zzorba requested review from dbkr and MidhunSureshR February 29, 2024 21:21
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Feb 29, 2024
@jryans
Copy link
Collaborator

jryans commented Mar 6, 2024

I can confirm that device verification while using sliding sync (with legacy / non-Rust crypto) currently aborts. With this PR applied, device verification works when using sliding sync.

kegsay
kegsay approved these changes Mar 7, 2024
View reviewed changes
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.

Excellent catch, thank you!

@t3chguy t3chguy added this pull request to the merge queue Mar 7, 2024
Merged via the queue into matrix-org:develop with commit 51ebd2f Mar 8, 2024
20 checks passed
thoraj added a commit to verji/matrix-js-sdk that referenced this pull request Mar 14, 2024
@thoraj @RiotRobot
@dbkr @zzorba @t3chguy @renovate @HarHarLinks @richvdh
Pull in upstream changes via Develop (#2)
d727120 
* v31.5.0-rc.0

* Don't re-fetch thread root if we already have it (matrix-org#4088)

The root event of a thread used to arrive with the pagination request, but this was unspecced and so got changed to simply fetch the root event. In many (almost all) cases this shouldn't be necessary because the thread should already have its root event: re-use it if it's already there. This is only in pagination, so there's no reason to believe that the root event would have changed and needs to be re-fetched.

This removes a number of duplicate calls to the /event/ endpoint from the tests.

* Fix race condition with sliding sync extensions (matrix-org#4089)

* Fix race condition with sliding sync extensions

* Fix types on sliding-sync spec test

* Prettier fixes

* Add `.m.rule.is_room_mention` push rule to DEFAULT_OVERRIDE_RULES (matrix-org#4100)

* Add intentional mentions push rules to DEFAULT_OVERRIDE_RULES

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

---------

Signed-off-by: Michael Telatynski <[email protected]>

* Export types describing all specced media event formats (matrix-org#4092)

* Export types describing all specced media event formats

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate PR

Signed-off-by: Michael Telatynski <[email protected]>

* Move types to a dedicated export

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Add readme entry

Signed-off-by: Michael Telatynski <[email protected]>

---------

Signed-off-by: Michael Telatynski <[email protected]>

* Update dependency typedoc to v0.25.11 (matrix-org#4102)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* v31.5.0

* Resetting package fields for development

* Temporarily disable broken step in the release process

Signed-off-by: Michael Telatynski <[email protected]>

* fix automatic DM avatar with functional members (matrix-org#4017)

* fix automatic DM avatar with functional members

* update comments

* lint

* add tests for functional members

* keep functional members out of the public API

- remove public API for functional members, reverting most of 0ce2d82, f9b41f6, e65fb24
- remove tests for functional members public API c114bf5
- add shared functional members getter for both room name and avatar fallback generation

* filter functional members from more candidates

- remove from hero(es)
- remove from previous members

* add tests for fallback avatars with functional members

* Add docstring for getFunctionalMembers

Co-authored-by: Richard van der Hoff <[email protected]>

* inline getInvitedAndJoinedFunctionalMemberCount

* update comments for getAvatarFallbackMember

* use correct list of heroes in getAvatarFallbackMember

* remove redundant type annotation

* optimize performance of invitedAndJoinedFunctionalMemberCount

* calculate nonFunctionalMemberCount in one step

instead of iterating redundantly

* clean up functional member tests with review feedback

* lint

* Update src/models/room.ts

Co-authored-by: Richard van der Hoff <[email protected]>

* apply feedback about comments

* non-functional per review, lint

---------

Co-authored-by: Richard van der Hoff <[email protected]>

---------

Signed-off-by: Michael Telatynski <[email protected]>
Co-authored-by: RiotRobot <[email protected]>
Co-authored-by: David Baker <[email protected]>
Co-authored-by: Daniel Salinas <[email protected]>
Co-authored-by: Michael Telatynski <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Kim Brose <[email protected]>
Co-authored-by: Richard van der Hoff <[email protected]>
thoraj added a commit to verji/matrix-js-sdk that referenced this pull request Mar 15, 2024
@thoraj @RiotRobot
@dbkr @zzorba @t3chguy @renovate @HarHarLinks @richvdh
Pull in upstream changes via verji#develop (#3)
7511389 
* v31.5.0-rc.0

* Don't re-fetch thread root if we already have it (matrix-org#4088)

The root event of a thread used to arrive with the pagination request, but this was unspecced and so got changed to simply fetch the root event. In many (almost all) cases this shouldn't be necessary because the thread should already have its root event: re-use it if it's already there. This is only in pagination, so there's no reason to believe that the root event would have changed and needs to be re-fetched.

This removes a number of duplicate calls to the /event/ endpoint from the tests.

* Fix race condition with sliding sync extensions (matrix-org#4089)

* Fix race condition with sliding sync extensions

* Fix types on sliding-sync spec test

* Prettier fixes

* Add `.m.rule.is_room_mention` push rule to DEFAULT_OVERRIDE_RULES (matrix-org#4100)

* Add intentional mentions push rules to DEFAULT_OVERRIDE_RULES

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

---------

Signed-off-by: Michael Telatynski <[email protected]>

* Export types describing all specced media event formats (matrix-org#4092)

* Export types describing all specced media event formats

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate PR

Signed-off-by: Michael Telatynski <[email protected]>

* Move types to a dedicated export

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Add readme entry

Signed-off-by: Michael Telatynski <[email protected]>

---------

Signed-off-by: Michael Telatynski <[email protected]>

* Update dependency typedoc to v0.25.11 (matrix-org#4102)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* v31.5.0

* Resetting package fields for development

* Temporarily disable broken step in the release process

Signed-off-by: Michael Telatynski <[email protected]>

* fix automatic DM avatar with functional members (matrix-org#4017)

* fix automatic DM avatar with functional members

* update comments

* lint

* add tests for functional members

* keep functional members out of the public API

- remove public API for functional members, reverting most of 0ce2d82, f9b41f6, e65fb24
- remove tests for functional members public API c114bf5
- add shared functional members getter for both room name and avatar fallback generation

* filter functional members from more candidates

- remove from hero(es)
- remove from previous members

* add tests for fallback avatars with functional members

* Add docstring for getFunctionalMembers

Co-authored-by: Richard van der Hoff <[email protected]>

* inline getInvitedAndJoinedFunctionalMemberCount

* update comments for getAvatarFallbackMember

* use correct list of heroes in getAvatarFallbackMember

* remove redundant type annotation

* optimize performance of invitedAndJoinedFunctionalMemberCount

* calculate nonFunctionalMemberCount in one step

instead of iterating redundantly

* clean up functional member tests with review feedback

* lint

* Update src/models/room.ts

Co-authored-by: Richard van der Hoff <[email protected]>

* apply feedback about comments

* non-functional per review, lint

---------

Co-authored-by: Richard van der Hoff <[email protected]>

---------

Signed-off-by: Michael Telatynski <[email protected]>
Co-authored-by: RiotRobot <[email protected]>
Co-authored-by: David Baker <[email protected]>
Co-authored-by: Daniel Salinas <[email protected]>
Co-authored-by: Michael Telatynski <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Kim Brose <[email protected]>
Co-authored-by: Richard van der Hoff <[email protected]>
thoraj added a commit to verji/matrix-js-sdk that referenced this pull request Mar 15, 2024
@thoraj @RiotRobot
@dbkr @zzorba @t3chguy @renovate @HarHarLinks @richvdh
Develop (#4)
8771159 
* v31.5.0-rc.0

* Don't re-fetch thread root if we already have it (matrix-org#4088)

The root event of a thread used to arrive with the pagination request, but this was unspecced and so got changed to simply fetch the root event. In many (almost all) cases this shouldn't be necessary because the thread should already have its root event: re-use it if it's already there. This is only in pagination, so there's no reason to believe that the root event would have changed and needs to be re-fetched.

This removes a number of duplicate calls to the /event/ endpoint from the tests.

* Fix race condition with sliding sync extensions (matrix-org#4089)

* Fix race condition with sliding sync extensions

* Fix types on sliding-sync spec test

* Prettier fixes

* Add `.m.rule.is_room_mention` push rule to DEFAULT_OVERRIDE_RULES (matrix-org#4100)

* Add intentional mentions push rules to DEFAULT_OVERRIDE_RULES

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

---------

Signed-off-by: Michael Telatynski <[email protected]>

* Export types describing all specced media event formats (matrix-org#4092)

* Export types describing all specced media event formats

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate PR

Signed-off-by: Michael Telatynski <[email protected]>

* Move types to a dedicated export

Signed-off-by: Michael Telatynski <[email protected]>

* Iterate

Signed-off-by: Michael Telatynski <[email protected]>

* Add readme entry

Signed-off-by: Michael Telatynski <[email protected]>

---------

Signed-off-by: Michael Telatynski <[email protected]>

* Update dependency typedoc to v0.25.11 (matrix-org#4102)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* v31.5.0

* Resetting package fields for development

* Temporarily disable broken step in the release process

Signed-off-by: Michael Telatynski <[email protected]>

* fix automatic DM avatar with functional members (matrix-org#4017)

* fix automatic DM avatar with functional members

* update comments

* lint

* add tests for functional members

* keep functional members out of the public API

- remove public API for functional members, reverting most of 0ce2d82, f9b41f6, e65fb24
- remove tests for functional members public API c114bf5
- add shared functional members getter for both room name and avatar fallback generation

* filter functional members from more candidates

- remove from hero(es)
- remove from previous members

* add tests for fallback avatars with functional members

* Add docstring for getFunctionalMembers

Co-authored-by: Richard van der Hoff <[email protected]>

* inline getInvitedAndJoinedFunctionalMemberCount

* update comments for getAvatarFallbackMember

* use correct list of heroes in getAvatarFallbackMember

* remove redundant type annotation

* optimize performance of invitedAndJoinedFunctionalMemberCount

* calculate nonFunctionalMemberCount in one step

instead of iterating redundantly

* clean up functional member tests with review feedback

* lint

* Update src/models/room.ts

Co-authored-by: Richard van der Hoff <[email protected]>

* apply feedback about comments

* non-functional per review, lint

---------

Co-authored-by: Richard van der Hoff <[email protected]>

---------

Signed-off-by: Michael Telatynski <[email protected]>
Co-authored-by: RiotRobot <[email protected]>
Co-authored-by: David Baker <[email protected]>
Co-authored-by: Daniel Salinas <[email protected]>
Co-authored-by: Michael Telatynski <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Kim Brose <[email protected]>
Co-authored-by: Richard van der Hoff <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kegsay kegsay kegsay approved these changes

@t3chguy t3chguy t3chguy approved these changes

@dbkr dbkr Awaiting requested review from dbkr dbkr is a code owner automatically assigned from matrix-org/element-web-reviewers

@MidhunSureshR MidhunSureshR Awaiting requested review from MidhunSureshR MidhunSureshR is a code owner automatically assigned from matrix-org/element-web-reviewers

Assignees
No one assigned
Labels
T-Defect Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zzorba @jryans @t3chguy @kegsay