The semaphore posix wrapper should not be movable.

[elfenpiff]

Required information

Observed result or behaviour:
According to the documentations:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_09_09
https://pubs.opengroup.org/onlinepubs/009695399/functions/sem_open.html

The effect of referring to a copy of the object when locking, unlocking, or destroying it is undefined.

The move operations are copying the underlying sem_t handle which leads to undefined behavior. It is most likely that we did not encounter this issue since the move operation was optimized away with RTVO. But it can become a problem

Expected result or behaviour:
All copy- and move-operations are deleted in posix::Semaphore.

See also #1036

[elfenpiff]

Actually, it is movable without moving the underlying sem_t management construct when it is not used concurrently while being moved.
The idea would be to set the semaphore value (acquired with sem_getvalue) to the value of the origin in the move operation. The move operation never destroys the semaphore with sem_close/sem_destroy only the dtor does and and then move should be possible.

Since the construction/destruction API of a named semaphore differs from the unnamed semaphore it may makes sense to split them into to abstractions. Semaphore and NamedSemaphore.

[elBoberido]

This might work for move assignment but you still have the problem with the move ctor since the move-to sem_t would never be initialized.

[elfenpiff]

I disagree, in the move ctor it is even easier. Here you just call sem_init or sem_open and both functions offer a parameter where you can set the value of the semaphore.
But when implementing this it is actually a copy operation and not a move operation. So maybe we should remove the move operations and implement only the copy operations.

[elBoberido]

I think copy is even more dangerous than a move. I would remove both of them like in the STL

[elfenpiff]

I agree with you but I really fear the smell of the mowed meadow. Maybe we need to tweak the creation pattern with this a bit. I am thinking of something like:

cxx::optional<Semaphore> mySemaphore;
createSemaphore(mySemaphore).or_else([]{ /*...*/ });

Here we do not require move and the semaphore is initialized in place. What do you think @elBoberido

[elBoberido]

I think this might be a viable option for some use cases. But then one needs to always call mySemaphore.value().wait() which makes the code harder to read. In the end if the creation of the semaphore fails, what can be done? The programmer usually doesn't do this for fun. Waiting and retrying might be an option, but that should be the rare case and what happens if the retry also doesn't work? Another retry and hoping it will work at some time?

We might need two options. One where the Semaphore is created by its ctor and terminates if it fails, similar to the Mutex and one with the optional where the programmer knows how to recover. One of this places might be RouDi when an application requests a condition variable. But in my opinion it would be better to have this already initialized during the RouDi startup in an object pool. Then termination would again be an option.

Another option would be to have a

cxx::optional<Semaphore> mySemaphore;
auto& mySemaphoreRef = mySemaphore.value();

but this is also cumbersome and requires relative pointer if placed in the shared memory

[elfenpiff]

The developer could also write mySemaphore->wait() instead of mySemaphore.value().wait() but your point is still valid.

At the moment I am playing with the thought to just remove the move operations from the semaphore and we do not work with expected at all.
Through the abstraction we ensure the correct usage of all posix functions so misuse failures cannot happen, the only remaining error sources then are:

1. mistakes in the abstraction which allow misuse cases
2. misconfigured system when the system limit for semaphores is reached
3. corrupt system when someone removes semaphores from the system with rm -rf /dev/sem/*

In all those error cases I don't think that it is useful that the user is informed about it. When the abstraction has a defect the user is screwed and cannot recover and this goes even more for a misconfigured or corrupted system.

So we only work with cxx::Expects and cxx::Ensures and terminate.

@elBoberido or did I miss an error case where we actually require an expected?

[elBoberido]

the only thing I can currently think of is a failed sem_post with EOVERFLOW but I guess when this happens something else is already completely messed up and a log to the console or cxx::Ensures should be sufficient