atomic file renames on windows

Bug #557585 reported by anatoly techtonik on 2010-04-07
12
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Dulwich
Fix Released
High
Unassigned

Bug Description

http://bazaar.launchpad.net/~vcs-imports/dulwich/trunk/revision/440.1.2

This commit puts hg-git 0.2.1 extension for Mercurial 1.5.1 into non-working state on windows.
HG repository is not populated with this commit although git checkout seems to be made.

> hg clone -v git://github.com/msoulier/tftpy.git
*** failed to import extension progress: No module named progress
destination directory: tftpy.git
importing Hg objects into Git
Counting objects: 599, done.
Compressing objects: 100% (365/365), done.
Total 599 (delta 388), reused 332 (delta 209)
importing Git objects into Hg
updating to branch default
resolving manifests
0 files updated, 0 files merged, 0 files removed, 0 files unresolved

anatoly techtonik (techtonik) wrote :
description: updated
anatoly techtonik (techtonik) wrote :

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.

Jelmer Vernooij (jelmer) on 2010-04-07
Changed in dulwich:
status: New → Triaged
importance: Undecided → High
Jelmer Vernooij (jelmer) wrote :

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?

anatoly techtonik (techtonik) wrote :

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.

anatoly techtonik (techtonik) wrote :
anatoly techtonik (techtonik) wrote :

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.

anatoly techtonik (techtonik) wrote :

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.

anatoly techtonik (techtonik) wrote :

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

anatoly techtonik (techtonik) wrote :

Dave, you should be interested to look at this.

anatoly techtonik (techtonik) wrote :

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

Dave Borowitz (dborowitz) wrote :

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.

anatoly techtonik (techtonik) wrote :

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.

anatoly techtonik (techtonik) wrote :

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

anatoly techtonik (techtonik) wrote :

Moved rename code into file.safe_rename() function.

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

anatoly techtonik (techtonik) wrote :

refreshed patch removing mysterious self. prefix from tmpfile

anatoly techtonik (techtonik) wrote :

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

anatoly techtonik (techtonik) wrote :

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.

Changed in dulwich:
status: Triaged → Fix Committed
Matthew L Daniel (mdaniel) wrote :

@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

Jelmer Vernooij (jelmer) on 2010-05-19
Changed in dulwich:
milestone: none → 0.6.0
milestone: 0.6.0 → 0.7.0
Jelmer Vernooij (jelmer) on 2010-05-20
Changed in dulwich:
milestone: 0.7.0 → 0.6.0
anatoly techtonik (techtonik) wrote :

@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))

Jelmer Vernooij (jelmer) on 2010-05-23
Changed in dulwich:
status: Fix Committed → Fix Released
nothize (nothize) wrote :

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?

nothize (nothize) wrote :

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?

Jelmer Vernooij (jelmer) wrote :

nothize, please open a separate bug for those issues.

summary: - GitFile breaks dulwich on Windows
+ atomic file renames on windows
Matthew L Daniel (mdaniel) wrote :

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

nothize (nothize) wrote :

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

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.