fix: guard AutoInterrupt terminate during interpreter shutdown#2105
fix: guard AutoInterrupt terminate during interpreter shutdown#2105lweyrich1 wants to merge 2 commits intogitpython-developers:mainfrom
Conversation
dae9b25 to
357aad1
Compare
EliahKagan
left a comment
There was a problem hiding this comment.
I've made two changes:
- Rebased this onto main after merging #2108 to fix #2107 (which arose more recently than #2105 (review) and is not due to changes here, but would have appeared here next time CI ran).
- Fixed the blocker mentioned in #2105 (review) where the lint job failed. This was due to an unnecessary
import pytest(since the module does not use anything from thepytestmodule in its own code).
I plan to merge this once CI passes.
There was a problem hiding this comment.
Pull request overview
Fixes a Windows interpreter-shutdown edge case where Git.AutoInterrupt._terminate() can raise AttributeError from subprocess.Popen.terminate() due to partially torn-down stdlib internals, preventing noisy “Exception ignored in: del” messages.
Changes:
- Treat
AttributeErrorfromproc.terminate()similarly to existingOSErrorhandling in_AutoInterrupt._terminate(). - Add a regression test that simulates
terminate()raisingAttributeErrorand asserts_terminate()does not raise and clearsproc.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
git/cmd.py |
Extends _AutoInterrupt._terminate() exception handling to include AttributeError during termination. |
test/test_autointerrupt.py |
Adds a focused regression test for AttributeError raised from terminate(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| # On interpreter shutdown (notably on Windows), parts of the stdlib used by | ||
| # subprocess can already be torn down (e.g. `subprocess._winapi` becomes None), | ||
| # which can cause AttributeError during terminate(). In that case, we prefer | ||
| # to silently ignore to avoid noisy "Exception ignored in: __del__" messages. |
There was a problem hiding this comment.
Catching AttributeError unconditionally here can mask real runtime failures when _terminate() is called outside interpreter shutdown (e.g. the kill_after_timeout path in handle_process_output). Consider only suppressing AttributeError during interpreter finalization (e.g. getattr(sys, "is_finalizing", lambda: False)()), and otherwise re-raising so unexpected terminate/wait breakages don’t silently leave processes running. Also, the comment says “silently ignore” but this logs at INFO—either adjust the wording or log level to match intent.
| # to silently ignore to avoid noisy "Exception ignored in: __del__" messages. | |
| # to log and ignore to avoid noisy "Exception ignored in: __del__" messages. | |
| if isinstance(ex, AttributeError) and not getattr(sys, "is_finalizing", lambda: False)(): | |
| # Outside interpreter finalization, an AttributeError here likely indicates | |
| # a real problem (e.g. broken subprocess internals), so re-raise it instead | |
| # of silently leaving the process running. | |
| raise |
Fixes #2102
On Windows during interpreter shutdown, process objects can be partially torn down and
terminate()may raiseAttributeError(e.g. missingTerminateProcess).This PR treats that similarly to the existing
OSErrorhandling in_AutoInterrupt._terminate()and adds a regression test covering theAttributeErrorcase.Tests:
pytest -q test/test_autointerrupt.py