2

I'm not too experienced with multithreaded programming, but I've come up with the following and I'm wondering whether there are any obvious problems I've overlooked in my naivety.

I have a resource (in my case a drawing surface) that can only safely be used by one thread at a time. To enforce this, I have the following pair of classes:

template <typename Surface>
class lockable
{
public:
    template <typename... Args, typename = /* is_constructible constraint*/>
    lockable(Args&&... args)
        : surface_(std::forward<Args>(args)...)
    {}

    locked_surface<Surface> lock()
    {
        return locked_surface<Surface>{*this};
    }

private:
    Surface surface_;
    // Heap-allocate the mutex to allow the class to be moveable
    std::unique_ptr<std::mutex> mutex_ = std::make_unique<std::mutex>();

    friend class locked_surface<Surface>;
};

template <typename Surface>
class locked_surface
{
public:
    // Construct wrapper, obtaining lock
    explicit locked_surface(lockable<Surface>& lockable_)
        : surface_(lockable_.surface_),
          lock_(*lockable_.mutex_)
    {}

    // Wrap the Surface API
    void move_to(point2f p) { surface_.move_to(p); }

    void line_to(point2f p) { surface_.line_to(p); }

    /* Other Surface API functions... */

private:
    Surface& surface_;
    std::unique_lock<std::mutex> lock_;
};

The idea is that you wrap a surface up in a lockable<>, and then call the lock() member function to obtain exclusive access to something which implements the Surface API, and forwards all its calls to the real surface. The duration of the lock is controlled by the lifetime of the returned wrapper using RAII. For example:

lockable<cairo_pixmap_surface> ls{/*args...*/};

auto draw_shape = [&ls] (auto&& shape) {
    auto surface = ls.lock(); // blocks until surface is available
    shape.draw(surface); // lock is released even if shape.draw() throws
};

std::thread t1(draw_shape, triangle{});
std::thread t2(draw_shape, circle{});

t1.join();
t2.join();

This seems like a simple, elegant and C++-y solution to the problem. It works well in my tests, but testing multithreaded stuff is tricky: things happen in real life that are hard to simulate. Like I said, I'm a bit of a novice when it comes to multithreading in general so I'd appreciate any advice, specifically:

  • Are there any obvious problems with the above that I've overlooked?
  • Is this RAII-controlled locking wrapper idea a common pattern? If so, are there any good links to read up on it?
Tristan Brindle
  • 16,281
  • 4
  • 39
  • 82
  • 2
    Maybe more appropriate for CodeReview... anyway, I see it mostly right, except that the `unique_ptr` looks unnecessary to me. And instead of duplicating the surface API you could just override `operator->()`. – rodrigo Jun 16 '16 at 06:31
  • Probably the biggest downside, is the sensitivity. If there are read-only functions `int getWidth() const`, you may want to avoid wrapping them. – mksteve Jun 16 '16 at 06:47
  • @rodrigo `std::mutex` is not copyable or moveable, so having it as a direct member makes `lockable<>` unmoveable too, which makes life awkward. Wrapping it in a `unique_ptr` is the easiest workaround that I've found, unless you know of a better way? – Tristan Brindle Jun 16 '16 at 07:37
  • @mksteve Thanks for the comment. Could you explain a bit more what you mean by "sensitivity"? – Tristan Brindle Jun 16 '16 at 07:40
  • @TristanBrindle: I see, but I'm not sure if it is wise to move the `mutex` around; it is non-moveable for a reason. You could still make your `lockable` moveable by implementing the move constructor and _not_ moving the mutex, Instead you would lock the source mutex and create another one in the target. Likewise the move operator would lock both source and target and just move the `surface_` member. That way you'll get thread safe moving! – rodrigo Jun 16 '16 at 10:14

0 Answers0