-
-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
harden directory removal for tests on Windows #59701
Comments
Currently, removing directories during testing on Windows w/NTFS can causing sporadic failures due to access denied errors. This is caused by other processes getting a handle to the directory itself (change notifications) or a handle to a file within the directory. The most notable offender is the Indexing Service. Most (but not all!) external programs can be configured to ignore the development directory. For example, the Indexing Service can be disabled or have those directories ignored. TortoiseSVN is another offender that can exclude directories but each directory needs to be listed separately so it is easy to forgot one. On my machine I have programs that simply do not have an option to ignore any directories thus causing some grief during testing. The attached patch to test.support eliminates the need to disable or and ignores to any programs (tested on a Win7-x64 i7-3770K@4.3GHz). It achieves this by checking for the removal of the directory before returning to the caller. It performs an exponential backoff timeout loop that amounts to a total of ~1 second in the worst case. If the directory is not removed from the filesystem by then, it will probably be in error anyway. However, the loop is seldom executed more than once. |
This is a (near) duplicate of bpo-7443, I think. |
Partially so it seems. However, my testing with Process Monitor (from The blind rename-remove dance doesn't work when removing entire For some background for newcomers, see http://support.microsoft.com/kb/159199 For testing, I used regrtest -F -j6 test_import Without patching it would fail consistently (albeit randomly). |
I must also add that the proposed solution works well within the test suite as the access denied error can also occur when creating subsequent files, not just removing them. This solution eliminates the need to wrap all creation calls with access denied handling, a huge plus IMHO. |
I'm +1 on the approach in principle. I'm tentative about using ctypes |
My inclination is to say that using ctypes is a reasonable option for improving Windows buildbot stability in the near term, but we'd probably want to move this into _winapi long term. Adding Antoine & MvL to get their opinion. |
I wonder why it couldn't use os.listdir to find out whether the directory is empty, and os.stat to find out whether a specific file or directory still exists. |
What are those programs exactly? A buildbot should ideally have a lean system install that does not interfere with the tests running. |
My development machine has add'l programs, not the buildbot machine. |
ctypes is already used later in test_support so I figured it was fine |
It is possible to do the same thing with just os.listdir. The use of |
But it's certainly much easier to maintain using regular Python APIs. We |
OK, here is another patch that uses just os.listdir |
I fail to see the point of not using os.stat. IIUC, Windows will delete the file once the last handle is been closed. Since stat will close any handle it temporarily gets, it will not prolong the live of the file; the file will still go away when the last process has closed it. Performance is not an issue at all here, since we are waiting for the deletion of the file anyway. So checking whether the file is in the directory listing is fine with me as well. Unless someone can demonstrate how os.stat can prevent removal of the file, I'd like to see the comment corrected, though. |
I've updated the comment in the patch to reflect Martin's concern. Martin is partially correct in that the handle opened in the stat() call will not prolong the pending status. It is due to the fact that it does not open the handle with any sharing mode set, thus effectively blocking any other process from grabbing another handle to the file while the stat function has its handle open. |
With the latest changes, is there anything left preventing the Without some change, the Win64 buildbot is relatively irrelevant as it |
Not that some change isn't necessary, but what else are you running on your build slave? I ran a Windows 2008 R2 x64 slave for some time and it never had issues around file/directory removal. I only had to decommission it because the physical machine became unreliable. |
The errors only started happening after upgrading the HD from a PATA |
Brian, Tim, do you think this should be committed? |
The latest patch to test.support looks reasonable. Go for it. |
Fine with me |
New changeset fcad4566910b by Brian Curtin in branch '3.2': |
New changeset c863dadc65eb by Brian Curtin in branch '2.7': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: