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

Improve signal handling #2501

Merged
merged 1 commit into from
Jul 31, 2024
Merged

Conversation

azeey
Copy link
Contributor

@azeey azeey commented Jul 26, 2024

🦟 Bug fix

Summary

This mainly deals with the problem where a signal is ignored if it occurs right before ServerPrivate::Run is called because we generally rely on the running variable to know when to stop, but that variable is set to true at the beginning of ServerPrivate::Run.

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.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey requested a review from mjcarroll as a code owner July 26, 2024 18:37
@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Jul 26, 2024
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
@iche033
Copy link
Contributor

iche033 commented Jul 30, 2024

checking what's a good way to test this? Launch gazebo then verify ctrl + c signal is handled right away?

@azeey
Copy link
Contributor Author

azeey commented Jul 31, 2024

This mainly improves signal handling when Gazebo starts up. So the best way to test it would be to start Gazebo in server mode (gz sim -s empty.sdf) and type ctrl+c in the first few seconds. Without this PR, I noticed the signal would sometimes be completely ignored.

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 did some manual testing and found that there is a specific point in time after launch when I hit ctrl + c, the signal is handled with these changes and ignored without these changes.

I did run into a case that gz sim would still freeze if I ctrl + c right after it starts up:

iche@jammy2:~/code/gz_i_ws$ gz sim -v 4 -s shapes.sdf
[Msg] Gazebo Sim Server v9.0.0
[Msg] Loading SDF world file[/home/iche/code/gz_i_ws/install/share/gz/gz-sim9/worlds/shapes.sdf].
^C^C^C^C^C^C^C^C

That can be addressed separately.

@azeey
Copy link
Contributor Author

azeey commented Jul 31, 2024

That can be addressed separately.

I'm hoping some of the remaining issues are related to gazebosim/gz-common#618, but there might be other instances of timing.

@azeey azeey merged commit 6b5ec20 into gazebosim:main Jul 31, 2024
10 checks passed
@azeey azeey deleted the improve_signal_handling branch July 31, 2024 21:26
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 🏛️ ionic Gazebo Ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants