Team2890/allwpilib
4
0
Fork 0
mirror of https://github.com/wpilibsuite/allwpilib synced 2026-06-19 00:41:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
This can cause lock inversions with other code (in particular with
the simulation WebSockets interface).
This commit is contained in:
Peter Johnson
2024-07-20 22:09:09 -07:00
committed by GitHub
parent b4d42d8980
commit 8548d83b03
2 changed files with 39 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
hal/src/main/native/sim/mockdata/SimDeviceData.cpp
View File

@@ -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();
}
}
}

32
hal/src/main/native/sim/mockdata/SimDeviceDataInternal.h
View File

@@ -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();
}
}
}