2
\$\begingroup\$

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());
}
\$\endgroup\$
1
  • \$\begingroup\$ @TobySpeight Thanks. Included additional information. \$\endgroup\$ Feb 5 '18 at 12:25
1
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks for your answer. I use sleep_for to prevent thread starvation. I create this project for the embedded linux system, but I develop the code mainly on my Mac. I found that their (Mac and Linux) threading mechanism behave differently. On a Mac, it is quite common for a thread to recapture a lock before another thread had a change to execute it's own code. \$\endgroup\$ Feb 5 '18 at 19:43
  • \$\begingroup\$ @CorellianAle: I would regard that as a bug. You have written your code so that it is procedural based on a looping. I would expect this to be written to work with an event driven framework so you register callbacks that are hit on certain conditions or clocks. \$\endgroup\$ Feb 6 '18 at 0:57
  • \$\begingroup\$ Personally multiple locks are more susceptible to deadlock , looks ugly and difficult to manage. In such scenarios, I would rather use mutex , conditional variable with a couple of atomic bools. Since the works in the multi-thread branches will keep waiting for the lock more often than not, i would prefer using a single thread pipeline consisting of packaged tasks. The later will look elegant and fail safe. \$\endgroup\$
    – seccpur
    Feb 9 '18 at 1:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.