From d89720a2f04466a7a6f615db30712815f33e54cb Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Tue, 2 May 2023 11:09:00 -0700 Subject: [PATCH] Allow registering notifiers in write transactions prior to the first change (#6560) This fixes one of the common problems resulting from https://github.com/realm/realm-swift/issues/4818, as it makes it so that adding new callbacks from within a notification callback will _usually_ work even if the notification was triggered by beginning a write transaction. It still will not work if a previously invoked callback made any changes (which would be a very strange thing to do, but possible). --- CHANGELOG.md | 2 +- src/realm/object-store/shared_realm.cpp | 16 +++++++---- src/realm/transaction.cpp | 20 +++++++++---- test/object-store/results.cpp | 37 +++++++++++++++++++++++-- 4 files changed, 61 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ff4c95650b..cb6d526a7ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### Enhancements * (PR [#????](https://github.com/realm/realm-core/pull/????)) -* None. +* New notifiers can now be registered in write transactions until changes have actually been made in the write transaction. This makes it so that new notifications can be registered inside change notifications triggered by beginning a write transaction (unless a previous callback performed writes). ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) diff --git a/src/realm/object-store/shared_realm.cpp b/src/realm/object-store/shared_realm.cpp index 43fc4e860b4..4a33abd4567 100644 --- a/src/realm/object-store/shared_realm.cpp +++ b/src/realm/object-store/shared_realm.cpp @@ -620,10 +620,16 @@ bool Realm::verify_notifications_available(bool throw_on_error) const throw WrongTransactionState("Cannot create asynchronous query for immutable Realms"); return false; } - if (is_in_transaction()) { - if (throw_on_error) - throw WrongTransactionState("Cannot create asynchronous query while in a write transaction"); - return false; + if (throw_on_error) { + if (m_transaction && m_transaction->get_commit_size() > 0) + throw WrongTransactionState( + "Cannot create asynchronous query after making changes in a write transaction."); + } + else { + // Don't create implicit notifiers inside write transactions even if + // we could as it wouldn't actually be used + if (is_in_transaction()) + return false; } return true; @@ -1310,7 +1316,7 @@ uint64_t Realm::get_schema_version(const Realm::Config& config) bool Realm::is_frozen() const { bool result = bool(m_frozen_version); - REALM_ASSERT_DEBUG((result && m_transaction) ? m_transaction->is_frozen() : true); + REALM_ASSERT_DEBUG(!result || !m_transaction || m_transaction->is_frozen()); return result; } diff --git a/src/realm/transaction.cpp b/src/realm/transaction.cpp index d2de3c0adf0..8d3f33374eb 100644 --- a/src/realm/transaction.cpp +++ b/src/realm/transaction.cpp @@ -322,12 +322,20 @@ TransactionRef Transaction::freeze() TransactionRef Transaction::duplicate() { auto version = VersionID(m_read_lock.m_version, m_read_lock.m_reader_idx); - if (m_transact_stage == DB::transact_Reading) - return db->start_read(version); - if (m_transact_stage == DB::transact_Frozen) - return db->start_frozen(version); - - throw WrongTransactionState("Can only duplicate a read/frozen transaction"); + switch (m_transact_stage) { + case DB::transact_Ready: + throw WrongTransactionState("Cannot duplicate a transaction which does not have a read lock."); + case DB::transact_Reading: + return db->start_read(version); + case DB::transact_Frozen: + return db->start_frozen(version); + case DB::transact_Writing: + if (get_commit_size() != 0) + throw WrongTransactionState( + "Can only duplicate a write transaction before any changes have been made."); + return db->start_read(version); + } + REALM_UNREACHABLE(); } void Transaction::copy_to(TransactionRef dest) const diff --git a/test/object-store/results.cpp b/test/object-store/results.cpp index 9f4e9976018..f52dc725bd1 100644 --- a/test/object-store/results.cpp +++ b/test/object-store/results.cpp @@ -403,16 +403,49 @@ TEST_CASE("notifications: async delivery") { SECTION("Results which already has callbacks") { SECTION("notifier before active") { - token4 = results2.add_notification_callback([&](CollectionChangeSet) {}); + token4 = results2.add_notification_callback([](CollectionChangeSet) {}); check(results3, results2); } SECTION("notifier after active") { - token4 = results2.add_notification_callback([&](CollectionChangeSet) {}); + token4 = results2.add_notification_callback([](CollectionChangeSet) {}); check(results, results2); } } } + SECTION("callbacks can be added from within callbacks triggered by beginning a write transaction") { + auto results2 = results; + auto results3 = results; + + bool called = false; + NotificationToken token2, token3; + token2 = results2.add_notification_callback([&](CollectionChangeSet) { + token2 = {}; + token3 = results3.add_notification_callback([&](CollectionChangeSet) { + called = true; + }); + }); + r->begin_transaction(); + REQUIRE_FALSE(called); + r->cancel_transaction(); + r->begin_transaction(); + REQUIRE(called); + r->cancel_transaction(); + } + + SECTION("callbacks can be added inside write transactions but only before any changes have been made") { + auto results2 = results; + auto results3 = results; + r->begin_transaction(); + REQUIRE_NOTHROW(results2.add_notification_callback([](CollectionChangeSet) {})); + table->begin()->remove(); + // Works because it already has a notifier + REQUIRE_NOTHROW(results2.add_notification_callback([](CollectionChangeSet) {})); + // Fails because we're in a state where we can't create a notifier + REQUIRE_EXCEPTION(results3.add_notification_callback([](CollectionChangeSet) {}), WrongTransactionState, + "Cannot create asynchronous query after making changes in a write transaction."); + } + SECTION("remote changes made before adding a callback from within a callback are not reported") { NotificationToken token2, token3; bool called = false;