Skip to content
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

Remove unused TASKKILL fallback in AutoInterrupt #1754

Merged
merged 1 commit into from
Dec 2, 2023

Commits on Dec 2, 2023

  1. Remove unused TASKKILL fallback in AutoInterrupt

    When Git.AutoInterrupt was first implemented in ead94f2, it used
    os.kill (sending SIGINT to the process to terminate it). This was
    in 2009, and not all supported versions of Python provided os.kill
    on Windows, since it was added for Windows in 2.7 and 3.2 (2.6 and
    3.1 reached EoL in 2013 and 2012, respectively). Catching
    AttributeError and running TASKKILL provided a fallback for Python
    versions in which os.kill was absent on Windows.
    
    Since then, all supported versions have os.kill on Windows, and
    also the code of GitPython, in the try-block, has changed, and no
    longer uses os.kill. It now contains four attribute access
    expressions:
    
    - proc.terminate. All currently suppported versions of Python
      (including 3.7, which GitPython still supports, and some before
      that) have Popen.terminate.
    
    - proc.wait. Same situation as proc.terminate. Popen.wait exists.
    
    - self._status_code_if_terminate. This is a class attribute of
      AutoInterrupt, accessible through its instances.
    
    - self.status. This is assigned to. AutoInterrupt is slotted, but
      "status" appears in __slots__, so this isn't an AttributeError
      either.
    
    The "except AttributeError" clause is thus no longer used and can
    be removed, which is done here. Removing it shortens and simplifies
    the code, better expresses the logic for how the wrapped process is
    actually being terminated, and eliminates the need to engage with
    any subtleties of TASKKILL (and of how it is called) when reading
    the code to verify its correctness.
    
    In addition, because they are now expected always to be available,
    if somehow an AttributeError managed to be raised in the terminate
    or wait calls, this would be very strange and we probably shouldn't
    silently catch that.
    
    (Because this AutoInterrupt._terminate method is sometimes called
    due to __del__ methods running as the interpreter is shutting down,
    it is possible for some attributes to unexpectedly be set to None,
    which could cause AttributeError indirectly if another attribute is
    looked up on the None object; and perhaps on some non-CPython
    implementations some attributes might even be deleted during
    shutdown. The _terminate method handles this where relevant. But
    the TASKKILL fallback removed here seems unrelated to that, which
    affects module attributes and global variables. The names used in
    the try-block are proc, status, and self, which are local
    variables; the except clause itself accessed os.name, which used a
    global variable and a module attribute, indicating that the intent
    was not to handle this interpreter shutdown scenario; and this
    whole issue, while not gone completely, is much less significant
    since Python 3.4, due to
    https://docs.python.org/3/whatsnew/3.4.html#whatsnew-pep-442.)
    EliahKagan committed Dec 2, 2023
    Configuration menu
    Copy the full SHA
    3f21391 View commit details
    Browse the repository at this point in the history