Implement /server_control::stop

[peci1]

🎉 New feature

Closes #1237

Summary

Implemented the stop part of /server_control service.

Test it

1. ign gazebo -v4
2. ign service -s /server_control --reqtype ignition.msgs.ServerControl --reptype ignition.msgs.Boolean --timeout 1000 --req 'stop: 1'
3. The server should stop and exit in a while.

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• 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

[peci1]

One question that came to my mind was what to do with the msgs::ServerControl message definition. It is apparently very much tied to Gazebo classic with the new_port member. But that's not in the scope of this PR.

[codecov]

Codecov Report

Merging #1240 (cbea6f4) into ign-gazebo3 (fe07887) will decrease coverage by 0.00%.
The diff coverage is 62.50%.

@@               Coverage Diff               @@
##           ign-gazebo3    #1240      +/-   ##
===============================================
- Coverage        77.26%   77.25%   -0.01%     
===============================================
  Files              227      227              
  Lines            13511    13535      +24     
===============================================
+ Hits             10439    10457      +18     
- Misses            3072     3078       +6     

Impacted Files	Coverage Δ
include/ignition/gazebo/Server.hh	100.00% <ø> (ø)
include/ignition/gazebo/gui/TmpIface.hh	0.00% <ø> (ø)
src/gui/TmpIface.cc	4.76% <0.00%> (-1.76%)	⬇️
src/ServerPrivate.cc	82.60% <71.42%> (-1.55%)	⬇️
src/Server.cc	84.33% <0.00%> (-0.61%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe07887...cbea6f4. Read the comment docs.

[chapulina]

Thanks, LGTM, just some minor comments.

[peci1]

I did a few tests locally, and it seems to me it'd be better to not call this->Stop() directly from the callback, but rather spin a separate thread for it. Otherwise, there could possibly be a deadlock in case the service is called from a plugin whose destructor cannot be finished until the service call finishes.

What do you think about adding this async execution?

With synchronous execution, I got some segfaults when calling the service (which practically also get to the requested point of stopping the server, but I'd say it's not the the expected way :) ). With the async implementation, I haven't seen any yet.

[peci1]

Here's a GUI plugin using the service: gazebosim/gz-gui#335 .

[chapulina]

there could possibly be a deadlock in case the service is called from a plugin whose destructor cannot be finished until the service call finishes.

I'd have to look a bit more closely. It looks like SimulationRunner is not handling the stop immediately and just sets a flag to handle the stop later.

With the async implementation, I haven't seen any yet.

But I think you're onto something. If you've seen that it works I think it's worth it pursuing that approach.

[peci1]

I've changed the callback to run in a thread.

If you look into SimulationRunner::Stop(), it emits an event, which I think can be blocking and could be the source of problems I'm talking about...

[chapulina]

LGTM, just a couple of minor comments left

[chapulina]

This is already set up-top, so I think we don't need to repeat it in every block

[peci1]

The individual parts are not else ifs, but ifs, so theoretically, the user could ask for more functions in one call. The logic I used was to report the worst result of all, i.e. report false as soon as any of the requested functionalities fails. It is set to false at the beginning, but I added these to "remind" or make sure that a failing function is reported. However, it might be wiser to turn the ifs to else ifs, as nobody probably wants to simultaneously stop the server and clone/create a world.

[chapulina]

Ah gotcha, I think future-proofing is ok

[osrf-triage]

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1

[osrf-triage]

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1