Skip to content

Ignore errors when deleting files and folders on Windows #3289

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
wants to merge 3 commits into from

Conversation

pkch
Copy link
Contributor

@pkch pkch commented Apr 30, 2017

This, in combination #3288, removes all known Windows test errors due to failed deletes (note: this PR has no overlap with #3288 and the closed #3239 ).

The change in this PR affects primarily Windows - by catching OSError when deletion fails.

In couple of cases, it also affects Linux in the sense that it checks whether a folder is present before attempting to shutil.rmtree it, and catches errors when attempting to cleanup a TemporaryDirectory. However, in the specific contexts where it happens, this change is harmless.

One potential issue is not solved in this PR, but it is not present in our codebase at the moment, and we should avoid introducing it anyway, so there's nothing to worry about. Still, just for reference in case this comes up:

TemporaryDirectory, in its finalizer, deletes the temporary folder without catching OSError (the finalizer is called from gc and from atexit). If cleanup() is called explicitly, the finalizer is detached, and that's what we do (and should continue to do) in our codebase (of course, I put cleanup() call inside try/except). However, if a TemporaryDirectory is accidentally allowed to go out of scope or survive till interpreter exit without a cleanup() call, the finalizer will kick in, and on Windows might raise an exception (if called from atexit) or print stuff to stderr (if called from gc). If we did want to fix this, we could simply wrap TemporaryDirectory in our own class that has its own finalizer that calls cleanup() of the wrapped TemporaryDirectory instance. The finalizer semantics would ensure that our own finalizer is called first (since gc can't touch the wrapped object while there are references to it, and atexit will call our finalizer first as it was created later). As I said though, this is unnecessary since we always explicitly call cleanup().

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer not to add a dependency on the mypy package to files in misc. Haven't seen the rest in detail yet.

@gvanrossum
Copy link
Member

(The rest LGTM.)

misc/util.py Outdated
@@ -0,0 +1,9 @@
import os
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please just restore the original code. This folder is not a package, and I really don't want a toplevel module named 'util'. (And there are many good reasons why the folder should not be made into a package.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh right I didn't notice util.py would become a top-level module..

@gvanrossum
Copy link
Member

I'm still at most lukewarm about this, since I've never heard any reports of errors due to os.remove() or shutil.rmtree().

@pkch
Copy link
Contributor Author

pkch commented May 2, 2017

Confirmed, I don't see any AppVeyor test failures due to this. I did have local test failures, however I'd be perfectly fine to just ignore them. So we can close this until we see any failures due to delete.

@gvanrossum
Copy link
Member

OK, let's close for now. Thanks for thinking about the issue!

@gvanrossum gvanrossum closed this May 2, 2017
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

Successfully merging this pull request may close these issues.

2 participants