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

Confusing signal handling #85

Closed
r7vme opened this issue Dec 4, 2024 · 2 comments · Fixed by #86
Closed

Confusing signal handling #85

r7vme opened this issue Dec 4, 2024 · 2 comments · Fixed by #86

Comments

@r7vme
Copy link
Contributor

r7vme commented Dec 4, 2024

Thanks for the great library. Small nitpick.

When reach was used from python script, I got "core dumps", while debugging I found out that RuntimeError was throwed. It was throwed from signal handler. Problem is that function runReachStudy already finished, but signals handlers left installed. This causes my application to fail with core dumps.

I removed signal handlers in the end of the function.

diff --git a/src/third_party/reach/src/reach_study.cpp b/src/third_party/reach/src/reach_study.cpp
index 0c9e0b4..317a3cc 100644
--- a/src/third_party/reach/src/reach_study.cpp
+++ b/src/third_party/reach/src/reach_study.cpp
@@ -382,6 +382,9 @@ void runReachStudy(const YAML::Node& config, const std::string& config_name, con
     logger->print("Press enter to quit");
     std::cin.get();
   }
+
+  signal(SIGINT, SIG_DFL);
+  signal(SIGTERM, SIG_DFL);
 }

 }  // namespace reach

What is the purpose of signal handlers in general? especially that they installed in the end of the function, when study already finished?

@marip8
Copy link
Member

marip8 commented Dec 4, 2024

What is the purpose of signal handlers in general?

The purpose of signal handlers is to call a user-provided function when OS signals (like SIGINT and SIGTERM) are emitted and before they terminate the program.

I think I implemented this to get around the issue of ROS2 launch not allowing input from stdin needed to terminate the reach study. In order to interact with the reach study markers, the reach study objects in that function need to stay in scope. In ROS1 we can wait for keyboard input to terminate the reach study once we are done interacting with the markers, but this doesn't work in ROS2. Instead, we still wait for keyboard input (which we can't get), but I re-routed the SIGINT and SIGKILL signals to a throw an exception that could be handled appropriately at the higher level (but currently isn't really handled). In retrospect, I don't think this really makes sense, and we could probably just get rid of the signal handling.

@r7vme
Copy link
Contributor Author

r7vme commented Dec 5, 2024

@marip8 thanks, I made a PR to remove signal handling #86

@marip8 marip8 closed this as completed in #86 Dec 5, 2024
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 a pull request may close this issue.

2 participants