Skip to content

Windows: WindowsError in def _shutdown(self): in repository.py when running many commands #78

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

Closed
null77 opened this issue Oct 17, 2020 · 9 comments

Comments

@null77
Copy link

null77 commented Oct 17, 2020

Using a .23 build of stgit I could reproduce this every time just by running stg pull:

$ stg pull
Popping all applied patches ... done
Pulling from "origin"
Already up to date.
Error in atexit._run_exitfuncs:
Traceback (most recent call last):
  File "C:\src\Python27\lib\atexit.py", line 24, in _run_exitfuncs
    func(*targs, **kargs)
  File "C:\src\Python27\lib\site-packages\stgit\lib\git\repository.py", line 122, in _shutdown
    os.kill(self._proc.pid(), signal.SIGTERM)
WindowsError: [Error 5] Access is denied
Error in sys.exitfunc:
Traceback (most recent call last):
  File "C:\src\Python27\lib\atexit.py", line 24, in _run_exitfuncs
    func(*targs, **kargs)
  File "C:\src\Python27\lib\site-packages\stgit\lib\git\repository.py", line 122, in _shutdown
    os.kill(self._proc.pid(), signal.SIGTERM)
WindowsError: [Error 5] Access is denied

I fixed this with the following update:

    def _shutdown(self):
        if self._proc:
            self._proc.stdin.close()
            try:
                os.kill(self._proc.pid(), signal.SIGTERM)
            except OSError:
                pass
            self._proc.wait()

I believe what's happening here is that the PID is no longer valid and we're trying to kill an already-dead process.

@jpgrayson
Copy link
Collaborator

Sorry for the delayed response. Thanks for reporting this issue and suggesting a repair.

It would be really helpful to know if this behavior still happens on the head of the master branch with Python 3.5+. If you'd be willing to try that out, it would be appreciated.

@null77
Copy link
Author

null77 commented Aug 17, 2021

Hey @jpgrayson , this issue happens to me on Windows with Python 3.9. It seems to be flaky. StackOverflow recommends using taskkill.exe as a fallback:

https://stackoverflow.com/questions/6688815/windowserror-error-5-access-is-denied

Doing what I proposed back in the description would at least silence the error.

@jpgrayson
Copy link
Collaborator

I appreciate the followup, @null77. I've gone ahead and applied the workaround you suggested.

I believe what is happening is that closing stdin of the child process causes that process to terminate. On non-Windows platforms, the process remains in a defunct/zombie state until wait() is called and thus the os.kill() call idempotently does nothing.

On Windows, however, the process model is different and it seems that the child process just ceases to exist such that os.kill() targets a non-existent process and thus fails as shown in this issue.

Another approach might have been to eliminate the self._proc.stdin.close() call, but without a Windows platform to test on, I went with the known working workaround.

@null77
Copy link
Author

null77 commented Aug 23, 2021

Thank you! I'll give the new version a shot.

@null77
Copy link
Author

null77 commented Aug 23, 2021

@jpgrayson oops, looks like somewhere in the shuffle this exception became

Error in atexit._run_exitfuncs:
Traceback (most recent call last):
  File "C:\src\Python39\lib\site-packages\stgit\lib\git\repository.py", line 168, in _shutdown
    os.kill(self._proc.pid(), signal.SIGTERM)
PermissionError: [WinError 5] Access is denied

Originally I had posted it as a WindowsError. Now it seems the flake still happens because of the exception type being PermissionError.

@jpgrayson
Copy link
Collaborator

Thanks for the quick feedback, @null77.

Could you check the behavior with the self._proc.stdin.close() removed? I.e.:

    def _shutdown(self):
        if self._proc:
            os.kill(self._proc.pid(), signal.SIGTERM)
            self._proc.wait()

My theory is that the explicit close of stdin is causing this race and I don't know why explicitly closing stdin would be necessary anyway.

@jpgrayson jpgrayson reopened this Aug 23, 2021
jpgrayson added a commit that referenced this issue Aug 23, 2021
Addresses #78

Closing stdin of the child git process to terminate itself. On Windows, the
subsequent attempt to kill the already-terminating child process results in
an exception (PermissionError or WindowsError or OSError). On Linux, the
terminated child process remains in a defunct/zombie state so that the
subsequent kill is idempotent and the child is finalized by wait().

Signed-off-by: Peter Grayson <[email protected]>
jpgrayson added a commit that referenced this issue Aug 23, 2021
This allows CatFileProcess and DiffTreeProcess to use Popen.terminate()
instead of os.kill(). Popen.terminate() uses _winapi.TerminateProcess() and
comprehends the potential PermissionError if the process is already
terminated.

This also allows using the Popen instance as a context manager, where its
__exit__() method cleans-up descriptors and waits for the terminated child
process.

And since all current uses of Run.run_background() do not want to use an
encoding for stdin/stdout, asserts are added to only allow this mode.

Lastly, Popen.stdout.read1() is used instead of os.read(), thus elimnating
all instances of os interfaces being used with these background processes.

Affects #78.

Signed-off-by: Peter Grayson <[email protected]>
@jpgrayson
Copy link
Collaborator

I'm pretty confident that the latest changes (8206770 and 499ed60) resolve this. Closing.

@null77
Copy link
Author

null77 commented Aug 30, 2021

Thanks @jpgrayson ! Sorry I didn't follow-up earlier. I removed the stdin close and the error was no longer showing up.

@jpgrayson
Copy link
Collaborator

👍 Thank you for the follow-up @null77. Please file more issues if you see any more problems on Windows. Also, note discussion #152 which I just posted yesterday. I'd really like StGit to provide a good experience on Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants