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
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -368,8 +368,12 @@ def _terminate(self) -> None: | ||||||||||||||||
| status = proc.wait() # Ensure the process goes away. | ||||||||||||||||
| self.status = self._status_code_if_terminate or status | ||||||||||||||||
| except (OSError, AttributeError) as ex: | ||||||||||||||||
| # 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. | ||||||||||||||||
CopilotAI | ||||||||||||||||
| # 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.)
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| from git.cmd import Git | ||
| class _DummyProc: | ||
| """Minimal stand-in for subprocess.Popen used to exercise AutoInterrupt. | ||
| We deliberately raise AttributeError from terminate() to simulate interpreter | ||
| shutdown on Windows where subprocess internals (e.g. subprocess._winapi) may | ||
| already be torn down. | ||
| """ | ||
| stdin = None | ||
| stdout = None | ||
| stderr = None | ||
| def poll(self): | ||
| return None | ||
| def terminate(self): | ||
| raise AttributeError("TerminateProcess") | ||
| def wait(self): # pragma: no cover - should not be reached in this test | ||
| raise AssertionError("wait() should not be called if terminate() fails") | ||
| def test_autointerrupt_terminate_ignores_attributeerror(): | ||
| ai = Git.AutoInterrupt(_DummyProc(), args=["git", "rev-list"]) | ||
| # Should not raise, even if terminate() triggers AttributeError. | ||
| ai._terminate() | ||
| # Ensure the reference is cleared to avoid repeated attempts. | ||
| assert ai.proc is None |
Uh oh!
There was an error while loading.Please reload this page.