Merge pull request #11885 from Filoppi/devices_deadlock_fix_v2

Input: Improve Controller Interface devices threading

Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp

@@ -45,6 +45,8 @@ ControllerInterface g_controller_interface;
 // will never interfere with game threads.
 static thread_local ciface::InputChannel tls_input_channel = ciface::InputChannel::Host;
 
+static thread_local bool tls_is_updating_devices = false;
+
 void ControllerInterface::Initialize(const WindowSystemInfo& wsi)
 {
   if (m_is_init)
@@ -122,8 +124,8 @@ void ControllerInterface::RefreshDevices(RefreshReason reason)
   // We lock m_devices_population_mutex here to make everything simpler.
   // Multiple devices classes have their own "hotplug" thread, and can add/remove devices at any
   // time, while actual writes to "m_devices" are safe, the order in which they happen is not. That
-  // means a thread could be adding devices while we are removing them, or removing them as we are
-  // populating them (causing missing or duplicate devices).
+  // means a thread could be adding devices while we are removing them from a different thread,
+  // or removing them as we are populating them (causing missing or duplicate devices).
   std::lock_guard lk_population(m_devices_population_mutex);
 
 #if defined(CIFACE_USE_WIN32) && !defined(CIFACE_USE_XLIB) && !defined(CIFACE_USE_OSX)
@@ -271,6 +273,10 @@ bool ControllerInterface::AddDevice(std::shared_ptr<ciface::Core::Device> device
   if (!m_is_init)
     return false;
 
+  ASSERT_MSG(CONTROLLERINTERFACE, !tls_is_updating_devices,
+             "Devices shouldn't be added within input update calls, there is a risk of deadlock "
+             "if another thread was already here");
+
   std::lock_guard lk_population(m_devices_population_mutex);
 
   {
@@ -328,6 +334,10 @@ void ControllerInterface::RemoveDevice(std::function<bool(const ciface::Core::De
   if (!m_is_init)
     return;
 
+  ASSERT_MSG(CONTROLLERINTERFACE, !tls_is_updating_devices,
+             "Devices shouldn't be removed within input update calls, there is a risk of deadlock "
+             "if another thread was already here");
+
   std::lock_guard lk_population(m_devices_population_mutex);
 
   bool any_removed;
@@ -358,29 +368,48 @@ void ControllerInterface::UpdateInput()
   if (!m_is_init)
     return;
 
-  // TODO: if we are an emulation input channel, we should probably always lock
-  // Prefer outdated values over blocking UI or CPU thread (avoids short but noticeable frame drop)
+  // We add the devices to remove while we still have the "m_devices_mutex" locked.
+  // This guarantees that:
+  // -We won't try to lock "m_devices_population_mutex" while it was already locked and waiting
+  //  for "m_devices_mutex", which would result in dead lock.
+  // -We don't keep shared ptrs on devices and thus unwillingly keep them alive even if somebody
+  //  is currently trying to remove them (and needs them destroyed on the spot).
+  // -If somebody else destroyed them in the meantime, we'll know which ones have been destroyed.
+  std::vector<std::weak_ptr<ciface::Core::Device>> devices_to_remove;
 
-  // Lock this first to avoid deadlock with m_devices_mutex in certain cases (such as a Wii Remote
-  // getting disconnected)
-  if (!m_devices_population_mutex.try_lock())
-    return;
-
-  std::lock_guard population_lock(m_devices_population_mutex, std::adopt_lock);
-
-  if (!m_devices_mutex.try_lock())
-    return;
-
-  std::lock_guard lk(m_devices_mutex, std::adopt_lock);
-
-  for (auto& backend : m_input_backends)
-    backend->UpdateInput();
-
-  for (const auto& d : m_devices)
   {
-    // Theoretically we could avoid updating input on devices that don't have any references to
-    // them, but in practice a few devices types could break in different ways, so we don't
-    d->UpdateInput();
+    // TODO: if we are an emulation input channel, we should probably always lock.
+    // Prefer outdated values over blocking UI or CPU thread (this avoids short but noticeable frame
+    // drops)
+    if (!m_devices_mutex.try_lock())
+      return;
+
+    std::lock_guard lk_devices(m_devices_mutex, std::adopt_lock);
+
+    tls_is_updating_devices = true;
+
+    for (auto& backend : m_input_backends)
+      backend->UpdateInput(devices_to_remove);
+
+    for (const auto& d : m_devices)
+    {
+      // Theoretically we could avoid updating input on devices that don't have any references to
+      // them, but in practice a few devices types could break in different ways, so we don't
+      if (d->UpdateInput() == ciface::Core::DeviceRemoval::Remove)
+        devices_to_remove.push_back(d);
+    }
+
+    tls_is_updating_devices = false;
+  }
+
+  if (devices_to_remove.size() > 0)
+  {
+    RemoveDevice([&](const ciface::Core::Device* device) {
+      return std::any_of(devices_to_remove.begin(), devices_to_remove.end(),
+                         [device](const std::weak_ptr<ciface::Core::Device>& d) {
+                           return d.lock().get() == device;
+                         });
+    });
   }
 }
 

Source/Core/InputCommon/ControllerInterface/ControllerInterface.h

@@ -127,7 +127,6 @@ private:
 
   std::list<std::function<void()>> m_devices_changed_callbacks;
   mutable std::recursive_mutex m_devices_population_mutex;
-  mutable std::mutex m_pre_population_mutex;
   mutable std::mutex m_callbacks_mutex;
   std::atomic<bool> m_is_init;
   // This is now always protected by m_devices_population_mutex, so

Source/Core/InputCommon/ControllerInterface/CoreDevice.h

@@ -36,6 +36,12 @@ constexpr ControlState BATTERY_INPUT_MAX_VALUE = 100.0;
 
 namespace Core
 {
+enum class DeviceRemoval
+{
+  Remove,
+  Keep,
+};
+
 class Device
 {
 public:
@@ -118,7 +124,7 @@ public:
   virtual std::string GetName() const = 0;
   virtual std::string GetSource() const = 0;
   std::string GetQualifiedName() const;
-  virtual void UpdateInput() {}
+  virtual DeviceRemoval UpdateInput() { return DeviceRemoval::Keep; }
 
   // May be overridden to implement hotplug removal.
   // Currently handled on a per-backend basis but this could change.
@@ -242,7 +248,8 @@ public:
   std::recursive_mutex& GetDevicesMutex() const { return m_devices_mutex; }
 
 protected:
-  // Exclusively needed when reading/writing "m_devices"
+  // Exclusively needed when reading/writing the "m_devices" array.
+  // Not needed when individually readring/writing a single device ptr.
   mutable std::recursive_mutex m_devices_mutex;
   std::vector<std::shared_ptr<Device>> m_devices;
 };

Source/Core/InputCommon/ControllerInterface/DInput/DInputJoystick.cpp

@@ -222,7 +222,7 @@ bool Joystick::IsValid() const
   return SUCCEEDED(m_device->Acquire());
 }
 
-void Joystick::UpdateInput()
+Core::DeviceRemoval Joystick::UpdateInput()
 {
   HRESULT hr = 0;
 
@@ -261,6 +261,8 @@ void Joystick::UpdateInput()
   // try reacquire if input lost
   if (DIERR_INPUTLOST == hr || DIERR_NOTACQUIRED == hr)
     m_device->Acquire();
+
+  return Core::DeviceRemoval::Keep;
 }
 
 // get name

Source/Core/InputCommon/ControllerInterface/DInput/DInputJoystick.h

@@ -57,7 +57,7 @@ private:
   };
 
 public:
-  void UpdateInput() override;
+  Core::DeviceRemoval UpdateInput() override;
 
   Joystick(const LPDIRECTINPUTDEVICE8 device);
   ~Joystick();

Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.cpp

@@ -205,7 +205,7 @@ void KeyboardMouse::UpdateCursorInput()
   m_state_in.cursor.y = (ControlState(point.y) / win_height * 2 - 1) * window_scale.y;
 }
 
-void KeyboardMouse::UpdateInput()
+Core::DeviceRemoval KeyboardMouse::UpdateInput()
 {
   UpdateCursorInput();
 
@@ -254,6 +254,8 @@ void KeyboardMouse::UpdateInput()
     else
       INFO_LOG_FMT(CONTROLLERINTERFACE, "Keyboard device failed to re-acquire, we'll retry later");
   }
+
+  return Core::DeviceRemoval::Keep;
 }
 
 std::string KeyboardMouse::GetName() const

Source/Core/InputCommon/ControllerInterface/DInput/DInputKeyboardMouse.h

@@ -94,7 +94,7 @@ private:
   };
 
 public:
