Fix a data race in notifier packaging

CHANGELOG.md

@@ -6,8 +6,8 @@
 
 ### Fixed
 * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
-* None.
- 
+* Fix a data race reported by thread sanitizer when preparing to deliver change notifications. This probably did not cause observable problems in practice ([PR #5892](https://github.com/realm/realm-core/pull/5892) since 12.7.0).
+
 ### Breaking changes
 * None.
 

src/realm/object-store/impl/collection_notifier.cpp

@@ -413,28 +413,23 @@ NotifierPackage::NotifierPackage(std::vector<std::shared_ptr<CollectionNotifier>
 {
 }
 
-NotifierPackage::NotifierPackage(std::vector<std::shared_ptr<CollectionNotifier>> notifiers)
-    : m_notifiers(std::move(notifiers))
+NotifierPackage::NotifierPackage(std::vector<std::shared_ptr<CollectionNotifier>> notifiers,
+                                 std::optional<VersionID> version)
+    : m_version(version)
+    , m_notifiers(std::move(notifiers))
 {
-    set_version();
 }
 
 void NotifierPackage::package_and_wait(VersionID::version_type target_version)
 {
     if (!m_coordinator || !*this)
         return;
 
-    m_coordinator->package_notifiers(m_notifiers, target_version);
-    set_version();
+    m_version = m_coordinator->package_notifiers(m_notifiers, target_version);
     if (m_version && m_version->version < target_version)
         m_version = util::none;
 }
 
-void NotifierPackage::set_version()
-{
-    m_version = m_notifiers.empty() ? util::none : std::make_optional(m_notifiers.front()->version());
-}
-
 void NotifierPackage::before_advance()
 {
     for (auto& notifier : m_notifiers)

src/realm/object-store/impl/collection_notifier.hpp

@@ -330,7 +330,7 @@ class NotifierPackage {
     NotifierPackage() = default;
 
     // Create a package which contains notifiers which have already been pacakged for delivery
-    NotifierPackage(std::vector<std::shared_ptr<CollectionNotifier>> notifiers);
+    NotifierPackage(std::vector<std::shared_ptr<CollectionNotifier>> notifiers, std::optional<VersionID> version);
     // Create a package which can have package_and_wait() called on it later
     NotifierPackage(std::vector<std::shared_ptr<CollectionNotifier>> notifiers, RealmCoordinator* coordinator);
 
@@ -362,8 +362,6 @@ class NotifierPackage {
     util::Optional<VersionID> m_version;
     std::vector<std::shared_ptr<CollectionNotifier>> m_notifiers;
     RealmCoordinator* m_coordinator = nullptr;
-
-    void set_version();
 };
 
 } // namespace realm::_impl

src/realm/object-store/impl/realm_coordinator.cpp

@@ -1172,7 +1172,8 @@ void RealmCoordinator::advance_to_ready(Realm& realm)
     }
 
     // We have notifiers for a newer version, so advance to that
-    transaction::advance(tr, realm.m_binding_context.get(), std::move(notifiers));
+    transaction::advance(tr, realm.m_binding_context.get(),
+                         _impl::NotifierPackage(std::move(notifiers), notifier_version));
 }
 
 std::vector<std::shared_ptr<_impl::CollectionNotifier>> RealmCoordinator::notifiers_for_realm(Realm& realm)
@@ -1198,11 +1199,11 @@ bool RealmCoordinator::advance_to_latest(Realm& realm)
         util::CheckedUniqueLock lock(m_notifier_mutex);
         notifiers = notifiers_for_realm(realm);
     }
-    package_notifiers(notifiers, m_db->get_version_of_latest_snapshot());
+    auto version = package_notifiers(notifiers, m_db->get_version_of_latest_snapshot());
 
-    auto version = tr->get_version_of_current_transaction();
-    transaction::advance(tr, realm.m_binding_context.get(), _impl::NotifierPackage(std::move(notifiers)));
-    return !realm.is_closed() && version != tr->get_version_of_current_transaction();
+    auto prev_version = tr->get_version_of_current_transaction();
+    transaction::advance(tr, realm.m_binding_context.get(), _impl::NotifierPackage(std::move(notifiers), version));
+    return !realm.is_closed() && prev_version != tr->get_version_of_current_transaction();
 }
 
 void RealmCoordinator::promote_to_write(Realm& realm)
@@ -1257,7 +1258,8 @@ void RealmCoordinator::process_available_async(Realm& realm)
         realm.m_binding_context->did_send_notifications();
 }
 
-void RealmCoordinator::package_notifiers(NotifierVector& notifiers, VersionID::version_type target_version)
+std::optional<VersionID> RealmCoordinator::package_notifiers(NotifierVector& notifiers,
+                                                             VersionID::version_type target_version)
 {
     auto ready = [&] {
         util::CheckedUniqueLock notifier_lock(m_notifier_mutex);
@@ -1280,6 +1282,7 @@ void RealmCoordinator::package_notifiers(NotifierVector& notifiers, VersionID::v
                notifier->version().version < target_version;
     };
     notifiers.erase(std::remove_if(begin(notifiers), end(notifiers), package), end(notifiers));
+    return notifiers.empty() ? std::optional<VersionID>{} : notifiers.front()->version();
 }
 
 bool RealmCoordinator::compact()

src/realm/object-store/impl/realm_coordinator.hpp

@@ -207,7 +207,7 @@ class RealmCoordinator : public std::enable_shared_from_this<RealmCoordinator> {
     // Called by NotifierPackage in the cases where we don't know what version
     // we need notifiers for until after we begin advancing (e.g. when
     // starting a write transaction).
-    void package_notifiers(NotifierVector& notifiers, VersionID::version_type)
+    std::optional<VersionID> package_notifiers(NotifierVector& notifiers, VersionID::version_type)
         REQUIRES(!m_notifier_mutex, !m_running_notifiers_mutex);
 
     // testing hook only to verify that notifiers are not being run at times