atomic file renames on windows

The patch fixes the major half of failed test suite on windows. Windows XP and earlier don't have atomic renames and throw warning that file exists for an attempt.

Thanks for the patch. I'm a bit worried about simply falling back to non-atomic renames. I wonder if there's an alternative we can use - mandatory locking perhaps?

Does it worth the complication? As long as only one process has access - everything should be ok. For multiprocess access it may be better to lock the whole repository instead of separate files, check the lock for each write operation, keep the list of files opened for writing, so that lock can only be removed after all files are closed.

See http://trac.edgewall.org/browser/trunk/trac/util/__init__.py?rev=9436#L85 for a more thorough solution and also http://stackoverflow.com/questions/167414/is-an-atomic-file-rename-with-overwrite-possible-on-windows

I attach test logs for version with applied patch and for 0.4.1 which also fails on Windows. However, 0.4.1 doesn't leave hg-git repository empty.

Atomic file renames discussion on Twisted http://twistedmatrix.com/trac/ticket/3454

It also should be noted that atomic renames on filesystems that don't support transactions (FAT, network samba share) may incur some overhead that can appear significant with increased amount of operations.

Fixed by adding os.O_BINARY flag for low-level os.open on Windows. Without it the .idx file is created corrupted with CRLF linefeeds.

I am curious - GitFile header contains Google copyright for GPL code, but I couldn't find where this GPL code was taken from.

Seems like Google folks may be interested in merging this back.

Dave, you should be interested to look at this.

Added a proposal to clarify docs in Python http://bugs.python.org/issue8399

Anatoly, thanks for fixing this bug on Windows. Please feel free to send your fixes to Jelmer. Unfortunately, I don't have the resources, and I don't think Jelmer does either, to test every change thoroughly on Windows. I'll try to do better in the future, but without something much more automated and comprehensive I don't think we'll be able to catch all of these things ahead of time.

The specific locking protocol I implemented in GitFile (exclusive lockfiles named foo.lock, atomic renames) is the same as the one used by cgit. Among other things, this means that repositories following this protocol can be accessed concurrently by dulwich and cgit, which IMHO is a Good Thing. I would argue against full-repository locking because that's very incompatible with cgit. (That said, cgit has notoriously poor support for both Windows and networked filesystems, so if we find something that performs better on those systems, we should suggest it to the cgit folks as well.)

Now, about the Google copyright. All of the code I've contributed to dulwich has been in the course of my day job at Google. According to corporate policy (and I'm pretty sure US copyright law, but don't quote me on that), that mean Google owns the copyright. So, instead of writing "Copyright 2010 David Borowitz," I have to say "Copyright 2010 Google, Inc." The code was still written by me either way.

To make sure Google "gets" any patches you submit, just make sure they're submitted to the master repo on samba.org; I'm always working as close to HEAD as possible.

That's interesting. At the moment I still unable to push changes from Windows, because of more Windows-Dulwich-HgGit-Mercurial bugs (I use Mercurial for bisecting Dulwich), but I hope Jelmer finds the time to apply attached patch manually. I attach rebased patch against first mention of 0.5.1 in repository. This probably be better rebased to the first revision, where the fix is desired, so that if people grab history for bisecting - they can make revisions in between GitFile inclusion and the patch also work on Windows by merging branches (or hg heads).

It lacks some test cases though, for example, that pack.write_pack_index_v2() writes binary file with linefeed data correctly. And locking logic on Windows doesn't allow atomic renames, but the issue is complicated, so it should be best split it into some "No atomic file renames on Windows" issue with description of consequences until the solution is found.

Please review new fancy rename patch - save file before rename and rollback if rename fails.

Moved rename code into file.safe_rename() function.

Attached here and uploaded to http://codereview.appspot.com/802052 for convenience.

refreshed patch removing mysterious self. prefix from tmpfile

Fancy rename v2, more useful when destination doesn't exists, added tests.

Both patches are integrated. This issue can be closed now, even though some HgGit operations (using Dulwich) on Windows still fail, because not all files are properly closed. Windows doesn't allow to overwrite files even opened for reading by default, but this is a subject of other ticket.

@anatoly what is the ticket number for the "not all files are properly closed" part?

I am experiencing the following error while using hggit and would like to track its progress:

  File "\Python26\Lib\site-packages\dulwich\file.py", line 54, in fancy_rename
    os.rename(newname, tmpfile)
WindowsError: [Error 32] The process cannot access the file because it is being used by another process
abort: The process cannot access the file because it is being used by another process

@Matthew, I haven't created this ticket yet. "not all files are properly closed" was aimed at bugs.python.org I used the following monkeypatching code to debug the issue, but it doesn't cover all the cases a file descriptor can be opened.

import __builtin__
import inspect
openfiles = set()
oldfile = __builtin__.file
class newfile(oldfile):
    def __init__(self, *args):
        self.x = args[0]
        print "### OPENING %s ###" % str(self.x)
        oldfile.__init__(self, *args)
        openfiles.add(self)

    def close(self):
        print "### CLOSING %s ###" % str(self.x)
        oldfile.close(self)
        openfiles.remove(self)
oldopen = __builtin__.open
def newopen(*args):
    return newfile(*args)
__builtin__.file = newfile
__builtin__.open = newopen

def printOpenFiles():
    print "### %d OPEN FILES: [%s]" % (len(openfiles), ", ".join(f.x for f in openfiles))

I've just downloaded Dulwich 0.6.0 and try out with the hg-git extension.

Combing Python debug mode with anatoly techtonik's "monkeypatching" code and file handle dumper, here is the result:

Output of printOpenFiles():

### 3 OPEN FILES: [R:\hg\inspection\.hg\hgrc, R:\hg\inspection\.hg\git\objects\pack\pack-1a59751d438961fde15357f30146e30cc612fe34.pack, R:\hg\inspection\.hg\git\objects\pack\tmpqxvqyi.pack]

Partial output of handle -p hg.exe:

   dc: File R:\hg\inspection
  1e0: File R:\hg\inspection\.hg\git\objects\pack\pack-1a59751d438961fde15357f30146e30cc612fe34.idx
  1e8: File R:\hg\inspection\.hg\git\objects\pack\pack-fcdb717aa8f634411314e2b8288f17333da2ed44.idx
  1ec: File R:\hg\inspection\.hg\git\objects\pack\pack-1a59751d438961fde15357f30146e30cc612fe34.pack
  1f0: File R:\hg\inspection\.hg\git\objects\pack\tmprkzdib.pack
  384: File R:\hg\inspection\.hg\hgrc

It shows that the files ending in .idx are so special that doesn't appear in the Python opened file list.

By debugging deep, it is found that the use of mmap.mmap(...) in pack.py(167)_load_file_contents would keep a file handle and there is no place to close the mmap. Perhaps this is where the file handle leak happen?

Attached the patch to demonstrate how the mmap.mmap() is affecting file moving on Windows. Another patch is just a quick and dirty use of the fancy_rename function to let the command "hg pull" pass.

Any clue for closing the mmap at the right time?

nothize, please open a separate bug for those issues.

And can you put a comment here with the follow-up bug-id, please?

Created a new issue, "File opened by mmap is not closed causes removing file failure", id is 645881.