[MERGE] Support FTP servers with non-atomic rename (bug #89436)

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John Arbash Meinel wrote:
> Aaron Bentley wrote:
>> Hi all,
> 
> === modified file bzrlib/osutils.py
> --- bzrlib/osutils.py
> +++ bzrlib/osutils.py
> @@ -224,7 +224,14 @@
>              # If the file used to exist, rename it back into place
>              # otherwise just delete it from the tmp location
>              if success:
> -                unlink_func(tmp_name)
> +                try:
> +                    unlink_func(tmp_name)
> +                except:
> +                    # If the unlink fails, then restore the old state,
> as if
> +                    # an atomic rename failed
> +                    rename_func(new, old)
> +                    rename_func(tmp_name, new)
> +                    raise
>              else:
>                  rename_func(tmp_name, new)
> 
> 
> I'm not sure... By this time we have already successfully moved the new
> file into place, so the "atomic" portion of it has happened. We haven't
> cleaned up a temp file, but that has been moved out of the way, so it
> won't conflict. So it seems artificial to fail the rest.

We depend on the fact that renaming on top of a directory that has
contents will fail, and will fail atomically.

This is tested by test_rename_dir_nonempty in
test_transport_implementations.py

Without this change, test_rename_dir_nonempty will fail.  Also, I
believe lockdirs will be broken on FTP-- you will be able to acquire
locks that you should not be able to acquire.

> 1) create new temp file (a.new.tmp)
> 2) rename existing file to temp file (a => a.old.tmp)
> 3) rename temp file to target (a.new.tmp => a)
> 4) delete old file (del a.old.tmp)

This is not an accurate summary of what fancy_rename does.  There is
never any creation of a temporary file.

It is more like:
1) rename oldpath => newpath
2) if 1 did not fail, exit
3) rename newpath => temppath
4) rename oldpath => newpath
5) delete temppath

> If step (4) fails:
>   I would say "let the user know, but everything else completed".

My understanding of fancy_rename is that it is meant to simulate atomic
rename behavior on non-atomic platforms.  Atomic behavior is:

>>> import os
>>> os.mkdir('foo')
>>> open('foo/bar', 'wb').write('contents')
>>> os.mkdir('bar')
>>> try:
...     os.rename('bar', 'foo')
... except Exception, e:
...     print e
...
[Errno 39] Directory not empty
>>> os.path.exists('bar')
True

So restoring the previous state if step 5 fails matches atomic behavior.

>   At the very least I wouldn't:
>     a => a.new.tmp
>     a.old.tmp => a
>   without "del a.new.tmp"

The del a.new.tmp is deferred until all other operations have been
performed.  Therefore, it can fail last.  But if it fails, an atomic
rename would also have failed.

> In the original way, you may end up with an "a.old.tmp" file lying
> around. In the new way, you end up with an "a.new.tmp" file lying around.

Huh?  If the delete fails, then we restore the initial state, and there
are no temp files lying around.  (From the perspective of fancy_rename,
that is.  old_path may be a temp file in some other context, of course)

> I would say that "you've succeeded" because the new file has taken the
> place of the old file. Not "you've failed" because a cleanup step failed.

If we want to closely emulate atomic rename, we must fail in this case.

> So I'm +1 on the update to FTP. I'm at least -0 on the change to
> fancy_rename. Because I think you are failing when you don't have to.

I changed fancy_rename so that the FTP tests would pass.  I cannot make
the FTP change without either fixing fancy_rename or disabling
test_rename_dir_nonempty, and the latter seems unwise.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGI7ET0F+nu1YWqI0RAum9AJ4siM1l7Q10wZ5nrfg2EHVNMk7rgACeICb5
0hbXUkFNIoreiP63JPfdUBU=
=Oj9m
-----END PGP SIGNATURE-----