iox-#751: remove old semaphore posix wrapper

[elfenpiff]

Pre-Review Checklist for the PR Author

1. Code follows the coding style of CONTRIBUTING.md
2. Tests follow the best practice for testing
3. Changelog updated in the unreleased section including API breaking changes
4. Branch follows the naming format (iox-#123-this-is-a-branch)
5. Commits messages are according to this guideline
   • Commit messages have the issue ID (iox-#123 commit text)
   • Commit messages are signed (git commit -s)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
6. Update the PR title
   • Follow the same conventions as for commit messages
   • Link to the relevant issue
7. Relevant issues are linked
8. Add sensible notes for the reviewer
9. All checks have passed (except task-list-completed)
10. Assign PR to reviewer

Notes for Reviewer

The posix::Semaphore was completely removed. Custom implementations of our iox::posix::waitForTerminationRequest(); were also removed from RoudiApp and the dds gateway.
With that the public API for RoudiApp changed and the waitForSignal protected method was deprecated (not removed) since iox::posix::waitForTerminationRequest() already implements that behavior.

Checklist for the PR Reviewer

• Commits are properly organized and messages are according to the guideline
• Code according to our coding style and naming conventions
• Unit tests have been written for new behavior
  • Each unit test case has a unique UUID
• Public API changes are documented via doxygen
• Copyright owner are updated in the changed files
• PR title describes the changes

Post-review Checklist for the PR Author

1. All open points are addressed and tracked via issues

References

• Closes The semaphore posix wrapper should not be movable. #751

[codecov]

Codecov Report

Merging #1402 (34fd386) into master (10f2ab0) will increase coverage by 0.28%.
The diff coverage is 70.37%.

@@            Coverage Diff             @@
##           master    #1402      +/-   ##
==========================================
+ Coverage   78.71%   78.99%   +0.28%     
==========================================
  Files         379      377       -2     
  Lines       14707    14511     -196     
  Branches     2050     2023      -27     
==========================================
- Hits        11576    11463     -113     
+ Misses       2466     2398      -68     
+ Partials      665      650      -15     

Flag	Coverage Δ
unittests	78.65% <70.37%> (+0.27%)	⬆️
unittests_timing	14.87% <29.62%> (-0.20%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
iceoryx_dds/source/gateway/main.cpp	0.00% <0.00%> (ø)
...ceoryx_hoofs/internal/cxx/functional_interface.inl	88.23% <0.00%> (ø)
...ude/iceoryx_hoofs/posix_wrapper/signal_watcher.hpp	100.00% <ø> (ø)
...linux/include/iceoryx_hoofs/platform/semaphore.hpp	100.00% <ø> (ø)
iceoryx_hoofs/source/cxx/functional_interface.cpp	0.00% <0.00%> (ø)
...l/popo/building_blocks/condition_variable_data.hpp	100.00% <ø> (+60.00%)	⬆️
...oryx_posh/include/iceoryx_posh/roudi/roudi_app.hpp	100.00% <ø> (+60.00%)	⬆️
...osh/source/roudi/application/iceoryx_roudi_app.cpp	35.71% <0.00%> (ø)
...ceoryx_posh/source/roudi/application/roudi_app.cpp	88.00% <0.00%> (+15.50%)	⬆️
iceoryx_hoofs/source/cxx/adaptive_wait.cpp	84.61% <60.00%> (-15.39%)	⬇️
... and 11 more

[elBoberido]

Hmm, in theory this will slow down our tests. At first it might slow down the start of the thread because the CPU is busy and then it might sleep longer than necessary.

I agree that this is a pattern that we use quite often but why not just wrap the semaphore into something like a Trigger class in the testing library with just a fire and wait method?

[elfenpiff]

You mean in the end a condition variable without predicate - notify and wait. I already implemented this and wanted to create a PR in the near future which would contain:

• posix condition wrapper (condition variable without predicate)
• unit tests for this wrapper
  see this here: https://github.com/ApexAI/iceoryx/blob/iox-%231037-implement-posix-condition-variable/iceoryx_hoofs/include/iceoryx_hoofs/posix_wrapper/condition.hpp

Would it be alright for you that I add this to the condition issue that adaptive_wait is replaced with the posix condition when it is merged?

[elBoberido]

Yes, that's fine but Condition alone is not sufficient. It needs either a Semaphore or a Condition with a predicate else there is a race between the main thread wait() and worker thread notifyOne/notifyAll. If notify is called before the main thread enters wait, the thread blocks.

[elfenpiff]

I found Barrier in hoofs testing which does exactly what we want here and replaced all adaptive waits with the barrier

[elBoberido]

This should be safe. Although getInstance will be called in the signal handler and getInstance creates the instance, it must be called before the signal handler is registered.

Out of curiosity. It seems the signal handler can only be registered with a call to getInstance, which happens when one calls e.g. waitForTerminationRequest(), which in turn is called quite late. What happens when a signal was raised before this function was called? I guess the same as when no signal handler is registered? Do we want to give the user the opportunity to explicitly register the signal handler quite early in the main method?

[elfenpiff]

@elBoberido

What happens when a signal was raised before this function was called?

You are right, it behaves like no signal handler was registered.

Do we want to give the user the opportunity to explicitly register the signal handler quite early in the main method?

I would refrain from that. For instance we register our own signal handler in the shared memory object when we zero the whole memory. After the zeroing we restore the previous signal handler but still the user may wonder why the custom call was not used.
I would prefer that the user is not allowed to register any signal handler globally and always has to use our abstractions when they want to handle something like wait for CTRL+c.

I would propose the following best practices.

1. Do not register a universal signal handler for all iceoryx applications.
2. To wait for CTRL+c always use the SignalWatcher
3. When registering a signal handler always use registerSignalHandler in the smallest scope possible with as less signals as possible (only the required ones).

// do stuff
{
  auto mySigbusGuard = registerSignalHandler(Signal::BUS, signalHandlerCallback);
  functionCallWhichMayLeadToSigbus();
}
// do other stuff

[elBoberido]

@elfenpiff I think I didn't communicate my intention clearly. It is not about the user to register a signal handler but about the abstraction registering the signal handler quite late. Let's assume Ctrl+C is pressed before the first call to iox::posix::hasTerminationRequested. In this scenario there is no graceful termination with stack unwinding and all sockets, etc. are not cleaned up. If one wants to use the convenient iox::posix::hasTerminationRequested function but also terminate gracefully, there is no obvious way to do it, except calling iox::posix::hasTerminationRequested right at the start of main. The question is, do we want to give the user a more obvious way to register our signal handler at the start of main?

There is of course also a downside of this since there might be blocking calls the user added which are stuck but then a custom signal handler needs to be registered.

[elfenpiff]

This is a bigger question and I created an issue to discuss it in more detail and come up with a clean solution: #1414

[elBoberido]

Yes, that's fine but Condition alone is not sufficient. It needs either a Semaphore or a Condition with a predicate else there is a race between the main thread wait() and worker thread notifyOne/notifyAll. If notify is called before the main thread enters wait, the thread blocks.

[elBoberido]

Only small nitpicks. Can be merged once the integration test issue is fixed

[FerdinandSpitzschnueffler]

Didn't review completely.

[elBoberido]

Hahaha, I didn't know this exists.

Not required but it feels a bit weird to read isThreadStarted.wait(). Can you come up with a more lyrical name like threadStartSync.wait() of so?

We can keep it if you like it as it is or don't find something that satisfies you.

[elBoberido]

Oh boy ... I think I even reviewed this 😕