Say we have something along the lines of the following pseudocode, with the goal of achieving both concurrency and taking advantage of RAII:

class Foo {
public:
    vector<int> nums;
    mutex lock;
};

class Bar {
public:
    Bar(Foo &foo) : m_foo(foo) 
    {
        lock_guard<mutex>(foo.lock);
        m_num = foo.nums.back();
        foo.nums.pop_back();
    }
    ~Bar()
    {
        lock_guard<mutex>(foo.lock);
        foo.nums.push_back(m_num);
    }
private:
    Foo &m_foo;
    int m_num;
};

Then, say we may have any number of instances of Bar, with the idea being that when they go out of scope, the destructor will return their held "resource" to the controller Foo class. However, we also need to ensure thread safety, hence the locks. I'm a little wary of this design, however, since taking a mutex in a destructor seems like a bad idea intuitively. Am I overthinking things, or if not, is there a better way to take advantage of RAII here?

CodePudding user response：

There's nothing inherently wrong with locking a mutex in a destructor. For instance, shared resources might need to be made thread-safe. Releasing ownership of a shared resource, then, might require locking a mutex. If RAII just fell apart in multithreaded programming, it wouldn't be a very useful tool. Indeed, access to a std::shared_ptr's control block is thread safe, including when decrementing the reference counter during the shared pointer's destruction. Apparently this is usually implemented with atomic operations rather than a mutex lock (don't quote me on this), but the context is the same: you're releasing ownership of a shared resource during destruction, and that has to be recorded in a thread-safe way.

However, keep in mind: locking a mutex can throw an exception, and you should (almost) always absorb exceptions in destructors with try/catch. Otherwise, if the stack is already unwinding due to another exception, the program will terminate immediately and irrecoverably, regardless of whether the calling code is equipped to absorb the original exception and / or the destructor's exception.

But there might be a way to restructure your code to avoid the issue entirely: A Bar doesn't actually need a reference to a Foo; it only needs an int. In your code, the Bar requests an int from a given Foo. When the Bar is destroyed, it needs to give the int back to the Foo so that it can be recycled; this requires storing an internal reference to the Foo throughout its lifetime so that it can communicate with it during destruction. Instead, consider giving the int to the Bar directly upon construction, and taking the int away from it upon destruction. This is the driving principle behind dependency injection, which constitutes the 'D' in "SOILD". Consequently, it brings with it all of the typical advantages of dependency injection (e.g., improving testability of the Bar class).

For instance, this logic could be tracked in a larger class which manages a Foo object along with all of its associated Bar objects. Here's some pseudocode, but the exact interface details will depend on your application:

class BarPool:
    Foo foo;
    Map<int, Bar> bars;
    mutex m;

    BarPool(Foo foo) : foo(foo) {}

    int add_bar():
        lock m;
        // Note: foo.pop() should probably be made thread-safe
        // by internally locking / unlocking foo's mutex
        int i = foo.pop()
        bars.add(i, new Bar(i));
        unlock m;
        return i

    void remove_bar(int i):
        lock m;
        // foo.push() should also probably be made thread-safe
        bars.remove(i)
        foo.push(i)
        unlock m;

    ...