libunifex icon indicating copy to clipboard operation
libunifex copied to clipboard

RAII in my async_mutex? It's more likely than you think.

Open Mrkol opened this issue 3 years ago • 2 comments

Right now, usage patterns of async_mutex violate RAII in any case whatsoever, as locking the mutex (the "resource acquisition" part) is an async operation, and so cannot be done as part of a constructor (the "is initialization" part).

I think there will be no objections to this requiring a fix, so how would we do that?

I propose changing the signature of async_lock from sender_of<void> async_lock() to sender_of<unique_lock_like> async_lock(), where the asynchronously returned unique_lock_like object will unlock the mutex in it's destructor. This does mend the problem, as now initialization of this unique_lock_like object is precisely the same as acquiring the mutex.

The intention is for this new signature to be used as follows:

let_value(mtx.async_lock(), [](auto&) {
        // safely work with the protected objects
    };

A further expansion of this idea would be a rust-like Mtx<T> class, which internally stores a T, and returns a reference to T from it's async_lock method. Although we won't ever have rust-like safety guarantees without borrowing, such a class would still prove to be useful in my opinion for a lot of use cases, simplifying the code and explicitly syntactically linking the protected object with the protecting synchronization primitive. With this, on could write code like

let_value(data_storage.async_lock(), [](auto& storage ) {
        return do_other_async_work(storage->at("key"));
    };

Which I think is more structured and less error-prone than the current status quo,

let_value(mtx.async_lock(), [&data_storage, &mtx]() {
        return then(do_other_async_work(data_storage->at("key")),
            [&mtx]() { mtx.unlock(); });
    };

Mrkol avatar Dec 18 '21 11:12 Mrkol

Thanks for flagging this bug with a suggested fix @Mrkol. We're planning on investing more time into maintaining this library, and we've filed a task to address this bug to our backlog. In the meantime, feel free to put up a PR for this!

AnujYamdagni avatar Jun 29 '22 20:06 AnujYamdagni

@AnujYamdagni thanks for not abandoning this project! This issue needs some design discussion before a PR can be created.

  1. The signature change for mtx.lock would be an API break, is that acceptable?
  2. Does the rust-like mutex wrapper sound like a good idea?
  3. If 2 is a yes, then should that be the new mutex, or should it be a separate "mutexed" class?

Mrkol avatar Jun 29 '22 21:06 Mrkol