Keep service around until client exits#48
Conversation
|
the "while true" loop in the service was leading to some problems with the return code, so I've changed that to a sleep for 5 seconds, which shouldn't give an error if interrupted. http://ci.ros2.org/job/ci_osx/979/ I believe the alternative is replicating the signal handling code in rclcpp in the rcl test suite. |
rcl/test/rcl/service_fixture.cpp
Outdated
| // Our scope exits should take care of fini for everything | ||
| // stick around until we are killed by the client | ||
| // To avoid an infinite loop and signal handling, just sleep for a "long" time | ||
| std::this_thread::sleep_for(std::chrono::seconds(3)); |
There was a problem hiding this comment.
Why not spin here instead? Or sleep in a loop?
There was a problem hiding this comment.
After discussing with @jacquelinekay offline, we agreed that adding a new exit handler to launch which allows for the "sigterm" return code after the server was interrupted by launch is the right way to go. Then the server can just busy-loop wait forever rather than do a single sleep here.
|
Why is this sleep needed: https://github.com/ros2/rcl/pull/48/files#diff-dfc3a1157a17788738b1748a8bce57d4R116 ? |
|
This change now needs ros2/launch#27 |
|
I think the way this uses the exit handlers doesn't work as you expect it to. If the second executable (which is the client) has a a primary exit handler its return code will determine the rc of the launch file. When the client exits it will sigint / sigkill all other processes. But the new |
|
I see. If I only want the launch file to return 0 if the client returned zero and the service returned zero or sigint/sigkill, how would I implement this in launch? Can I access the original return code of the other process containing the primary exit handler from the process that is getting shut down? I assume |
|
The ignore signal handler should simply not be a primary exit handler, since you want to use it on the non-primary executable. It never needs to change the launch rc directly. If The primary exit handler can then still decide to use |
|
Continuing this discussion in ros2/launch#27 |
e95d2d5 to
8df7164
Compare
8df7164 to
8001ed1
Compare
|
I removed the extraneous sleep that @wjwwood pointed out. I don't think it's need since |
|
@jacquelinekay to run CI for other platforms. |
|
The changed test passes for all platforms. Therefore I think this is good to merge. |
|
lgtm, but since @dirk-thomas was previously commenting, I'd wait for feedback from him too. |
8182943 to
667f1f3
Compare
|
My comment about the exit handler was addressed within the launch PR. +1 |
changes to support rcl refactor
Connects to ros2/launch#27
The remote services test is flaky for Connext because the service process needs to stick around until the client exits.