-  void UpdateInput() override;
+  Core::DeviceRemoval UpdateInput() override;
 
   KeyboardMouse(const LPDIRECTINPUTDEVICE8 kb_device, const LPDIRECTINPUTDEVICE8 mo_device);
   ~KeyboardMouse();

Source/Core/InputCommon/ControllerInterface/DualShockUDPClient/DualShockUDPClient.cpp

@@ -128,7 +128,7 @@ private:
   };
 
 public:
-  void UpdateInput() override;
+  Core::DeviceRemoval UpdateInput() override;
 
   Device(std::string name, int index, std::string server_address, u16 server_port, u32 client_uid);
 
@@ -614,7 +614,7 @@ std::string Device::GetSource() const
   return std::string(DUALSHOCKUDP_SOURCE_NAME);
 }
 
-void Device::UpdateInput()
+Core::DeviceRemoval Device::UpdateInput()
 {
   // Regularly tell the UDP server to feed us controller data
   const auto now = SteadyClock::now();
@@ -660,6 +660,8 @@ void Device::UpdateInput()
       m_prev_touch_valid = true;
     }
   }
+
+  return Core::DeviceRemoval::Keep;
 }
 
 std::optional<int> Device::GetPreferredId() const

Source/Core/InputCommon/ControllerInterface/InputBackend.cpp

