trjhtr

Trying to review my own code to be sure I implemented thread-safety correctly. I need additional opinions because I may have missed something. The amount of locks feels like an ugly code an I am also not sure about passing std::shared_ptr to std::thread. Basically, any critics is welcome. If you see something unrelated to multithreading but still incorrect/bad-practice, please share.

General idea - HardwareManager controls several IBusManagers. HardwareManager launches several threads to update it's state: _threadUpdate calls update() method and _updateBusThreads threads call updateBus() to update each bus. getState() method of a HardwareManager may be called by several threads from different parts of the programs, so it needs to me thread-safe.

#include <atomic>
#include <chrono>
#include <memory>
#include <mutex>
#include <thread>
#include <vector>

struct IoState

 IoState() ;

 std::vector<bool> ios;
 std::vector<bool> directions;

 //...
;

/**
 * State interface.
 */
class IDeviceState

public:

 IDeviceState() ;
 virtual ~IDeviceState();

 virtual std::vector<IoState> getIoStates() const = 0;
;

/**
 * Bus manager Interface.
 */
class IBusManager

public:

 IBusManager() ;
 virtual ~IBusManager();

 virtual bool initialize() = 0;
 virtual void update() = 0;
 virtual std::vector<IDeviceState> getState() const = 0;
 ;

class HardwareManager

public:

 HardwareManager();

 void start();
 void stop();

 std::vector<std::vector<IDeviceState>> getDeviceStates() const;

private:

 std::vector<std::vector<IDeviceState>> _deviceStates;
 std::vector<std::shared_ptr<IBusManager>> _busManagers; //pointer because intended to be passed to the corresponding thread

 std::thread _updateThread;
 std::vector<std::shared_ptr<std::thread>> _updateBusThreads; //pointer because std::thread is immutable?

 std::atomic<bool> _isShutdown;

 std::mutex _mutexBusManagers;
 mutable std::mutex _mutexDeviceStates;
 std::mutex _mutexUpdateThread;
 std::mutex _mutexUpdateBusThreads;

 /**
 * Initializes all resources. Starts background worker threads.
 */
 bool initialize();

 /**
 * Joins all background threads.
 */
 bool shutdown();

 /**
 * Main update loop. Executed my _updateThread.
 */
 void update();

 /**
 * Bus manager update loop. Executed by _updateBusThreads.
 * @param busManager bus manager to be updated
 */
 void updateBus(std::shared_ptr<IBusManager> busManager);

 //...
;

void HardwareManager::start()

 _isShutdown = false;
 auto result = initialize();

 if (!result)
 
 _isShutdown = true;
 


void HardwareManager::stop()

 _isShutdown = false;
 shutdown();


//...

bool HardwareManager::shutdown()

 try
 
 //wait for the bus loops
 std::lock_guard<std::mutex> lockUpdateBusThreads(_mutexUpdateBusThreads);

 for (auto& updateBusThread : _updateBusThreads)
 
 if (updateBusThread->joinable())
 
 updateBusThread->join();
 
 

 //wait for the main loop
 std::unique_lock<std::mutex> lockUpdateThread(_mutexUpdateThread);

 if (_updateThread.joinable())
 
 _updateThread.join();
 
 
 catch (const std::exception& ex)
 
 return false;
 

 return true;


bool HardwareManager::initialize()

 try
 
 //lock
 std::unique_lock<std::mutex> lockBusManagers(_mutexBusManagers);
 std::unique_lock<std::mutex> lockDeviceStates(_mutexDeviceStates);
 std::unique_lock<std::mutex> lockUpdateBusThreads(_mutexUpdateBusThreads);

 //allocate
 _updateBusThreads = std::vector<std::shared_ptr<std::thread>>(_busManagers.size());
 _deviceStates = std::vector<std::vector<IDeviceState>>(_busManagers.size());

 //initialize resource and background threads
 for (auto& busManager : _busManagers)
 
 auto result = busManager->initialize();

 if (!result)
 
 return false;
 

 auto state = busManager->getState();
 _deviceStates.push_back(state);

 auto thread = std::make_shared<std::thread>(&HardwareManager::updateBus, this, busManager);
 _updateBusThreads.push_back(thread);
 

 //unlock
 lockBusManagers.unlock();
 lockDeviceStates.unlock();
 lockUpdateBusThreads.unlock();

 //launch main update
 std::lock_guard<std::mutex> lockUpdateThread(_mutexUpdateThread);
 _updateThread = std::thread(&HardwareManager::update, this);
 
 catch (const std::exception& ex)
 
 return false;
 

 return true;


