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

Fixed issues with reporting sync state events from different threads #6341

Merged
merged 1 commit into from
Jul 25, 2022

Conversation

artkoenig
Copy link
Contributor

@artkoenig artkoenig commented Jun 17, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Replaced LiveData with Flow for sync state reporting.

Motivation and context

SyncRequestStateTracker instance is reused in different threads, because of the Scope annotation. If LiveData::postValue is called fast enough, some events can get lost, because only the last one is dispatched.
So in my case I could reproduce loosing of InitialSyncRequestState.Idle due to sending IncrementalSyncIdle from another thread.

Signed-off-by: Artjom König [email protected]

@bmarty bmarty added the Z-Community-PR Issue is solved by a community member's PR label Jun 17, 2022
Copy link
Member

@bmarty bmarty 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 the PR. One first remark.

@bmarty bmarty self-requested a review June 30, 2022 12:55
Copy link
Member

@bmarty bmarty 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 the update (and sorry for the delay to review it). Just one remark about the Kdoc, then this can be merged. For the remark about the CoroutineScope we will handle it.

Note that I will squash and merge your commits to cleanup the Git history. Feel free to squash your commits and force push if you want to.

@artkoenig
Copy link
Contributor Author

Hi @bmarty, are there still any issues preventing this to be merged?

Copy link
Member

@bmarty bmarty 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 the update.
I have triggered the CI on this PR.

@bmarty bmarty enabled auto-merge July 22, 2022 13:23
@bmarty bmarty disabled auto-merge July 25, 2022 08:21
@bmarty bmarty merged commit c28e7c8 into element-hq:develop Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants