Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use self-pipe trick to implement signal handlers #618

Merged
merged 9 commits into from
Aug 21, 2024

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Jul 26, 2024

🦟 Bug fix

Fixes #530

Summary

This uses the self-pipe trick to implement our signal handling so that downstream users can use any function in a callback registerd to gz::common::SignalHandler.

TODO

  • Documentation
  • Windows

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

This allows the rest of the Gazebo codebase to use callbacks that can
use non async-signal-safe functions

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
@azeey azeey requested a review from iche033 August 12, 2024 18:58
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea of using a self-pipe here is looks good. I wonder if the implementation is cross platform (windows..)? If not, it maybe possible to implement reading the pipe with a condition variable.

I tested gz-sim and sending Ctrl+C at different times. I think it may have resolved the issue I ran into before here: gazebosim/gz-sim#2501 (review) as I haven't been able to reproduce it with this branch.

The advantage of a self-pipe is that it should be able to handle multiple signals fired in quick succession right? Maybe we could expand the test to cover the case when multiple signals are fired at the same time and verify the pipe still works fine.

src/SignalHandler.cc Outdated Show resolved Hide resolved
src/SignalHandler.cc Outdated Show resolved Hide resolved
src/SignalHandler.cc Outdated Show resolved Hide resolved
@azeey azeey marked this pull request as ready for review August 15, 2024 23:03
@azeey azeey requested a review from marcoag as a code owner August 15, 2024 23:03
@azeey
Copy link
Contributor Author

azeey commented Aug 15, 2024

The advantage of a self-pipe is that it should be able to handle multiple signals fired in quick succession right? Maybe we could expand the test to cover the case when multiple signals are fired at the same time and verify the pipe still works fine.

No, the main advantage is that our callbacks can freely use any function they want including printing to the console. Previously, since the callbacks were called in a signal handler, they were limited to using async-signal-safe functions. However, most of our callbacks did not comply with this and so risked causing a deadlock when a signal was fired. For example, malloc or new is not async-signal-safe, but one of the signal handlers in gz-sim emits a Stop event which requires allocating memory. If before the signal was fired, another thread was allocating memory, trying to emit this event inside the signal handler will lock up since it'll internally try to lock a mutex when calling malloc/new. The same is true for using functions like gzdbg to print to the console which is not permitted inside a signal hander.

I could add a test that allocates memory inside a callback, but I don't think the test would fail if we had a regression. For example, if we went back to the previous implementation, the test would still pass most of the time. So I'm not sure if it's worth the effort.

I wonder if the implementation is cross platform (windows..)?

I've added (hopefully) windows support in 258a565. I doubt we can use condition variables since none of the pthread_mutex_* or pthread_cond_* functions are async-signal-safe.

@azeey azeey added the Breaking change Breaks API, ABI or behavior. Must target unstable version. label Aug 20, 2024
@iche033
Copy link
Contributor

iche033 commented Aug 21, 2024

oh ok I was thinking of just a simple test to check if the handler is re-entrant. If it's not worth the effort then we can leave it. The new updates look good to me.

Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey
Copy link
Contributor Author

azeey commented Aug 21, 2024

oh ok I was thinking of just a simple test to check if the handler is re-entrant. If it's not worth the effort then we can leave it. The new updates look good to me.

Add a rapid fire test in a6c47de and 06d0586

@azeey azeey enabled auto-merge (squash) August 21, 2024 05:39
@azeey azeey merged commit b733ac3 into gazebosim:main Aug 21, 2024
10 checks passed
@azeey azeey deleted the self_pipe branch August 21, 2024 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection Breaking change Breaks API, ABI or behavior. Must target unstable version. 🏛️ ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants