Make explicitly pausing the sync session a separate state #6183
Conversation
…y_pausing_resuming_sync
…y_pausing_resuming_sync
| // Changing the state releases the lock, which means that by the | ||
| // time we reacquire the lock the state may have changed again | ||
| // (either due to one of the callbacks being invoked or another | ||
| // thread coincidentally doing something). We just attempt to keep | ||
| // switching it to inactive until it stays there. | ||
| become_inactive(std::move(state_lock)); | ||
| if (m_state != State::Paused) { |
| case State::WaitingForAccessToken: | ||
| become_paused(std::move(lock)); | ||
| break; | ||
| case State::Inactive: |
There was a problem hiding this comment.
Is it correct to stay in Inactive here? This'd mean that pause() -> revive_if_needed() does sometimes result in an active session.
There was a problem hiding this comment.
No, it's not correct. become_paused() needs to check if the old state was inactive and early return in this case.
| case State::WaitingForAccessToken: | ||
| become_paused(std::move(lock)); | ||
| break; | ||
| case State::Inactive: |
There was a problem hiding this comment.
No, it's not correct. become_paused() needs to check if the old state was inactive and early return in this case.
| // Changing the state releases the lock, which means that by the | ||
| // time we reacquire the lock the state may have changed again | ||
| // (either due to one of the callbacks being invoked or another | ||
| // thread coincidentally doing something). We just attempt to keep | ||
| // switching it to inactive until it stays there. | ||
| become_inactive(std::move(state_lock)); | ||
| if (m_state != State::Paused) { |
|
If I understand the changes correctly it's completely fine to merge; we can preserve the old behavior by continuing to call force_close()/revive_if_needed() in suspend() and resume() while figuring out how handle the switch. I think maybe we'd call |
…y_pausing_resuming_sync
…y_pausing_resuming_sync
| @@ -970,6 +1028,9 @@ void SyncSession::close(util::CheckedUniqueLock lock) | |||
| case State::Dying: | |||
| m_state_mutex.unlock(lock); | |||
| break; | |||
| case State::Paused: | |||
| m_state_mutex.unlock(lock); | |||
There was a problem hiding this comment.
Would you mind adding a comment here that mentions paused is supposed to be sticky and that state should not be changed to inactive.
What, How & Why?
This makes the pausing of
SyncSession's a separate state than beingInactivethat is functionally the same except for being "sticky" until the user explicitly resumes the sync session. There are a bunch of places/rules where we revive a sync session besides the user explicitly callingresume(), and it can be confusing to users to pause a sync session and then have it resume unexpectedly because of some seemingly unrelated action (like freezing a realm).This also renames
SyncSession::log_out()toSyncSession::force_close()since the function's behavior doesn't actually have anything to do with authentication and is really just the behavior ofclose()if close ignored theStopPolicyof the realm'sSyncConfig☑️ ToDos
log_out()/revive_if_needed())