search

ilpincy / argos3-kilobot

Kilobot plugin for the multi-robot ARGoS3 simulator
15 stars 14 forks source link

Fixed: Controller 'Destroy' now sends the correct signal to the child process. #3

Closed adentinger closed 7 years ago

ilpincy commented 7 years ago

Thanks for the patch. Before deciding that it's "the correct" signal, why do you prefer SIGKILL to SIGTERM? Did you find a case in which SIGTERM wasn't managed correctly?

adentinger commented 7 years ago

The handler on the kilolib side is called when the process receives a SIGKILL signal. There was a case where ARGoS would wait forever for the process to "stop normally" when sending the SIGTERM signal actually caused a segfault on the kilobot's process (I haven't looked into why yet but I think it was a problem in our code, not the extenstion). I just changed SIGTERM to SIGKILL, and now ARGoS stops when you close the GUI.

Since the signal that is sent is different from the signal that is handled, I just assumed that the handler was changed from a SIGTERM handler to a SIGKILL handler, but that the signal sent was forgotten to be changed.

EDIT : I just looked up the difference between SIGKILL and SIGTERM, and, according to this page, SIGKILL cannot be caught. In that case, maybe it would be better to change the handler to a SIGTERM handler so that we can cleanup before finishing. That wouldn't solve our segfault problem, but as long as it's not a bug in the extension that caused the segfault I don't think it matters. What do you think?

ilpincy commented 7 years ago

I took your feedback into account, and fixed signal handling. I kept SIGTERM because that's supposed to be the right signal, and it works in all of my tests. If you could update and confirm that it works for you too, it would be great. Thanks for your feedback! :)