-
Notifications
You must be signed in to change notification settings - Fork 277
fix: prevent pytest hanging by avoiding threading.Lock in Process.__d… #446
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -212,8 +212,33 @@ def __exit__(self, exc_type, exc_val, exc_tb): | |||||||||||||||||||
| self.cleanup() | ||||||||||||||||||||
|
|
||||||||||||||||||||
| 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 | ||||||||||||||||||||
| if hasattr(self, 'pid') and self.pid: | ||||||||||||||||||||
| try: | ||||||||||||||||||||
| if self.pid.poll() is None: | ||||||||||||||||||||
| self.pid.kill() | ||||||||||||||||||||
| self.pid.wait() | ||||||||||||||||||||
|
||||||||||||||||||||
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is a regression fix for an interpreter-shutdown hang, but there’s no automated test asserting that
Process.__del__avoids callingcleanup()/ProcessManager.unregister_process()(the lock-acquiring path). Adding a small unit test that patchesProcess.cleanupand/orProcessManager.unregister_processto raise and verifies__del__doesn’t touch them would help prevent this from coming back.