-
Notifications
You must be signed in to change notification settings - Fork 742
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 EventInsertLiveObserver gets blocked by reverting and adding lock… #6279
Conversation
observerScope.launch { | ||
lock.withLock { | ||
if (!results.isLoaded || results.isEmpty()) { | ||
return@launch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably equivalent, but is it safer to use return@withLock
instead of return@launch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I don't know, but I can change if you prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, do you think there will be a performance effect?
if (!results.isLoaded || results.isEmpty()) { | ||
return | ||
} | ||
observerScope.launch { onResultsChangedFlow.emit(results) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we know the cause of the issue/reproduction steps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reproduce a lot. Just sent some reactions, if you go back on room list and open again the timeline, there is a high chance you don't see the reactions as the event is not aggregated.
For the exact cause I don't know, but looks like the inner channel gets blocked by the the looper thread...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same for me, I cannot repro)
for context, the PR that introduced the change #5909 |
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this 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.
"Request changes" state to prevent merging this PR, I will cherry-pick commit on the hotfix branch.
Commits are included in 1.4.20 |
Fix #6278
Introduce a Lock instead of relying on Flow.