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

On Windows, CTRL-C needs to terminate MiniZinc and its child processes #191

Open
aklietz opened this issue Jan 14, 2025 · 2 comments
Open
Assignees

Comments

@aklietz
Copy link

aklietz commented Jan 14, 2025

The fix that had I submitted for issue #171 to send signal.CTRL_C_EVENT in lieu of proc.terminate() is not working. MiniZinc is running in a separate console window, and there is an undocumented
limitation in GenerateConsoleCtrlEvent():

msg315109

sending Ctrl+Break requires already being attached to the same console as the target.. Sending a control event is implemented by calling WinAPI GenerateConsoleCtrlEvent ... if you started the process as a new group, then its ID is also the group ID. For example, p = subprocess.Popen('vault.exe', creationflags=subprocess.CREATE_NEW_PROCESS_GROUP). In this case you can send Ctrl+Break via os.kill(p.pid, signal.CTRL_BREAK_EVENT). Windows will generate the event only in the subset of processes that are both in the target process group and currently attached to the console that originated the request.

The problem is that MiniZinc is running in a different console window. The driver creates the MiniZinc process with CREATE_NEW_CONSOLE. I tried changing it to CREATE_NEW_PROCESS_GROUP, on the theory that it would keep the same console handle, but it didn't work.

Microsoft strongly discourages using the Win32 Console API in new code because it doesn't play well with Windows Terminal (wt). It is also quite buggy.

I tried reverting #171 to using proc.terminate(), which had worked on earlier versions of Python. However, testing shows that as of Python 3.12, _ = proc.wait() will always hang until the child solver subprocess finishes. Furthermore, it leaves the Python interpreter in a bizarre state where pressing Ctrl-C at the >>> prompt instantly kills the Python interpreter. The weird behavior is sticky across multiple launches of the Python interpreter. I had to close and re-open the console window to eliminate the broken behavior.

Even more weird is if I insert a 1-second delay await asyncio.sleep(1) just ahead of _ = proc.wait(), the wait returns immediately as expected and correctly re-raises KeyboardInterrupt. Without the sleep it hangs both with CTRL_C_EVENT and proc.terminate(). In both cases the child solver subprocess keeps running after MiniZinc dies.

I strongly suspect that asyncio.ProactorEventLoop has a timing bug in handling Windows I/O Completion Ports (IOCPs). The event notification for the OVERLAPPED object is getting dropped somehow, causing proc.wait() to block indefinitely.

The fix is as follows:

--- a/src/minizinc/instance.py
+++ b/src/minizinc/instance.py
@@ -659,9 +659,8 @@ class Instance(Model):
                 # an unexpected Python exception occurred
                 # First, terminate the process
                 if sys.platform == "win32":
-                    import signal
-
-                    proc.send_signal(signal.CTRL_C_EVENT)
+                    import subprocess
+                    subprocess.call(['taskkill.exe', '/PID', str(proc.pid), '/T', '/F'])
                 else:
                     proc.terminate()
                 _ = await proc.wait()
                 raise

The taskkill /F flag kills MiniZinc. The /T flag terminates the child subprocess(es).

As a side effect, invoking taskkill also fixes the IOCP timing problem.

Otherwise, you must write a bunch of Python code to dig out the proc.pid (not accessible currently), and then use psutil to loop over all the process descendants recursively. This creates a dependency on the psutil module, which is not installed by default, plus you still have the timing problem.

@aklietz
Copy link
Author

aklietz commented Jan 14, 2025

I finally figured out that if proc.send_signal(signal.CTRL_C_EVENT) cannot do GenerateConsoleCtrlEvent(), it secretly falls back to calling TerminateProcess() instead.

This explains a lot of the confusion about it appearing to work on other console windows.

@aklietz
Copy link
Author

aklietz commented Jan 19, 2025

Fix amended to remove pylint warnings.

--- a/src/minizinc/instance.py
+++ b/src/minizinc/instance.py
@@ -659,9 +659,8 @@ class Instance(Model):
                 # an unexpected Python exception occurred
                 # First, terminate the process
                 if sys.platform == "win32":
-                    import signal
-
-                    proc.send_signal(signal.CTRL_C_EVENT)
+                    import subprocess
+                    subprocess.call(['C:/Windows/System32/taskkill.exe', '/PID', str(proc.pid), '/T', '/F']) # noqa S603
                 else:
                     proc.terminate()
                 _ = await proc.wait()

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

No branches or pull requests

2 participants