Repository navigation
test.support.unlink issue on Windows platform #51692
Description
| BPO | 7443 |
|---|---|
| Nosy | @loewis, @jaraco, @ncoghlan, @pitrou, @tjguk, @bitdancer, @briancurtin, @asvetlov, @florentx, @cjerdonek |
| Files |
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:
assignee = 'https://github.com/tjguk'
closed_at = <Date 2013-07-31.15:12:19.626>
created_at = <Date 2009-12-05.15:20:02.770>
labels = ['type-bug', 'tests', 'OS-windows']
title = 'test.support.unlink issue on Windows platform'
updated_at = <Date 2013-07-31.15:26:00.800>
user = 'https://github.com/asvetlov'bugs.python.org fields:
activity = <Date 2013-07-31.15:26:00.800>
actor = 'r.david.murray'
assignee = 'tim.golden'
closed = True
closed_date = <Date 2013-07-31.15:12:19.626>
closer = 'tim.golden'
components = ['Tests', 'Windows']
creation = <Date 2009-12-05.15:20:02.770>
creator = 'asvetlov'
dependencies = []
files = ['15454', '15455', '16839', '16869']
hgrepos = []
issue_num = 7443
keywords = ['patch', 'needs review']
message_count = 34.0
messages = ['95991', '95992', '95993', '96004', '96012', '99089', '99093', '101355', '102049', '102137', '102685', '102696', '102698', '102720', '102833', '102837', '102860', '102863', '102869', '102873', '102880', '114124', '132203', '132326', '132330', '132333', '132352', '132589', '132766', '132787', '132793', '167191', '194002', '194003']
nosy_count = 14.0
nosy_names = ['loewis', 'jafo', 'jaraco', 'ncoghlan', 'pitrou', 'techtonik', 'tim.golden', 'nadeem.vawda', 'r.david.murray', 'brian.curtin', 'asvetlov', 'flox', 'chris.jerdonek', 'jdigital']
pr_nums = []
priority = 'high'
resolution = 'works for me'
stage = 'needs patch'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue7443'
versions = ['Python 3.3']
Activity
asvetlov commentedon Dec 5, 2009
On Windows there are tiny delay between call to os.unlink and real file
removing. Periodically it leads to unittest crashes in cases like this:
Proposed solution: wait in support.unlink for end of deletion asking
os.stat for removed file (only if os.name == 'nt', of course).
Also test.test_linecache:LineCacheTests.test_checkcache should be fixed
Both patches for trunk and py3k is attached.
asvetlov commentedon Dec 5, 2009
patch for python 2 is attached
asvetlov commentedon Dec 5, 2009
Problem can be reproduced with several run of test_bufio in both python
versions. trunk contains more tests (+1 test case) so it's easer to catch
'access denied' exception on it.
loewis commentedon Dec 5, 2009
I'm fairly skeptical that your analysis is right. Can you prove that a
file still exists after unlink returns?
asvetlov commentedon Dec 5, 2009
You right, problem is not in os.unlink (Windows call DeleteFile) itself.
I have TortoiseSVN (very popular Explorer extension to work with
Subversion) installed.
This tool run TSVNCache.exe process to update own data in background.
TSVNCache.exe receive NotifyChangeDirectory events for svn checkout
folders.
support.TESTFN is tempfile name in current working directory. If I try
to run unittests from some folder from python checkout TSVNCache.exe get
change notify and analyze it.
So, sometimes I can see race condition:
and lock deleted file by holding opened handles to it.
According to MSDN for DeleteFile function:
The DeleteFile function marks a file for deletion on close. Therefore,
the file deletion does not occur until the last handle to the file is
closed. Subsequent calls to CreateFile to open the file fail with
ERROR_ACCESS_DENIED.
We get exactly same.
actually deleted.
I see this situation in sysinternals Process Monitor tool.
Probability of race condition is tiny but non-zero. Intensive
create/drop/create again sequences can catch this one.
florentx commentedon Feb 9, 2010
Another report of something looking very similar on bpo-7712.
florentx commentedon Feb 9, 2010
Note: the fix in test_linecache.py is useless.
The "with open(source_name, 'w') as source:" context manager is in charge of closing the file on __exit__.
jafo commentedon Mar 20, 2010
Andrew: There have been changes committed within the last week to bpo-7712, which Florent suggested might be related. Can you please re-test this to see if it still exists, and if it does, bug me and I'll try to get some more movement on this.
techtonik commentedon Apr 1, 2010
It would be nice to see standalone test case that illustrates the problem.
asvetlov commentedon Apr 1, 2010
Florent, sorry.
I have no Windows workstation now, so I cannot follow this issue.
tjguk commentedon Apr 9, 2010
This is basically a rerun of this discussion a couple of years ago:
http://mail.python.org/pipermail/python-dev/2008-April/078333.html
The problem certainly still happens against trunk -- I have a semi-aggressive test-harness which can cause it to reproduce pretty much on-demand. I proposed an approach here:
http://mail.python.org/pipermail/python-dev/2008-April/078339.html
but when I started digging into test_support it all got a bit hairy because -- naturally -- test.support.unlink is used in a *lot* of places. In short, there's still a problem to be fixed. I believe that a rename-unlink dance would solve it, but only at the cost of affecting a lot of tests.
techtonik commentedon Apr 9, 2010
It is unlikely that it will go further then discussion unless this bug can be reliably reproduced to be debugged. If not testcase when at least Process Monitor log would be helpful.
10 remaining items
ncoghlan commentedon Aug 17, 2010
I closed bpo-9627 as a duplicate of this - the buildbot failure referenced there was most likely due to something else holding open a temporary file that the test suite thought was closed.
asvetlov commentedon Mar 26, 2011
As I can see cygwin's unlink_nt uses Nt* functions family (NtSetInformationFile etc) which are part of Windows DDK.
Do you like to use them or prefer SDK ones (say SetFileInformationByHandle)?
In second case python unlink can borrow deletion schema from cygwin for modern Windows versions (Vista+) and return to legacy trivial DeleteFile call if OS is WinXP.
pitrou commentedon Mar 27, 2011
FWIW, Mercurial uses the following dance:
http://selenic.com/repo/hg/file/463aca32a937/mercurial/windows.py#l296
(Mercurial is under the GPL, so we can't copy that code verbatim; but it can serve as an inspiration)
tjguk commentedon Mar 27, 2011
For clarity, while making unlink more robust is no bad thing, the error occurs when the unlink *succeeds* but a subsequent create of the same name fails. This happens when an indexer, Virus scanner or TortoiseSvn etc. has opened the file with SHARE_DELETE. This allows a DeleteFile to succeed but continues to hold an open handle on the file. A following test which uses an identically named file (such as the global TESTFN) will fail if not enough time has elapsed for the background process to release its handle. A good candidate to see this in action is the test for tarfile.
I did start to undertake a conversion of TESTFN to a named temporary, but it started to sprawl all over the place and came up against a number of corner cases (eg where tests deliberately wanted two filenames to be the same) so I gave up.
pitrou commentedon Mar 27, 2011
How about renaming to a unique random name just before the unlink(), as
Mercurial does?
Would it alleviate the problem or am I missing something?
tjguk commentedon Mar 27, 2011
Well http://bugs.python.org/issue7443#msg102833 outlines the problems I encountered while trying to do essentially that. Nothing insurmountable, but definitely bigger than simply adding one line of code.
Looks to me like there are two avenues of approach (and, given the chatter on this issue, several people willing to address them):
Patch Py_DeleteFileW in posixmodule.c so that it renames before deleting: should solve the problem overall but obviously has a possible wider impact, in general and on performance in particular. This rename might be a simple rename-to-guid or something more sophisticated such as the rename-to-recycler which cygwin uses.
Patch support.unlink in the test package to do the rename dance on the basis that it'll fix at least some of the problems with less impact overall.
Opinions? I'm willing to do either.
pitrou commentedon Mar 30, 2011
Well, since I'm not a Windows expert, I can only give an intuitive
opinion. I think that we should start with patching support.unlink();
tweaking Py_DeleteFile() so as to do something more sophisticated than a
simple removal sounds like it could hide some behaviour change that
could hit some legitimate uses.
(as an aside, for higher-level variants of OS functions, the shutil may
be an appropriate recipient)
jdigital commentedon Apr 1, 2011
I vote for fixing the test package.
File system "extensions" may track and record this activity. To use DropBox as an example, doing the rename and delete will cause the renamed and deleted file to be recorded.
Just my opinion, but the code path to delete a file should be as short as possible. Making lots of other OS calls just doesn't seem right.
I understand the wish to have a reliable unlink call but I'd be uncomfortable with a workaround that may be visible around the edges.
ncoghlan commentedon Apr 2, 2011
I'll throw in my 2 cents as to a possible way forward:
asvetlov commentedon Apr 2, 2011
FYI: implementation of unlink already has multiple calls for unicode version to process symlinks. Ansi version is the simple DeleteFileA call.
Now we have different behavior for unicode and ansi variants as I can see.
Now I'm working on unlink implementations partially borrowed from cygwin as Martin suggested. I still not have a final patch covering all known cases. Perhaps I can introduce it next week.
ncoghlan commentedon Aug 2, 2012
See bpo-15496 for an alternative approach to solving this problem, at least in the test suite - as noted in that issue, the rename dance isn't sufficient when the problem gets triggered by a different sequence like:
tjguk commentedon Jul 31, 2013
This has been covered off by work done with the test.support package including context managers for temporary files / directories, and a waitfor mechanism which waits some time if a file can't be accessed.
bitdancer commentedon Jul 31, 2013
The support package is fixed (I presume :), but there was also a desire expressed to expose this functionality in shutil, since it can arise in "normal" Python programs as well as in the test suite. Given that the fix has been successful in the test suite, shouldn't we open an issue to expose it in shutil?