Fix a race condition in Promise (#7602)

Promise needs to update `m_shared_state` before fulfilling the promise, as
`Future::get()` on another thread could return before the write occurs. If the
Promise is destroyed immediately after `get()` returns, the destructor could
then read the non-null value of `m_shared_state` before the write happens and
attempt to report a broken promise error, which hits an assertion.

Author: tgoyne

CHANGELOG.md

@@ -48,6 +48,7 @@
 * The metadata disabled mode has been replaced with an in-memory metadata mode which performs similarly and doesn't work weirdly differently from the normal mode. The new mode is intended for testing purposes, but should be suitable for production usage if there is a scenario where metadata persistence is not needed. ([PR #7300](https://github.com/realm/realm-core/pull/7300).
 * The ownership relationship between App and User has changed. User now strongly retains App and App has a weak cache of Users. This means that creating a SyncConfig or opening a Realm will keep the parent App alive, rather than things being in a broken state if the App is deallocated. ([PR #7300](https://github.com/realm/realm-core/pull/7300).
 * A new CMake define `REALM_APP_SERVICES` can be used to compile out core's default implmentation of the application services. ([#7268](https://github.com/realm/realm-core/issues/7268))
+* Fix a race condition in Promise which could result in an assertion failure if it was destroyed immediately after a `get()` on the Future returned. The problematic scenario only occurred in test code and not in library code ([PR #7602](https://github.com/realm/realm-core/pull/7602)).
 
 ----------------------------------------------
 

src/realm/util/future.hpp

@@ -492,24 +492,24 @@ class future_details::Promise {
 
     void set_from_status_with(StatusWith<T> sw) noexcept
     {
-        set_impl([&] {
-            m_shared_state->set_from(std::move(sw));
+        set_impl([&](auto& shared_state) {
+            shared_state.set_from(std::move(sw));
         });
     }
 
     template <typename... Args>
     void emplace_value(Args&&... args) noexcept
     {
-        set_impl([&] {
-            m_shared_state->emplace_value(std::forward<Args>(args)...);
+        set_impl([&](auto& shared_state) {
+            shared_state.emplace_value(std::forward<Args>(args)...);
         });
     }
 
     void set_error(Status status) noexcept
     {
         REALM_ASSERT_DEBUG(!status.is_ok());
-        set_impl([&] {
-            m_shared_state->set_status(std::move(status));
+        set_impl([&](auto& shared_state) {
+            shared_state.set_status(std::move(status));
         });
     }
 
@@ -547,8 +547,10 @@ class future_details::Promise {
     void set_impl(Func&& do_set) noexcept
     {
         REALM_ASSERT(m_shared_state);
-        do_set();
-        m_shared_state.reset();
+        // Update m_shared_state before fulfilling the promise so that anything
+        // waiting on the future will see `m_shared_state = nullptr`.
+        auto shared_state = std::move(m_shared_state);
+        do_set(*shared_state);
     }
 
     util::bind_ptr<SharedState<T>> m_shared_state = make_intrusive<SharedState<T>>();
@@ -1270,8 +1272,8 @@ inline Future<T> Promise<T>::get_future() noexcept
 template <typename T>
 inline void Promise<T>::set_from(Future<T>&& future) noexcept
 {
-    set_impl([&] {
-        std::move(future).propagate_result_to(m_shared_state.get());
+    set_impl([&](auto& shared_state) {
+        std::move(future).propagate_result_to(&shared_state);
     });
 }
 

test/test_util_future.cpp

@@ -2235,6 +2235,17 @@ TEST(Future_Void_Fail_onCompletionFutureAsync)
     });
 }
 
+TEST(Future_PromiseUpdatesSharedStateBeforeCompletingCallbacks)
+{
+    auto pf = util::make_promise_future<void>();
+    std::move(pf.future).get_async([&](Status) {
+        // If pf.promise != nullptr here this will hit an assertion failure
+        // when `promise` is destroyed
+        auto promise = std::move(pf.promise);
+    });
+    pf.promise.emplace_value();
+}
+
 } // namespace
 } // namespace realm::util