void HardwareManager::update()

 while (!_isShutdown)
 
 std::lock_guard<std::mutex> lockBusManagers(_mutexBusManagers);
 std::lock_guard<std::mutex> lockDeviceStates(_mutexDeviceStates);

 for (std::size_t i = 0; i < _busManagers.size(); ++i)
 
 _deviceStates[i] = _busManagers[i]->getState(); //getState is supposed to be thread-safe
 

 std::this_thread::sleep_for(std::chrono::milliseconds(10));
 


void HardwareManager::updateBus(std::shared_ptr<IBusManager> busManager)

 while (!_isShutdown)
 
 busManager->update(); //update is supposed to be thread-safe

 std::this_thread::sleep_for(std::chrono::milliseconds(10));
 


std::vector<std::vector<IDeviceState>> HardwareManager::getDeviceStates() const

 std::lock_guard<std::mutex> lockDeviceStates(_mutexDeviceStates);
 return _deviceStates;


//...

The HardwareManager is being used like this (simplified example):

//...

HardwareManager manager;
manager.start();

//pass state with network clients/web interface
std::thread broadcastToNetworkThread = std::thread([&]
 while (true)
 
 network.send(manager.getState());
 
);

//pass state to main task controller
while(true)

 controller.updateIo(manager.getState());

I can't see anything major I want to complain about.

Seems very solid code.

When you have multiple locks.

There is always an issue with deadlock caused by acquiring the locks in the wrong order. To avoid this you should use std::lock() to make sure that all locks are acquired in the correct order.

So things like this:

 std::unique_lock<std::mutex> lockBusManagers(_mutexBusManagers);
 std::unique_lock<std::mutex> lockDeviceStates(_mutexDeviceStates);
 std::unique_lock<std::mutex> lockUpdateBusThreads(_mutexUpdateBusThreads);

Should be done like this:

 // When you use unique lock you indicate your attention to lock
 // first. But don't specifically acquire them
 std::unique_lock<std::mutex> lockBusManagers(_mutexBusManagers, std::defer_lock);
 std::unique_lock<std::mutex> lockDeviceStates(_mutexDeviceStates, std::defer_lock);
 std::unique_lock<std::mutex> lockUpdateBusThreads(_mutexUpdateBusThreads, std::defer_lock);


 // Then use `std::lock()` to acquire the locks.
 // This guarantees that the locks are acquired in the correct order.
 // Correct: Same order every where they are used in combination.
 std::lock(_mutexBusManagers, _mutexDeviceStates, _mutexUpdateBusThreads);

I would note that you seem to have always used a consistent order (so there should be no issues in your code). But not doing it makes your code very brittle to maintenance. Even if you write it correctly at some point somebody else will maintain it and they may not be as careful. So you need to guard against these.

Avoid the temptation of releasing the locks manually.

 //unlock
 lockBusManagers.unlock();
 lockDeviceStates.unlock();
 lockUpdateBusThreads.unlock();

Prefer to let the std::unique_lock destructor do the unlocking.

In bool HardwareManager::initialize() you catch all std::exceptions and return true/false to indicate success. This is fine on an internal interface. But you should note that not all exceptions derive from std::exception. I would catch (...) or mark the function as noexcept (or both).

This is very suspicious:

std::this_thread::sleep_for(std::chrono::milliseconds(10));

Do you have a timing bug in your code or is there a specific requirement that you are implementing here. If there is some limitation you should document it with a comment. If it is a BUG (timing or otherwise) that you are compensating for then you also need to document that.