fix: prevent pytest hanging by avoiding threading.Lock in Process.__d…#446
Conversation
…el__ During interpreter shutdown, Python tears down module globals including the threading module. When Process.__del__ called cleanup() which called ProcessManager.unregister_process(), it tried to acquire a threading.Lock. This caused an infinite process-restart loop, making pytest tests/ hang indefinitely after test completion. The fix replaces the full cleanup() call in __del__ with lightweight inline cleanup that only kills subprocesses and closes file descriptors, skipping the lock-dependent ProcessManager.unregister_process() call. The atexit handler already handles bulk process cleanup at exit. https://claude.ai/code/session_017pnDqUB249KbWHseNGTS16
There was a problem hiding this comment.
Pull request overview
Fixes pytest hangs during interpreter shutdown by preventing Process.__del__ from taking the ProcessManager lock (which can be unsafe once threading globals are torn down), while still performing best-effort subprocess/FD cleanup.
Changes:
- Replace
__del__’s fullcleanup()call with inline, lock-free cleanup (kill subprocess + close streams/devnull handles). - Document why
ProcessManager.unregister_process()is intentionally skipped in__del__(atexit handler does bulk cleanup).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| if self.pid.poll() is None: | ||
| self.pid.kill() | ||
| self.pid.wait() |
There was a problem hiding this comment.
In del, calling self.pid.wait() can block indefinitely if the subprocess doesn't exit cleanly (even after kill()), which risks reintroducing the “pytest hangs at shutdown” failure mode. Consider avoiding a blocking wait in the destructor (or use a short timeout and swallow subprocess.TimeoutExpired) so interpreter finalization can’t hang here.
| self.pid.wait() | |
| try: | |
| # Use a short, bounded wait to avoid hanging | |
| # interpreter shutdown if the subprocess misbehaves. | |
| self.pid.wait(timeout=0.5) | |
| except subprocess.TimeoutExpired: | |
| # Best-effort only: if the process still hasn't | |
| # exited, don't block interpreter finalization. | |
| pass |
| def __del__(self): | ||
| # During interpreter shutdown, module globals (including threading) | ||
| # may already be torn down. Acquiring a threading.Lock in that state | ||
| # can trigger an infinite-restart loop, so we only do lightweight | ||
| # cleanup here and skip ProcessManager.unregister_process entirely | ||
| # (the atexit handler already handles bulk cleanup). | ||
| try: | ||
| self.cleanup() | ||
| if getattr(self, '_cleaned_up', True): | ||
| return | ||
| # Best-effort: kill still-running subprocesses and close FDs |
There was a problem hiding this comment.
This change is a regression fix for an interpreter-shutdown hang, but there’s no automated test asserting that Process.__del__ avoids calling cleanup() / ProcessManager.unregister_process() (the lock-acquiring path). Adding a small unit test that patches Process.cleanup and/or ProcessManager.unregister_process to raise and verifies __del__ doesn’t touch them would help prevent this from coming back.
…el__
During interpreter shutdown, Python tears down module globals including the threading module. When Process.del called cleanup() which called ProcessManager.unregister_process(), it tried to acquire a threading.Lock. This caused an infinite process-restart loop, making pytest tests/ hang indefinitely after test completion.
The fix replaces the full cleanup() call in del with lightweight inline cleanup that only kills subprocesses and closes file descriptors, skipping the lock-dependent ProcessManager.unregister_process() call. The atexit handler already handles bulk process cleanup at exit.
https://claude.ai/code/session_017pnDqUB249KbWHseNGTS16