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

Do not cancel the current sync request when the app goes to background #5719

Merged
merged 5 commits into from
Apr 12, 2022

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Apr 7, 2022

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:

  • on a big account, go to bg
  • start the app
  • observe that a incr sync is started (show debug info setting can help)
  • go the bg
  • previously the current incr sync was cancelled
  • now the treatment continues
  • if the user put the app in fg, we can see the current sync treatment is still in progress
  • if the user wait a long time, the current sync treatment can end in bg
  • the sync thread does not start another sync request when the app stay in bg

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.

@bmarty bmarty force-pushed the feature/bma/improved_logs branch from 7cfa388 to ea65547 Compare April 7, 2022 11:54
@bmarty bmarty requested review from a team and ericdecanini and removed request for a team April 7, 2022 11:54
@bmarty bmarty changed the title Feature/bma/improved logs Do not cancel the current sync request when the app goes to background Apr 7, 2022
@github-actions
Copy link

github-actions bot commented Apr 7, 2022

Unit Test Results

114 files  ±0  114 suites  ±0   1m 16s ⏱️ -15s
201 tests ±0  201 ✔️ ±0  0 💤 ±0  0 ±0 
674 runs  ±0  674 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 03d6aa8. ± Comparison against base commit bd3e980.

♻️ This comment has been updated with latest results.

@@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

Why do you remove reaction here? it can be helpful to see which one is being synced

Copy link
Member Author

Choose a reason for hiding this comment

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

For privacy reason

// 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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 bmarty force-pushed the feature/bma/improved_logs branch from ea65547 to 03d6aa8 Compare April 8, 2022 14:01
@bmarty bmarty merged commit 836a12d into develop Apr 12, 2022
@bmarty bmarty deleted the feature/bma/improved_logs branch April 12, 2022 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sync request parsing takes a long time (can be more than 30s)
4 participants