Do not cancel the current sync request when the app goes to background #5719
Conversation
bmarty
force-pushed
the
feature/bma/improved_logs
branch
from
7cfa388
to
ea65547
Compare
bmarty
requested review from
a team and
ericdecanini
and removed request for
a team
bmarty
changed the title
| @@ -584,11 +584,11 @@ internal class EventRelationsAggregationProcessor @Inject constructor( | |||
| sum.key = reaction | |||
| sum.firstTimestamp = event.originServerTs ?: 0 | |||
| if (isLocalEcho) { | |||
| Timber.v("Adding local echo reaction $reaction") | |||
| Timber.v("Adding local echo reaction") | |||
There was a problem hiding this comment.
Why do you remove reaction here? it can be helpful to see which one is being synced
| // Incremental sync can be long and it requires the user to wait for the treatment to end, | ||
| // else all is restarted from the beginning each time the user moves the app to foreground. | ||
| Timber.tag(loggerTag.value).d("Pause sync... Not cancelling incremental sync") | ||
| // syncScope.coroutineContext.cancelChildren() |
There was a problem hiding this comment.
Fix looks good but I'd rather remove the offending line than simply comment it out. We always have git history if we need to bring it back, but commented code is a code smell
There was a problem hiding this comment.
I agree on the principle, also sonar for instance complain about commented code. But git history is less visible than a commented out line of code. Also sometimes the history can be lost during rework (if not done correctly).
So I prefer to comment the line.
Also in case of trouble it's easier to see the change and to revert.
There was a problem hiding this comment.
I would also be in the removing the line camp, if we want to protect against regressions a unit test to replace the comment which asserts that pauses do not cancel inprogress syncs would be stronger
although with the upcoming rust SDK changes I'm not sure it would be the best use of time to add tests here~
if context/history is important I would be for linking to the PR rather than commenting the line
There was a problem hiding this comment.
OK. I have force push the last commit, so that it reduces noise. Also note that the commit contains the changelog file, which links to the PR :).
Incremental sync can be long and it requires the user to wait for the treatment to end, else all is restarted from the beginning each time the user moves the app to foreground.
bmarty
force-pushed
the
feature/bma/improved_logs
branch
from
ea65547
to
03d6aa8
Compare
The important change is in 7cfa388 : we do not cancel the current sync request when the app goes to background, to give a chance to the current sync, or the current sync response treatment to finish.
Test:
According to my tests, this is all fine.
This PR also contains some other minor changes regarding logs.
Closes #5621 . We will not investigate more the sync duration since it will be replaced by the Rust implementation in a near future.