Use signal.CTRL_C_EVENT on windows instead of signal.SIGINT

[ivanpauno]

We were using signal.SIGTERM on windows instead of signal.SIGINT.
I think that signal.CTRL_C_EVENT is the correct signal to be send in replacement of signal.SIGINT, which is not supported in SubprocessTransport.send_signal.

Related with ros2/ros2cli#315.
See pypa/setuptools#1818.

[hidmic]

I presume you tried this and it works. If so, I wonder why Python documentation states we need custom subprocess creation flags to make it work.

[ivanpauno]

I presume you tried this and it works. If so, I wonder why Python documentation states we need custom subprocess creation flags to make it work.

I've tried and it works well.

I've also read that part of the documentation, and I forgot about it after seeing that it was working 😄.
I will double check if osrf_pycommon is already adding that flag, or if we can add it.

---

This PR doesn't solve the following problem: a Python program catching KeyboardInterrupt and ignoring it (ctrl-c event also raise a KeyboardInterrupt in python). After sigterm_timeout, ExecuteProcess will send a SIGTERM signal, that will have the same problem with executables installed with entry_points console_scripts. That's why I opened an issue in setuptools repo.

[ivanpauno]

I presume you tried this and it works. If so, I wonder why Python documentation states we need custom subprocess creation flags to make it work.

After further testing, I realized that when I send the ctrl_c_event, the parent process was also being finished 😄 (it wasn't a problem in the example I tried).
And the funny thing, is that if I use creationflags=subprocess.CREATE_NEW_PROCESS_GROUP, the child process stop receiving signals (something broken on Python?).

I will just close this, and wait for an answer in pypa/setuptools#1818.

[clalancette]

For what it is worth, this change does solve the "zombie" Python processes I reported over in ros2/demos#107 (comment) . I guess it is still not the correct solution, but at least for that use case it is definitely an improvement.

[ivanpauno]

For what it is worth, this change does solve the "zombie" Python processes I reported over in ros2/demos#107 (comment) . I guess it is still not the correct solution, but at least for that use case it is definitely an improvement.

The problem is that it isn't just killing the child process, is killing the launch service itself (it's in the same process group, and ctrl_c_event work in that way on windows).
I also tried running the child process in a new process group, but it stopped receiving signals at all (a bug in python?).

The "zombie" python process is generated because the installed executable wrapper of the python program (by setuptools), is not handling TerminateProcess correctly (a Windows API equivalent to SIGTERM).

I haven't found a workaround for this combination of unfortunate bugs on Windows (sending SIGINT to asincio subprocesses is broken, creating subprocesses with CREATE_NEW_PROCESS_GROUP creationflag seems to be broken, setuptools installed console_scripts handles TerminateProcess incorrectly).

Note: Here is a branch of osrf_pycommon adding support to creationflags https://github.com/ivanpauno/osrf_pycommon/tree/ivanpauno/add-creationflags.

[ivanpauno]

I haven't tried escalating to kill, but it seems to be the same than term on Windows:
https://docs.python.org/3/library/asyncio-subprocess.html#asyncio.asyncio.subprocess.Process.kill