Early release of access object?

[Amomum]

For a couple of times while using this (very nice!) library I found myself in a little predicament when I want to use exit-on-error idiom, like so:

auto devices = devices_.readAcces();

auto dev = devices->find("somehow");
if( dev == devices->end() ) {
  connect("somehow");
  return;
}

if( dev->connected() == false ) {
  connect("somehow");
  return;
}

Since connect requires writeAccess to the same devices_ - it gets stuck, because ReadAccess object is still alive even though not needed inside either of if's.

Can you please give me any advice on what can be done in such case to keep everything clean?

(In this particular example I can extract both checks to a lambda but this, to me at least, looks a bit like hoop-jumping)

My suggestion

I myself would add a method like `release` that will release the mutex and put access object in an empty state (that will cause an exception if accessed) (even though it sorta kinda smells bed), since this will require the least amount of typing.

[LouisCharlesC]

Hey there, I'm glad you like the library!

I guess I see the problem. I also guess I like typing even less then you, because the first solution that comes to my mind is to wrap your devices into a std::optional. This would let you .reset() it whenever you like, effectively unlocking the mutex and throwing an exception if you try to access the value (given you use the throwing interface of std::optional).

See, access objects do not really do anything on their own. This is by design because it allows them to work why any kind of lock (having no expectation makes things very simple!). The .release() method you talk about couldn't work with a std::lock_guard, for instance, since it is impossible to have a std::lock_guard release its mutex short of destructing it. Or did I misinterpret the suggestion ?

Be aware though that you can use safe with std::unique_lock which does have an .unlock() method. Now sadly because of what I just said above, it is impossible for the access object to intercept the call to .unlock() and know that the value object should not be accessed anymore. So its not the safest solution, but it qualifies as a low typing solution.

Now, if you own the code and can change the way connect() works, you probably have other options.

Does any of this seem to solve your problem ?

[Amomum]

Hm... I forgot about the optional and it seemed to be the good solution indeed, however for some reason it doesn't compile in the most simple (and therefore, desired) form with CTAD:

    safe::Safe<int> test;

    auto t = std::optional{ test.readAccess() };

nor with a wordy:

std::optional< safe::ReadAccess< safe::Safe<int> > > t = test.readAccess();

They produce different but similarly bizarre errors; first one gives a lot of template errors that I can't comprehend quickly... sorry, I'm not that good with template wizardry :(

Can you please elaborate on how to use optional with an access object?

---

The .release() method you talk about couldn't work with a std::lock_guard, for instance, since it is impossible to have a std::lock_guard release its mutex short of destructing it. Or did I misinterpret the suggestion ?

No, you interpreted me correctly. Hm. Well, you could use placement new (or optional :) to construct lock_guard and destruct it by explicit call to destructor or resetting the optional..

[LouisCharlesC]

Argh, I guess the "non-movable, non-copyable" nature of std::lock_guard strikes again. The compiler does not let you do what you want because you construct an Access object and then try to move it inside the std::optional. Access objects are by default non-movable and non-copyable because they by default contain a std::lock_guard.

Try with a std::unique_lock to confirm the problem: auto t = std::optional{test.readAccess<std::unique_lock>()};. It compiles. But if we want a std::lock_guard rather than a std::unique_lock, we must construct the Access object in place in the std::optional. Indeed, std::optional<safe::ReadAccess<safe::Safe<int>>> t2{std::in_place, test}; compiles. I never tried combining safe and std::optional before. Sorry it is a bit more messy than expected.

Now, I could do all that inside the Access objects and offer a release() method and I guess it could have some value in that it would tie the release of the mutex with the "release" of the value object pointed to by the Access object. But it would also totally allow one to by-pass the guarantees of std::lock_guard which are: if the lock lives, the mutex is locked. I want that guarantee in a default Access object: if the Accessobject lives, the mutex is locked and the value is accessible.

I would argue that your code seems to not be very "scopy". I prefer to deal with mutexes with clear scoped variables, when I can. In your case, it could mean to have the connect() method work with an already locked mutex. Maybe it could accept an Access object as argument, No need for a release() method, then. Maybe it is impossible in your case, but if it is so, you can always to resort to the std::optional we just discovered. Sorry for the free jab to your code ;)

[Amomum]

I want that guarantee in a default Access object: if the Accessobject lives, the mutex is locked and the value is accessible.

Fair enough, can't argue with that one. I'm also not a big fan of objects that can exist in a invalid state without being explicitly optional-like, however in C++ a lot of objects behave like that.. I guess we should not increase their numbers :)

I would argue that your code seems to not be very "scopy".

That is also a fair point. I've came up with a more scopy version so I guess I will stick to this style:

auto shouldConnect = [&] -> bool {
  auto devices = devices_.readAcces();

  auto dev = devices->find("somehow");
  if( dev == devices->end() ) {
    return true;
  }

  if( dev->connected() == false ) {
    return true;
  }

  return false;
}();

if( shouldConnect ) {
  connect("somehow");
}

Indeed, std::optional<safe::ReadAccess<safe::Safe>> t2{std::in_place, test}; compiles. I never tried combining safe and std::optional before. Sorry it is a bit more messy than expected.

I guess this could be hidden inside the library in something like getOptionalReadAccess but.. meh. Not sure if it's worth it.

[LouisCharlesC]

I guess this could be hidden inside the library in something like getOptionalReadAccess but.. meh. Not sure if it's worth it.

Agreed. I'm a "less is more" nazy. Others have tried to make me add stuff to the library and failed. If the point ever comes up again, I guess I'll reconsider.

I've came up with a more scopy version so I guess I will stick to this style

That looks much nicer :)

Should we consider this issue closed ?

[Amomum]

Others have tried to make me add stuff to the library and failed.

:sweat_smile:

Should we consider this issue closed ?

Yes! Thank you for your time!