From 8548d83b03e0ea0819b70c668ad39eef47e18796 Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Sat, 20 Jul 2024 22:09:09 -0700 Subject: [PATCH] [hal] SimDevice: Don't hold lock during callbacks (#6855) This can cause lock inversions with other code (in particular with the simulation WebSockets interface). --- .../native/sim/mockdata/SimDeviceData.cpp | 30 +++++++++-------- .../sim/mockdata/SimDeviceDataInternal.h | 32 +++++++++++++------ 2 files changed, 39 insertions(+), 23 deletions(-) diff --git a/hal/src/main/native/sim/mockdata/SimDeviceData.cpp b/hal/src/main/native/sim/mockdata/SimDeviceData.cpp index 9c7fdbc2024..5b60cbb264a 100644 --- a/hal/src/main/native/sim/mockdata/SimDeviceData.cpp +++ b/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(); } } } diff --git a/hal/src/main/native/sim/mockdata/SimDeviceDataInternal.h b/hal/src/main/native/sim/mockdata/SimDeviceDataInternal.h index deff6ed2822..9e202d527c1 100644 --- a/hal/src/main/native/sim/mockdata/SimDeviceDataInternal.h +++ b/hal/src/main/native/sim/mockdata/SimDeviceDataInternal.h @@ -58,12 +58,19 @@ class SimUnnamedCallbackRegistry { 1; } - template - void Invoke(const char* name, U&&... u) const { + template + void Invoke(std::unique_lock& lock, const char* name, U&&... u) const { if (m_callbacks) { - for (auto&& cb : *m_callbacks) { - reinterpret_cast(cb.callback)(name, cb.param, - std::forward(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(callback)(name, param, + std::forward(u)...); + lock.lock(); + } } } } @@ -115,12 +122,17 @@ class SimPrefixCallbackRegistry { return m_callbacks->emplace_back(prefix, param, callback) + 1; } - template - void Invoke(const char* name, U&&... u) const { + template + void Invoke(std::unique_lock& 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)...); + 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)...); + lock.lock(); } } }