fix: guard AutoInterrupt terminate during interpreter shutdown by lweyrich1 · Pull Request #2105 · gitpython-developers/GitPython
self.status = self._status_code_if_terminate or status except OSError as ex: _logger.info("Ignored error after process had died: %r", ex) 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.
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. 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 |
Copilot uses AI. Check for mistakes.
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, since AttributeError 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.)
def __del__(self) -> None: