Race condition between ProbeProcessAliveWithSigTerm in RouDi and SignalWatcher singleton in applications about iceoryx

[gpalmer-latai]

Thanks for the feedback. I'm going to try the suggested workaround for now, and I agree the proper thing to do is to de-singleton the runtime. If I ever get the resources to do so I might help contribute to this (after I get through my backlog of other things I want to upstream).

[elBoberido]

In this case I could at least guide you a little bit.

[elBoberido]

@gpalmer-latai there is a reason I don't like singletons.

I think I have a workaround and a solution. Since the destruction order of static variables is in reverse order of their construction, you could just call hasTerminationRequested before initRuntime.

Unfortunately, we need to reset the signal handler in the dtor, else if a signal was raised after the dtor it would access a destroyed semaphore. But it should be possible to just register an empty function as signal handler. AFAIK, this should prevent immediate termination and since the dtor of the SignalWatcher runs after main, it shouldn't have a negative effect. @elfenpiff what do you think?

There is a third solution but it is not yet fully workable for you. The experimental posh API does not use a singleton for the runtime but a local variable. With this, it is also guaranteed that the unregistering from RouDi is finished before the dtor of the static SignalWatcher runs. Unfortunately, not all endpoints are already ported and request-response and the listener are not yet available.

[elfenpiff]

@elBoberido @gpalmer-latai

Since the destruction order of static variables is in reverse order of their construction, you could just call hasTerminationRequested before initRuntime.

In one translation unit you are right, but when you have multiple ones this is no longer guaranteed. In this use case it should work but I wouldn't recommend this for production code. However, it is perfect for a quick intermediate solution to remove the annoying failures and gives us time to solve this issue cleanly.

But it should be possible to just register an empty function as signal handler. AFAIK, this should prevent immediate termination and since the dtor of the SignalWatcher runs after main, it shouldn't have a negative effect.

It would render the SignalWatcher useless in all other cases, but I agree that something like this is the only viable solution, when we have a singleton runtime. I am pointing this out because I think I used the SignalWatcher before to capture signals like SIGABRT when the shared memory is set to zero - to provide a useful error output to the user.

Also, I would not fix a design mistake we made - with the singleton - by adding another mistake in the SignalWatcher, which would then have surprising behavior. The contract is clear: register a signal, and when it goes out of scope, it restores the previous signal catching calls - maybe another instance has overridden some kind of signal handler, and with this change, we would break it.

There is a third solution but it is not yet fully workable for you. The experimental posh API does not use a singleton for the runtime but a local variable. With this, it is also guaranteed that the unregistering from RouDi is finished before the dtor of the static SignalWatcher runs. Unfortunately, not all endpoints are already ported and request-response and the listener are not yet available.

I would recommend this path. As far as I remember @elBoberido you mentioned that less than a week has to be invested to finish the featureset.

Otherwise, it feels like that we try to reduce bugs/technical debt by introducing even more technical debt.

[elBoberido]

@elfenpiff when my memories from creating the API for the Publisher, Subscriber and WaitSet were fresh, it would have taken me about two days but now I guess it takes at least one more day to get into the details again.