[hal] SimDevice: Don't hold lock during callbacks (wpilibsuite#6855)

This can cause lock inversions with other code (in particular with
the simulation WebSockets interface).

hal/src/main/native/sim/mockdata/SimDeviceData.cpp

@@ -81,7 +81,7 @@ bool SimDeviceData::IsDeviceEnabled(const char* name) {
 }
 
 HAL_SimDeviceHandle SimDeviceData::CreateDevice(const char* name) {
-  std::scoped_lock lock(m_mutex);
+  std::unique_lock lock(m_mutex);
 
   // don't create if disabled
   for (const auto& elem : m_prefixEnabled) {
@@ -111,13 +111,13 @@ HAL_SimDeviceHandle SimDeviceData::CreateDevice(const char* name) {
   m_deviceMap[name] = deviceImpl;
 
   // notify callbacks
-  m_deviceCreated(name, deviceHandle);
+  m_deviceCreated(lock, name, deviceHandle);
 
   return deviceHandle;
 }
 
 void SimDeviceData::FreeDevice(HAL_SimDeviceHandle handle) {
-  std::scoped_lock lock(m_mutex);
+  std::unique_lock lock(m_mutex);
   --handle;
 
   // see if it exists
@@ -136,14 +136,14 @@ void SimDeviceData::FreeDevice(HAL_SimDeviceHandle handle) {
   m_devices.erase(handle);
 
   // notify callbacks
-  m_deviceFreed(deviceImpl->name.c_str(), handle + 1);
+  m_deviceFreed(lock, deviceImpl->name.c_str(), handle + 1);
 }
 
 HAL_SimValueHandle SimDeviceData::CreateValue(
     HAL_SimDeviceHandle device, const char* name, int32_t direction,
     int32_t numOptions, const char** options, const double* optionValues,
     const HAL_Value& initialValue) {
-  std::scoped_lock lock(m_mutex);
+  std::unique_lock lock(m_mutex);
 
   // look up device
   Device* deviceImpl = LookupDevice(device);
@@ -188,7 +188,7 @@ HAL_SimValueHandle SimDeviceData::CreateValue(
   deviceImpl->valueMap[name] = valueImpl;
 
   // notify callbacks
-  deviceImpl->valueCreated(name, valueHandle, direction, &initialValue);
+  deviceImpl->valueCreated(lock, name, valueHandle, direction, &initialValue);
 
   return valueHandle;
 }
@@ -209,7 +209,7 @@ HAL_Value SimDeviceData::GetValue(HAL_SimValueHandle handle) {
 
 void SimDeviceData::SetValue(HAL_SimValueHandle handle,
                              const HAL_Value& value) {
-  std::scoped_lock lock(m_mutex);
+  std::unique_lock lock(m_mutex);
   Value* valueImpl = LookupValue(handle);
   if (!valueImpl) {
     return;
@@ -218,12 +218,12 @@ void SimDeviceData::SetValue(HAL_SimValueHandle handle,
   valueImpl->value = value;
 
   // notify callbacks
-  valueImpl->changed(valueImpl->name.c_str(), valueImpl->handle,
+  valueImpl->changed(lock, valueImpl->name.c_str(), valueImpl->handle,
                      valueImpl->direction, &value);
 }
 
 void SimDeviceData::ResetValue(HAL_SimValueHandle handle) {
-  std::scoped_lock lock(m_mutex);
+  std::unique_lock lock(m_mutex);
   Value* valueImpl = LookupValue(handle);
   if (!valueImpl) {
     return;
@@ -240,7 +240,7 @@ void SimDeviceData::ResetValue(HAL_SimValueHandle handle) {
   }
 
   // notify reset callbacks (done here so they're called with the old value)
-  valueImpl->reset(valueImpl->name.c_str(), valueImpl->handle,
+  valueImpl->reset(lock, valueImpl->name.c_str(), valueImpl->handle,
                    valueImpl->direction, &valueImpl->value);
 
   // set user-facing value to 0
@@ -259,14 +259,14 @@ void SimDeviceData::ResetValue(HAL_SimValueHandle handle) {
   }
 
   // notify changed callbacks
-  valueImpl->changed(valueImpl->name.c_str(), valueImpl->handle,
+  valueImpl->changed(lock, valueImpl->name.c_str(), valueImpl->handle,
                      valueImpl->direction, &valueImpl->value);
 }
 
 int32_t SimDeviceData::RegisterDeviceCreatedCallback(
     const char* prefix, void* param, HALSIM_SimDeviceCallback callback,
     bool initialNotify) {
-  std::scoped_lock lock(m_mutex);
+  std::unique_lock lock(m_mutex);
 
   // register callback
   int32_t index = m_deviceCreated.Register(prefix, param, callback);
@@ -275,7 +275,11 @@ int32_t SimDeviceData::RegisterDeviceCreatedCallback(
   if (initialNotify) {
     for (auto&& device : m_devices) {
       if (wpi::starts_with(device->name, prefix)) {
-        callback(device->name.c_str(), param, device->handle);
+        auto name = device->name;
+        auto handle = device->handle;
+        lock.unlock();
+        callback(name.c_str(), param, handle);
+        lock.lock();
       }
     }
   }

hal/src/main/native/sim/mockdata/SimDeviceDataInternal.h

@@ -58,12 +58,19 @@ class SimUnnamedCallbackRegistry {
            1;
   }
 
-  template <typename... U>
-  void Invoke(const char* name, U&&... u) const {
+  template <typename Mutex, typename... U>
+  void Invoke(std::unique_lock<Mutex>& lock, const char* name, U&&... u) const {
     if (m_callbacks) {
-      for (auto&& cb : *m_callbacks) {
-        reinterpret_cast<CallbackFunction>(cb.callback)(name, cb.param,
-                                                        std::forward<U>(u)...);
+      for (size_t i = 0; i < m_callbacks->size(); ++i) {
+        auto& cb = (*m_callbacks)[i];
+        if (cb.callback) {
+          auto callback = cb.callback;
+          auto param = cb.param;
+          lock.unlock();
+          reinterpret_cast<CallbackFunction>(callback)(name, param,
+                                                       std::forward<U>(u)...);
+          lock.lock();
+        }
       }
     }
   }
@@ -115,12 +122,17 @@ class SimPrefixCallbackRegistry {
     return m_callbacks->emplace_back(prefix, param, callback) + 1;
   }
 
-  template <typename... U>
-  void Invoke(const char* name, U&&... u) const {
+  template <typename Mutex, typename... U>
+  void Invoke(std::unique_lock<Mutex>& lock, const char* name, U&&... u) const {
     if (m_callbacks) {
-      for (auto&& cb : *m_callbacks) {
-        if (wpi::starts_with(name, cb.prefix)) {
-          cb.callback(name, cb.param, std::forward<U>(u)...);
+      for (size_t i = 0; i < m_callbacks->size(); ++i) {
+        auto& cb = (*m_callbacks)[i];
+        if (cb.callback && wpi::starts_with(name, cb.prefix)) {
+          auto callback = cb.callback;
+          auto param = cb.param;
+          lock.unlock();
+          callback(name, param, std::forward<U>(u)...);
+          lock.lock();
         }
       }
     }