Merge pull request #14128 from jordan-woyak/fix-run-on-object-race

DolphinQt/QtUtils: Simplify RunOnObject and eliminate Common::Event race. Introduce Common::OneShotEvent class.

Source/Core/Common/CMakeLists.txt

@@ -118,6 +118,7 @@ add_library(common
   NandPaths.h
   Network.cpp
   Network.h
+  OneShotEvent.h
   PcapFile.cpp
   PcapFile.h
   Profiler.cpp

Source/Core/Common/Functional.h

@@ -4,6 +4,7 @@
 #pragma once
 
 #include <concepts>
+#include <functional>
 #include <memory>
 #include <type_traits>
 
@@ -51,4 +52,17 @@ private:
   std::unique_ptr<FuncBase> m_ptr;
 };
 
+// A functor type with an invocable non-type template parameter.
+// e.g. Providing a function pointer will create a functor type that invokes said function.
+// It allows using function pointers in contexts that expect a type, e.g. as a "deleter".
+template <auto Invocable>
+struct InvokerOf
+{
+  template <typename... Args>
+  constexpr auto operator()(Args... args) const
+  {
+    return std::invoke(Invocable, std::forward<Args>(args)...);
+  }
+};
+
 }  // namespace Common

Source/Core/Common/OneShotEvent.h

@@ -0,0 +1,55 @@
+// Copyright 2025 Dolphin Emulator Project
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#pragma once
+
+#include <chrono>
+#include <memory>
+#include <semaphore>
+
+#include "Common/Functional.h"
+
+namespace Common
+{
+
+// A one-time-use single-producer single-consumer thread synchronization class.
+// Safe when `Set` will cause a `Wait`ing thread to immediately destruct the event itself.
+class OneShotEvent
+{
+public:
+  // One thread should call Set() exactly once.
+  void Set() { m_semaphore.release(1); }
+
+  // One thread may call Wait() once or WaitFor() until it returns true.
+
+  void Wait() { m_semaphore.acquire(); }
+
+  template <typename Rep, typename Period>
+  bool WaitFor(const std::chrono::duration<Rep, Period>& rel_time)
+  {
+    return m_semaphore.try_acquire_for(rel_time);
+  }
+
+private:
+  std::binary_semaphore m_semaphore{0};
+};
+
+// Invokes Set() on the given object upon destruction.
+template <typename EventType>
+class ScopedSetter
+{
+public:
+  ScopedSetter() = default;
+  explicit ScopedSetter(EventType* ptr) : m_ptr{ptr} {}
+
+  // Forgets the object without invoking Set().
+  void Dismiss() { m_ptr.release(); }
+
+private:
+  // Leveraging unique_ptr conveniently makes this class move-only.
+  // It does no actual deletion, just calls Set().
+  using NonOwningSetOnDeletePtr = std::unique_ptr<EventType, InvokerOf<&EventType::Set>>;
+  NonOwningSetOnDeletePtr m_ptr;
+};
+
+}  // namespace Common

Source/Core/Common/ScopeGuard.h

@@ -13,10 +13,7 @@ class ScopeGuard final
 public:
   ScopeGuard(Callable&& finalizer) : m_finalizer(std::forward<Callable>(finalizer)) {}
 
-  ScopeGuard(ScopeGuard&& other) : m_finalizer(std::move(other.m_finalizer))
-  {
-    other.m_finalizer = nullptr;
-  }
+  ScopeGuard(ScopeGuard&& other) : m_finalizer(std::move(other.m_finalizer)) { other.Dismiss(); }
 
   ~ScopeGuard() { Exit(); }
   void Dismiss() { m_finalizer.reset(); }

Source/Core/DolphinLib.props

@@ -149,6 +149,7 @@
     <ClInclude Include="Common\Mutex.h" />
     <ClInclude Include="Common\NandPaths.h" />
     <ClInclude Include="Common\Network.h" />
+    <ClInclude Include="Common\OneShotEvent.h" />
     <ClInclude Include="Common\PcapFile.h" />
     <ClInclude Include="Common\Profiler.h" />
     <ClInclude Include="Common\Projection.h" />

Source/Core/DolphinQt/QtUtils/RunOnObject.h

@@ -3,74 +3,34 @@
 
 #pragma once
 
-#include <QCoreApplication>
-#include <QEvent>
-#include <QPointer>
-#include <QThread>
+#include <concepts>
+#include <functional>
 #include <optional>
-#include <type_traits>
-#include <utility>
 
-#include "Common/Event.h"
-#include "DolphinQt/QtUtils/QueueOnObject.h"
+#include <QMetaObject>
 
-class QObject;
+#include "Common/OneShotEvent.h"
 
 // QWidget and subclasses are not thread-safe! This helper takes arbitrary code from any thread,
 // safely runs it on the appropriate GUI thread, waits for it to finish, and returns the result.
 //
-// If the target object is destructed before the code gets to run, the QPointer will be nulled and
-// the function will return nullopt.
+// If the target object is destructed before the code gets to run the function will return nullopt.
 
-template <typename F>
-auto RunOnObject(QObject* object, F&& functor)
+auto RunOnObject(QObject* object, std::invocable<> auto&& functor)
 {
-  using OptionalResultT = std::optional<std::invoke_result_t<F>>;
+  Common::OneShotEvent task_complete;
+  std::optional<decltype(functor())> result;
 
-  // If we queue up a functor on the current thread, it won't run until we return to the event loop,
-  // which means waiting for it to finish will never complete. Instead, run it immediately.
-  if (object->thread() == QThread::currentThread())
-    return OptionalResultT(functor());
+  QMetaObject::invokeMethod(
+      object, [&, guard = Common::ScopedSetter{&task_complete}] { result = functor(); });
 
-  class FnInvokeEvent : public QEvent
-  {
-  public:
-    FnInvokeEvent(F&& functor, QObject* obj, Common::Event& event, OptionalResultT& result)
-        : QEvent(QEvent::None), m_func(std::move(functor)), m_obj(obj), m_event(event),
-          m_result(result)
-    {
-    }
-
-    ~FnInvokeEvent()
-    {
-      if (m_obj)
-      {
-        m_result = m_func();
-      }
-      else
-      {
-        // is already nullopt
-      }
-      m_event.Set();
-    }
-
-  private:
-    F m_func;
-    QPointer<QObject> m_obj;
-    Common::Event& m_event;
-    OptionalResultT& m_result;
-  };
-
-  Common::Event event{};
-  OptionalResultT result = std::nullopt;
-  QCoreApplication::postEvent(object,
-                              new FnInvokeEvent(std::forward<F>(functor), object, event, result));
-  event.Wait();
+  // Wait for the lambda to go out of scope. The result may or may not have been assigned.
+  task_complete.Wait();
   return result;
 }
 
-template <typename Base, typename Type, typename Receiver>
-auto RunOnObject(Receiver* obj, Type Base::* func)
+template <std::derived_from<QObject> Receiver>
+auto RunOnObject(Receiver* obj, auto Receiver::* func)
 {
-  return RunOnObject(obj, [obj, func] { return (obj->*func)(); });
+  return RunOnObject(obj, std::bind(func, obj));
 }