@@ -12,7 +12,7 @@ InputBackend::InputBackend(ControllerInterface* controller_interface)
 
 InputBackend::~InputBackend() = default;
 
-void InputBackend::UpdateInput()
+void InputBackend::UpdateInput(std::vector<std::weak_ptr<ciface::Core::Device>>& devices_to_remove)
 {
 }
 

Source/Core/InputCommon/ControllerInterface/InputBackend.h

@@ -3,10 +3,19 @@
 
 #pragma once
 
+#include <memory>
+#include <vector>
+
 class ControllerInterface;
 
 namespace ciface
 {
+
+namespace Core
+{
+class Device;
+}
+
 class InputBackend
 {
 public:
@@ -15,7 +24,9 @@ public:
   virtual ~InputBackend();
 
   virtual void PopulateDevices() = 0;
-  virtual void UpdateInput();
+  // Do NOT directly add/remove devices within here,
+  // just add them to the removal list if necessary.
+  virtual void UpdateInput(std::vector<std::weak_ptr<ciface::Core::Device>>& devices_to_remove);
 
   ControllerInterface& GetControllerInterface();
 

Source/Core/InputCommon/ControllerInterface/Pipes/Pipes.cpp

@@ -86,7 +86,7 @@ PipeDevice::~PipeDevice()
   close(m_fd);
 }
 
-void PipeDevice::UpdateInput()
+Core::DeviceRemoval PipeDevice::UpdateInput()
 {
   // Read any pending characters off the pipe. If we hit a newline,
   // then dequeue a command off the front of m_buf and parse it.
@@ -105,6 +105,7 @@ void PipeDevice::UpdateInput()
     m_buf.erase(0, newline + 1);
     newline = m_buf.find("\n");
   }
+  return Core::DeviceRemoval::Keep;
 }
 
 void PipeDevice::AddAxis(const std::string& name, double value)

Source/Core/InputCommon/ControllerInterface/Pipes/Pipes.h

@@ -29,7 +29,7 @@ public:
   PipeDevice(int fd, const std::string& name);
   ~PipeDevice();
 
-  void UpdateInput() override;
+  Core::DeviceRemoval UpdateInput() override;
   std::string GetName() const override { return m_name; }
   std::string GetSource() const override { return "Pipe"; }
 

Source/Core/InputCommon/ControllerInterface/Quartz/QuartzKeyboardAndMouse.h

@@ -62,7 +62,7 @@ private:
   };
 
 public:
-  void UpdateInput() override;
+  Core::DeviceRemoval UpdateInput() override;
 
   explicit KeyboardAndMouse(void* view);
   ~KeyboardAndMouse() override;

Source/Core/InputCommon/ControllerInterface/Quartz/QuartzKeyboardAndMouse.mm

