Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork966
fix: guard AutoInterrupt terminate during interpreter shutdown#2105
Conversation
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.
Thanks a lot for the fix, looks good to me!
I will merge once CI is fixed.
dae9b25 to357aad1Compare
EliahKagan left a comment
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
Fixes a Windows interpreter-shutdown edge case whereGit.AutoInterrupt._terminate() can raiseAttributeError fromsubprocess.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 includeAttributeError during termination. |
test/test_autointerrupt.py | Adds a focused regression test forAttributeError raised fromterminate(). |
💡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. |
CopilotAIMar 9, 2026
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.
Catching AttributeError unconditionally here can mask real runtime failures when_terminate() is called outside interpreter shutdown (e.g. thekill_after_timeout path inhandle_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.
| # tosilently ignore to avoid noisy "Exception ignored in: __del__" messages. | |
| # tolog and ignore to avoid noisy "Exception ignored in: __del__" messages. | |
| ifisinstance(ex,AttributeError)andnotgetattr(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 |
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.
There may be some merit to this concern, but I don't think it needs to block the change here, sinceAttributeError really should not arise here in any other way. I'm also not entirely sure that a change along the lines of what the model suggests here would be robust enough to be worth doing. But probably some future change to make this more robust, or do log in more detail when the capability to do so is present, or both, could be worthwhile. (I reiterate that I don't consider this blocking.)
e23c548 intogitpython-developers:mainUh oh!
There was an error while loading.Please reload this page.
EliahKagan commentedMar 9, 2026
Thanks@lweyrich1! |
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