Deliver appropriate subscription state change notifications in DiscardLocal client resets #7119
Conversation
| /// | ||
| /// This function is guaranteed to not be called before activation, and also | ||
| /// not after initiation of deactivation. | ||
| ClientReplication& access_realm(); |
There was a problem hiding this comment.
access_realm() should have gone away in #4839, as it was part of the MRU cache that the sync client used to have. These days it was just confusing and weird (and claimed to do something quite different from what it did).
| // we don't mind leaving the fresh lock file around because trying to delete it | ||
| // here could cause a race if there are multiple resets ongoing |
There was a problem hiding this comment.
All this code breaks horribly in any scenario where deleting the lockfile would be a problem. The sync agent claiming on the parent realm is supposed to ensure only one process can be performing a client reset on the realm, and if that's violated very bad things happen, so we don't really need to account for that possiblity.
| // If we've already been superceded by another version getting completed, then we should skip registering | ||
| // a notification because it may never fire. | ||
| if (mgr->m_min_outstanding_version > version()) { | ||
| return util::Future<State>::make_ready(State::Superseded); | ||
| } | ||
|
|
||
| // Begin by blocking process_notifications from starting to fill futures. No matter the outcome, we'll | ||
| // unblock process_notifications() at the end of this function via the guard we construct below. | ||
| mgr->m_outstanding_requests++; |
There was a problem hiding this comment.
It appears that the goal of this semaphore was to allow multiple concurrent calls to get_state_change_notification() to be refreshing at the same time. That doesn't seem like a very important thing to optimize for, and waiting on a condition variable is much more expensive than waiting on a mutex.
| return mut.commit(); | ||
| } | ||
|
|
||
| TEST(ClientReset_DiscardLocal_DiscardsPendingSubscriptions) |
There was a problem hiding this comment.
These two tests are the ones which were failing without the change to set_active_as_latest().
Pull Request Test Coverage Report for Build github_pull_request_283788
💛 - Coveralls |
| m_logger.debug("ClientResetOperation::finalize, realm_path = %1, local_realm_exists = %2, mode = %3", | ||
| m_db->get_path(), local_realm_exists, m_mode); | ||
| auto latest_version = db.get_version_id_of_latest_snapshot(); | ||
| bool local_realm_exists = latest_version.version > 1; |
There was a problem hiding this comment.
shouldn't this check be >=?
There was a problem hiding this comment.
No, the old check was incorrect. The version number for a never-written-to file is 1 and the first write produces version 2.
| { | ||
| util::CheckedLockGuard lk(mgr->m_pending_notifications_mutex); | ||
| splice_if(mgr->m_pending_notifications, to_finish, [&](auto& req) { | ||
| return (req.version == m_version && |
There was a problem hiding this comment.
should either use m_version or my_version everywhere.
tgoyne
force-pushed
the
tg/client-reset-flx-notifications
branch
from
3abd6a7 to
c933533
Compare
…dLocal client resets
tgoyne
force-pushed
the
tg/client-reset-flx-notifications
branch
from
c933533 to
9e5d133
Compare
#7110 accidentally made it so that we no longer complete subscription state notifications in DiscardLocal client resets. I uncovered this while writing tests for a separate set of changes, so this includes some refactoring which those tests depend on.
No changelog entry since the bug hasn't been shipped.