@@ -236,7 +236,7 @@ void KeyboardAndMouse::MainThreadInitialization(void* view)
   m_window_pos_observer = [[DolWindowPositionObserver alloc] initWithView:cocoa_view];
 }
 
-void KeyboardAndMouse::UpdateInput()
+Core::DeviceRemoval KeyboardAndMouse::UpdateInput()
 {
   NSRect bounds = [m_window_pos_observer frame];
 
@@ -268,6 +268,8 @@ void KeyboardAndMouse::UpdateInput()
     m_cursor.x = (loc.x / window_width * 2 - 1.0) * window_scale.x;
     m_cursor.y = (loc.y / window_height * 2 - 1.0) * -window_scale.y;
   }
+
+  return Core::DeviceRemoval::Keep;
 }
 
 std::string KeyboardAndMouse::GetName() const

Source/Core/InputCommon/ControllerInterface/SDL/SDL.cpp

@@ -18,6 +18,11 @@
 #include <Windows.h>
 #endif
 
+namespace ciface::Core
+{
+class Device;
+}
+
 namespace ciface::SDL
 {
 static std::string GetJoystickName(int index)
@@ -35,7 +40,7 @@ public:
   InputBackend(ControllerInterface* controller_interface);
   ~InputBackend();
   void PopulateDevices() override;
-  void UpdateInput() override;
+  void UpdateInput(std::vector<std::weak_ptr<ciface::Core::Device>>& devices_to_remove) override;
 
 private:
   void OpenAndAddDevice(int index);
@@ -637,7 +642,7 @@ void Joystick::Motor::SetState(ControlState state)
 }
 #endif
 
-void InputBackend::UpdateInput()
+void InputBackend::UpdateInput(std::vector<std::weak_ptr<ciface::Core::Device>>& devices_to_remove)
 {
   SDL_JoystickUpdate();
 }

Source/Core/InputCommon/ControllerInterface/SteamDeck/SteamDeck.cpp

@@ -113,7 +113,7 @@ public:
   Device(hid_device* device);
   std::string GetName() const final override;
   std::string GetSource() const final override;
-  void UpdateInput() override;
+  Core::DeviceRemoval UpdateInput() override;
 
 private:
   hid_device* m_device;
@@ -279,7 +279,7 @@ std::string Device::GetSource() const
   return std::string(STEAMDECK_SOURCE_NAME);
 }
 
-void Device::UpdateInput()
+Core::DeviceRemoval Device::UpdateInput()
 {
   // As of a certain mid-2023 update to the Steam client,
   // Steam will disable gyro data if gyro is not mapped in Steam Input.
@@ -308,16 +308,18 @@ void Device::UpdateInput()
   }
   // In case there were no reports available to be read, bail early.
   if (!got_anything)
-    return;
+    return Core::DeviceRemoval::Keep;
 
   if (rpt.major_ver != 0x01 || rpt.minor_ver != 0x00 || rpt.report_type != 0x09 ||
       rpt.report_sz != sizeof(rpt))
   {
     ERROR_LOG_FMT(CONTROLLERINTERFACE, "Steam Deck bad report");
-    return;
+    return Core::DeviceRemoval::Keep;
   }
 
   m_latest_input = rpt;
+
+  return Core::DeviceRemoval::Keep;
 }
 
 }  // namespace ciface::SteamDeck

Source/Core/InputCommon/ControllerInterface/WGInput/WGInput.cpp

@@ -490,7 +490,7 @@ private:
 
   std::string GetSource() const override { return std::string(SOURCE_NAME); }
 
-  void UpdateInput() override
+  Core::DeviceRemoval UpdateInput() override
   {
     // IRawGameController:
     static_assert(sizeof(bool) == sizeof(ButtonValueType));
@@ -527,6 +527,8 @@ private:
     // IGameControllerBatteryInfo:
     if (!UpdateBatteryLevel())
       DEBUG_LOG_FMT(CONTROLLERINTERFACE, "WGInput: UpdateBatteryLevel failed.");
+
+    return Core::DeviceRemoval::Keep;
   }
 
   void UpdateMotors()

Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.cpp

@@ -1395,14 +1395,10 @@ void Device::UpdateRumble()
   QueueReport(OutputReportRumble{});
 }
 
-void Device::UpdateInput()
+Core::DeviceRemoval Device::UpdateInput()
 {
   if (!m_wiimote->IsConnected())
-  {
-    g_controller_interface.RemoveDevice(
-        [this](const Core::Device* device) { return device == this; });
-    return;
-  }
+    return Core::DeviceRemoval::Remove;
 
   UpdateRumble();
   RunTasks();
@@ -1413,6 +1409,8 @@ void Device::UpdateInput()
     ProcessInputReport(report);
     RunTasks();
   }
+
+  return Core::DeviceRemoval::Keep;
 }
 
 void Device::MotionPlusState::ProcessData(const WiimoteEmu::MotionPlus::DataFormat& data)

Source/Core/InputCommon/ControllerInterface/Wiimote/WiimoteController.h

@@ -35,7 +35,7 @@ public:
   std::string GetSource() const override;
   int GetSortPriority() const override;
 
-  void UpdateInput() override;
+  Core::DeviceRemoval UpdateInput() override;
 
 private:
   using Clock = std::chrono::steady_clock;

Source/Core/InputCommon/ControllerInterface/XInput/XInput.cpp

@@ -264,7 +264,7 @@ std::string Device::GetSource() const
   return "XInput";
 }
 
-void Device::UpdateInput()
+Core::DeviceRemoval Device::UpdateInput()
 {
   PXInputGetState(m_index, &m_state_in);
 
@@ -286,6 +286,8 @@ void Device::UpdateInput()
       break;
     }
   }
+
+  return Core::DeviceRemoval::Keep;
 }
 
 void Device::UpdateMotors()

Source/Core/InputCommon/ControllerInterface/XInput/XInput.h

@@ -33,7 +33,7 @@ public:
   std::optional<int> GetPreferredId() const override;
   int GetSortPriority() const override { return -2; }
 
-  void UpdateInput() override;
+  Core::DeviceRemoval UpdateInput() override;
 
   void UpdateMotors();
 

Source/Core/InputCommon/ControllerInterface/Xlib/XInput2.cpp

@@ -275,7 +275,7 @@ void KeyboardMouse::UpdateCursor(bool should_center_mouse)
   m_state.cursor.y = (win_y / win_height * 2 - 1) * window_scale.y;
 }
 
-void KeyboardMouse::UpdateInput()
+Core::DeviceRemoval KeyboardMouse::UpdateInput()
 {
   XFlush(m_display);
 
@@ -369,6 +369,8 @@ void KeyboardMouse::UpdateInput()
 
   if (update_keyboard)
     XQueryKeymap(m_display, m_state.keyboard.data());
+
+  return Core::DeviceRemoval::Keep;
 }
 
 std::string KeyboardMouse::GetName() const

Source/Core/InputCommon/ControllerInterface/Xlib/XInput2.h

@@ -111,7 +111,7 @@ private:
   void UpdateCursor(bool should_center_mouse);
 
 public:
-  void UpdateInput() override;
+  Core::DeviceRemoval UpdateInput() override;
 
   KeyboardMouse(Window window, int opcode, int pointer_deviceid, int keyboard_deviceid,
                 double scroll_increment);

Source/Core/InputCommon/ControllerInterface/evdev/evdev.cpp

@@ -674,7 +674,7 @@ void InputBackend::RemoveDevnodeObject(const std::string& node)
   m_devnode_objects.erase(node);
 }
 
-void evdevDevice::UpdateInput()
+Core::DeviceRemoval evdevDevice::UpdateInput()
 {
   // Run through all evdev events
   // libevdev will keep track of the actual controller state internally which can be queried
@@ -691,6 +691,7 @@ void evdevDevice::UpdateInput()
         rc = libevdev_next_event(node.device, LIBEVDEV_READ_FLAG_NORMAL, &ev);
     }
   }
+  return Core::DeviceRemoval::Keep;
 }
 
 bool evdevDevice::IsValid() const

Source/Core/InputCommon/ControllerInterface/evdev/evdev.h

@@ -72,7 +72,7 @@ private:
   };
 
 public:
-  void UpdateInput() override;
+  Core::DeviceRemoval UpdateInput() override;
   bool IsValid() const override;
 
   evdevDevice(InputBackend* input_backend);