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

Added SIGTERM handler for linux and closed session when reading error is caught #252

Merged
merged 4 commits into from
Aug 5, 2024

Conversation

CihatAltiparmak
Copy link
Contributor

@CihatAltiparmak CihatAltiparmak commented Jul 29, 2024

Addresses #251

I have no chance to try it in windows machine. but seems windows handles all signals in one handler function.

@CihatAltiparmak CihatAltiparmak changed the title Added SIGTERM handler for linux and closed session when reading error is catched Added SIGTERM handler for linux and closed session when reading error is caught Jul 29, 2024
Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Thanks for the enhancement! Left a minor comment to consider

rmw_zenoh_cpp/src/zenohd/main.cpp Outdated Show resolved Hide resolved
rmw_zenoh_cpp/src/zenohd/main.cpp Outdated Show resolved Hide resolved
@CihatAltiparmak
Copy link
Contributor Author

I'm ready for second review @Yadunund , but i want to mention just one point. If i don't remove perror in router executable, terminal looks like below when i send SIGTERM to ros2 (assuming that executes ros2 run rmw_zenoh_cpp rmw_zenohd)
. But the relavant processes are closed. This behavior is not related to my branch.
Screenshot from 2024-07-31 11-33-24

Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Thanks for iterating so quickly!

@Yadunund Yadunund merged commit a701097 into ros2:rolling Aug 5, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants