Callbacks and `std::recursive_mutex` - valid use case?

Question

I have the following polymorphic interface:

struct service
{
    virtual void connect(std::function<void>(bool) cb);
      // Invoke 'cb' with 'true' on connection success, 'false' otherwise.

    virtual ~service() { }
};

Some implementations of service are synchronous:

struct synchronous_service : service
{
    void connect(std::function<void>(bool) cb) override
    {
        cb(true);
    }
};

Others are asynchronous:

struct asynchronous_service : service
{
    void connect(std::function<void>(bool) cb) override
    {
        _thread_pool.post([this, cb]{ cb(true); });
    }
};

I need to create a service wrapper, which is a service itself. This needs to be thread-safe and maintain some state under a mutex:

struct wrapped_service : service 
{
    state                    _state;
    std::mutex               _mutex;
    std::unique_ptr<service> _underlying;

    void connect(std::function<void>(bool) cb) override
    {
        std::lock_guard<decltype(_mutex)> guard{_mutex};
        // update `_state`

        _underlying->connect([this, cb]
        {
            std::lock_guard<decltype(_mutex)> guard{_mutex};
            // update `_state`
            cb(true);
        });

        // update `_state`
    }
}

If the _underlying->connect call is always asynchronous, then std::mutex will work fine. However, in the case that _underlying->connect is synchronous, the program will freeze.

This can be solved by using std::recursive_mutex instead of std::mutex, but it's generally known that it is a code smell.

Is this a valid use case for an std::recursive_mutex?

Or is the design flawed? Note that I have no control over the service interface.

Given your restrictions, it seems legit to me. Minor bug: You need to pass `_mutex` into the `lock_guard` constructors. — metal, Feb 01 '19 at 13:13
Can you not pass a different callback in the synchronous case? — Galik, Feb 01 '19 at 13:29
@Galik: what do you mean? I do not know if `_underlying` is synchronous or asynchronous. — Vittorio Romeo, Feb 01 '19 at 13:30
Then if the callback *always* locks the mutex can't you simply release any lock *before* calling `connect(cb)` and then if it is synchronous, you can guarantee the cb will have finished (and released its lock) by the end of the `connect(cb);` call and safely reacquire the lock. If it is asynchronous then it should be safe to re-acquire the lock anyway, because it will be on a different thread. Does that work in your scenario? — Galik, Feb 01 '19 at 13:40
@Galik: `_underlying->connect` is not guaranteed to be thread safe. Two different threads could invoke `wrapped_services::connect` at the same time. If `_underlying->connect` is not protected there could be a data race. — Vittorio Romeo, Feb 01 '19 at 13:41
@VittorioRomeo If you didn't provide the wrapper, how could the 2 threads call the `_underlying` safely? Or is the point of your wrapper to allow multiple threads to safely call the connect of a service implementation? — Makers_F, Feb 01 '19 at 14:10
@Makers_F: I was convinced of the latter, but maybe it's a wrong assumption. In that case, there would be no need for the `recursive_mutex`. — Vittorio Romeo, Feb 01 '19 at 14:26

score 0 · Answer 1 · answered Feb 01 '19 at 14:29

There are two modes of callback: immediate and delayed. That requires the client to be ready for immediate callback and be re-entrant. That complicates the client implementation. If you make the callback to be always delayed that removes the need for the client to be re-entrant.

Callbacks and `std::recursive_mutex` - valid use case?

1 Answers1