RCORE-2191 Add support for multi-process subscription state change notifications #7862

tgoyne · 2024-07-04T00:21:48Z

As with the other multi-process notifications, the core idea here is to eliminate the in-memory state and produce notifications based entirely on the current state of the Realm file.

SubscriptionStore::update_state() has been replaced with separate functions for the specific legal state transitions, which also take a write transaction as a parameter. These functions are called by PendingBootstrapStore inside the same write transaction as the bootstrap updates which changed the subscription state. This is both a minor performance optimization (due to fewer writes) and eliminates a brief window between the two writes where the Realm file was in an inconsistent state.

There's a minor functional change here: previously old subscription sets were superseded when the new one reached the Completed state, and now they are superseded on AwaitingMark. This aligns it with when the new subscription set becomes the one which is returned by get_active().

Fixes #7861.

tgoyne · 2024-07-04T02:08:13Z

src/realm/sync/noinst/client_impl_base.cpp

-    if (auto status = sess->receive_query_error_message(raw_error_code, message, query_version); !status.is_ok()) {
-        close_due_to_protocol_error(std::move(status));
+    if (Session* sess = find_and_validate_session(session_ident, "QUERY_ERROR")) {
+        sess->receive_query_error_message(raw_error_code, message, query_version);


receive_query_error_message() only ever returned Status::Ok() the code handling it returning an error was dead.

tgoyne · 2024-07-04T02:09:24Z

src/realm/sync/noinst/client_impl_base.cpp

-    // Ignore the message if the deactivation process has been initiated,
-    // because in that case, the associated Realm and SessionWrapper must
-    // not be accessed any longer.
-    if (m_state == Active) {


Checking for m_state == Active was done several time along the way so I eliminated all but one.

tgoyne · 2024-07-04T02:10:41Z

src/realm/sync/noinst/pending_bootstrap_store.cpp

 {
-    auto tr = m_db->start_read();
-    auto bootstrap_table = tr->get_table(m_table);
-    if (bootstrap_table->is_empty()) {


peek_pending() and pop_front_pending() are only called if has_pending() returns true, so the table should never be empty in either of them.

tgoyne · 2024-07-04T02:11:01Z

src/realm/sync/noinst/pending_bootstrap_store.cpp

@@ -245,7 +240,7 @@ PendingBootstrapStore::PendingBatch PendingBootstrapStore::peek_pending(size_t l
            progress_obj.get<int64_t>(m_progress_download_client_version);
        progress.upload.last_integrated_server_version = progress_obj.get<int64_t>(m_progress_upload_server_version);
        progress.upload.client_version = progress_obj.get<int64_t>(m_progress_upload_client_version);
-        ret.progress = std::move(progress);
+        ret.progress = progress;


SyncProgress is trivially copyable so moving it doesn't do anything.

tgoyne · 2024-07-04T02:11:24Z

src/realm/sync/noinst/pending_bootstrap_store.hpp


 private:
    DBRef m_db;
    // The pending_bootstrap_store is tied to the lifetime of a session, so a shared_ptr is not needed
    util::Logger& m_logger;
-    _impl::ClientProtocol m_client_protocol;


This has been unused for a while.

tgoyne · 2024-07-04T02:12:55Z

src/realm/sync/subscriptions.cpp

@@ -548,7 +558,7 @@ SubscriptionSet MutableSubscriptionSet::commit()
    const auto flx_version = version();
    m_tr->commit_and_continue_as_read();

-    mgr->process_notifications(m_state, flx_version, std::string_view(error_str()));
+    mgr->report_progress();


Notifications will be sent without this call, but the timing would change and that maybe could be a problem.

tgoyne · 2024-07-04T02:13:56Z

test/test_sync_pending_bootstraps.cpp

    CHECK_NOT(store.has_pending());
-    pending_batch = store.peek_pending(1024);


This is now an assertion failure rather than returning an empty result.

michael-wb · 2024-07-18T15:38:08Z

src/realm/sync/client.cpp

-        m_sess->logger.debug("Marking query version %1 as complete after receiving MARK message",
-                             m_flx_pending_mark_version);


It would be nice to keep this debug message to show when the bootstrap/version is complete. Perhaps move it to be printed out in download_complete().

michael-wb · 2024-07-18T17:24:24Z

src/realm/sync/subscriptions.hpp

+    // Mark query_version as having begun bootstrapping. This should be called
+    // inside the write transaction used to store the first set of changesets.
+    //
+    // This should only be called internally within the sync client.
+    void begin_bootstrap(const Transaction&, int64_t query_version);
+    // Mark query_version as having completed bootstrapping. This should be
+    // called inside the write transaction which removes the final pending changeset.
+    //
+    // This should only be called internally within the sync client.
+    void complete_bootstrap(const Transaction&, int64_t query_version);
+    // Roll query_version back to the Pending state if it is currently Bootstrapping.
+    // Has no effect if the bootstrap in progress is not the first boostrap for
+    // this subscription set.
    //
    // This should only be called internally within the sync client.
-    void update_state(int64_t version_id, SubscriptionSet::State state,
-                      std::optional<std::string_view> error_str = util::none)
-        REQUIRES(!m_pending_notifications_mutex);
+    void cancel_bootstrap(const Transaction&, int64_t query_version);
+    // Report that a download has completed, meaning that the active subscription
+    // set should advance to the Completed state if it is currently in the
+    // AwaitingMark state. Has no effect if it is in any other state.
+    //
+    // This should only be called internally within the sync client.
+    void download_complete();


Add a comment line about server initiated bootstraps for these 4 functions - e.g.,
// Has no effect if this is a server initiated bootstrap for the active query version.

michael-wb

Overall the changes look good; just two minor comments
This branch needs to be updated to the latest feature/role-change and I'd prefer to get the feature branch merged to master prior to merging these changes.

coveralls-official · 2024-07-21T03:23:17Z

Pull Request Test Coverage Report for Build thomas.goyne_466

Details

459 of 465 (98.71%) changed or added relevant lines in 13 files are covered.
60 unchanged lines in 18 files lost coverage.
Overall coverage increased (+0.03%) to 91.095%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/realm/sync/subscriptions.cpp	142	148	95.95%

Files with Coverage Reduction	New Missed Lines	%
src/realm/array_string.cpp	1	88.03%
src/realm/index_string.hpp	1	93.48%
src/realm/list.cpp	1	87.52%
src/realm/query_engine.hpp	1	93.94%
src/realm/query_expression.hpp	1	93.81%
test/fuzz_tester.hpp	1	57.73%
src/realm/array_blobs_big.cpp	2	98.58%
test/test_lang_bind_helper.cpp	2	93.2%
test/test_sync.cpp	2	93.03%
src/realm/sync/client.cpp	3	91.22%

Totals
Change from base Build 2531:	0.03%
Covered Lines:	216809
Relevant Lines:	238003

💛 - Coveralls

jbreams · 2024-07-29T18:22:50Z

src/realm/sync/subscriptions.cpp

+    if (old_state == State::Complete) {
+        throw RuntimeError(ErrorCodes::SyncProtocolInvariantFailed,
+                           util::format("Received error '%1' for already-completed query version %2. This "
+                                        "may be due to a queryable field being removed in the server-side "


is this even possible?

I don't know if the scenario mentioned in the message is real or if this is just guarding against the server doing something invalid.

jbreams · 2024-07-29T20:24:40Z

src/realm/sync/client.cpp

@@ -1626,15 +1508,34 @@ void SessionWrapper::check_progress()
    REALM_ASSERT(!m_finalized);
    REALM_ASSERT(m_sess);

-    if (!m_progress_handler && m_upload_completion_handlers.empty() && m_sync_completion_handlers.empty())
+    // Check if there's anything which even wants progress or completion information
+    bool has_progress_handler = m_progress_handler && m_reliable_download_progress;


Is there a valid reason outside of the unit tests to not have a progress handler at this point?

No, m_progress_handler is always set when using SyncSession and it's only null in the sync unit tests.

michael-wb

LGTM

As with the other multi-process notifications, the core idea here is to eliminate the in-memory state and produce notifications based entirely on the current state of the Realm file. SubscriptionStore::update_state() has been replaced with separate functions for the specific legal state transitions, which also take a write transaction as a parameter. These functions are called by PendingBootstrapStore inside the same write transaction as the bootstrap updates which changed the subscription state. This is both a minor performance optimization (due to fewer writes) and eliminates a brief window between the two writes where the Realm file was in an inconsistent state. There's a minor functional change here: previously old subscription sets were superseded when the new one reached the Completed state, and now they are superseded on AwaitingMark. This aligns it with when the new subscription set becomes the one which is returned by get_active().

tgoyne self-assigned this Jul 4, 2024

cla-bot bot added the cla: yes label Jul 4, 2024

tgoyne force-pushed the tg/subscription-state-notification branch from 9da38f2 to 876436b Compare July 4, 2024 00:55

tgoyne commented Jul 4, 2024

View reviewed changes

tgoyne marked this pull request as ready for review July 4, 2024 03:42

tgoyne requested review from jbreams and michael-wb July 4, 2024 03:42

tgoyne force-pushed the tg/subscription-state-notification branch from 876436b to 54a10e7 Compare July 9, 2024 22:50

michael-wb reviewed Jul 18, 2024

View reviewed changes

Base automatically changed from feature/role-change to master July 19, 2024 00:39

tgoyne force-pushed the tg/subscription-state-notification branch 3 times, most recently from 7d3c8d8 to dd4488c Compare July 21, 2024 02:46

tgoyne force-pushed the tg/subscription-state-notification branch from dd4488c to 2f366b5 Compare July 22, 2024 02:01

jbreams reviewed Jul 29, 2024

View reviewed changes

jbreams approved these changes Jul 30, 2024

View reviewed changes

michael-wb approved these changes Jul 30, 2024

View reviewed changes

tgoyne force-pushed the tg/subscription-state-notification branch from 2f366b5 to dec4408 Compare July 30, 2024 18:04

tgoyne merged commit 819bb98 into master Jul 30, 2024
44 checks passed

tgoyne deleted the tg/subscription-state-notification branch July 30, 2024 19:11

github-actions bot mentioned this pull request Aug 1, 2024

Prepare for 14.11.2 #7939

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RCORE-2191 Add support for multi-process subscription state change notifications #7862

RCORE-2191 Add support for multi-process subscription state change notifications #7862

tgoyne commented Jul 4, 2024 •

edited

Loading

tgoyne Jul 4, 2024

tgoyne Jul 4, 2024

tgoyne Jul 4, 2024

tgoyne Jul 4, 2024

tgoyne Jul 4, 2024

tgoyne Jul 4, 2024

tgoyne Jul 4, 2024

michael-wb Jul 18, 2024

michael-wb Jul 18, 2024 •

edited

Loading

michael-wb left a comment

coveralls-official bot commented Jul 21, 2024 •

edited

Loading

jbreams Jul 29, 2024

tgoyne Jul 29, 2024

jbreams Jul 29, 2024

tgoyne Jul 29, 2024

michael-wb left a comment

		CHECK_NOT(store.has_pending());
		pending_batch = store.peek_pending(1024);

		m_sess->logger.debug("Marking query version %1 as complete after receiving MARK message",
		m_flx_pending_mark_version);

RCORE-2191 Add support for multi-process subscription state change notifications #7862

RCORE-2191 Add support for multi-process subscription state change notifications #7862

Conversation

tgoyne commented Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-wb Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

michael-wb left a comment

Choose a reason for hiding this comment

coveralls-official bot commented Jul 21, 2024 • edited Loading

Pull Request Test Coverage Report for Build thomas.goyne_466

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-wb left a comment

Choose a reason for hiding this comment

tgoyne commented Jul 4, 2024 •

edited

Loading

michael-wb Jul 18, 2024 •

edited

Loading

coveralls-official bot commented Jul 21, 2024 •

edited

Loading