Fix #931 by using psutil to terminate #932
Conversation
laundmo
force-pushed
the
master
branch
2 times, most recently
from
1d087b5
to
b7fe0f4
Compare
py4web/core.py
Outdated
| _, still_alive = psutil.wait_procs(children, timeout=2) | ||
| for child in still_alive: | ||
| child.kill() # unfriendly termination | ||
| main_process.terminate() |
There was a problem hiding this comment.
does this really work? The issue is with threads that seem to get stuck, not processes?
There was a problem hiding this comment.
Well, worst case its the same as the previous SIGKILL, but also works on windows.
I haven't noticed any threads sticking around, but i'm also on Windows so its only tested there.
Anyone working on linux could always just test it with pip uninstall py4web && pip install git+https://github.com/laundmo/py4web
There was a problem hiding this comment.
wait no, i'm wrong: .terminate() sends a SIGTERM not a SIGKILL - i could replace it with .kill() to keep sending a SIGKILL which would mean it works exactly as before when it concerns the main process.
There was a problem hiding this comment.
The issue is with threads that seem to get stuck, not processes?
After further consideration, I think I may have misunderstood originally. You're questioning whether this will stop even if a thread is currently "stuck" doing some long-term work. As a test, I added a while True: loop to my index controller, which of course leads to that page just waiting. I can still stop py4web with CTRL+C even after this, so it seems that isn't a concern.
There was a problem hiding this comment.
I will test this some more. if it works on linux then I trust you it works on windows. ;-)
There was a problem hiding this comment.
@mdipierro Any update on this? would be nice to switch back from using my fork to the main release
There was a problem hiding this comment.
I tested it on Linux and this fails to stop the threads. Can you provide a PR that only affects windows?
There was a problem hiding this comment.
Could you please elaborate on the exact difference you observed between this PR and the released version which just sends SIGKILL?
Threads can't exist without a process on any OS, so does it actually fail to kill some process?
There was a problem hiding this comment.
In case you haven't noticed: after looking at psutil a bit more, i've updated the code. its now using .kill() instead of .terminate() for the main process.
on windows, both .kill() and .terminate() behave the same way, killing the process. But on linux, .kill() sends SIGKILL and .terminate() sends SIGTERM.
This means that for linux, this solution is now no different from the previous os.kill(os.getpid(), signal.SIGKILL)
If this doesn't solve the issue you've noticed, is is possible that this was already an issue before this PR?
…uch as missing signals and process group issues
This fixes
by using psutil to terminate the process tree. This should avoid any of the issues mentioned in https://discuss.python.org/t/terminateprocess-via-os-kill-on-windows/30882
i'm unsure if this will require extra work on https://github.com/nicozanf/py4web-pyinstaller as psutil includes native code (C)