Skip to content
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

os: make Rename atomic on Windows #8914

Closed
gopherbot opened this issue Oct 9, 2014 · 10 comments
Closed

os: make Rename atomic on Windows #8914

gopherbot opened this issue Oct 9, 2014 · 10 comments
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 9, 2014

by MartyMacGyver:

As described in bug 3366 (https://golang.org/issue/3366) the
os.Rename() function doesn't work in Windows in a way that is consistent with other
OSes. In Posix-compliant OSes, the os.Rename() function works properly (and atomically),
including when replacing an existing file. In Windows this does not work correctly as
designed and fails when replacing an existing file. Since it has been decided that this
functionality for Windows will not be implemented in the existing os.Rename(), it is
desirable to have an equivalent os.Replace() function that works across platforms. In
the case of non-Windows platforms it would be a simple wrapper for os.Rename(), but in
the case of Windows it would call the proper OS-level APIs to perform the operation in
an equivalent fashion.

Other languages have encountered and successfully tackled this same problem in Windows.
As one example, the report of the same issue in Python led to a detailed discussion and
ultimately had a successful resolution (http://bugs.python.org/issue8828). The
workaround used in other Go projects follows the same pattern - effectively creating an
os.Replace() function for this circumstance, using the Windows MoveFileEx API for
Windows platforms and falling through to os.Rename() for the others.

Since os.Rename() cannot be enhanced to handle Windows cases in an equivalent fashion,
an os.Replace() function that satisfies the same criteria would prevent developers from
re-inventing the same wheel in very similar ways way simply to avoid the disjoint
functionality in os.Rename() when it comes to this relatively common use case.
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 9, 2014

Comment 1:

Should os.Replace handle the EXDEV case on Unix systems?  (On Unix os.Rename will fail
when trying to move a file to a different mounted file system).

Labels changed: added repo-main, release-go1.5.

Status changed to Accepted.

@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Oct 9, 2014

Comment 2 by MartyMacGyver:

I don't think that (at this point) os.Replace() should have extended functionality
beyond what os.Rename offers (e.g., os.Rename() isn't supposed to work across
filesystems). Atomicity with os.Replace() varies with filesystem - though it would be
implicit for Posix when using the fallthrough to os.Rename(), Windows requires more
finesse. The major functional change versus os.Rename() is handling Windows properly,
where one would use the MoveFileEx API *without* the MOVEFILE_COPY_ALLOWED flag to
maintain parity with Posix-style rename.
For the sake of atomicity I understand why this can't all be done in os.Rename(), but
os.Replace() is meant to loosen that guarantee for select OSes, not remove it. If kept
simple, (and documented as such), devs will know that on Posix hosts this will be
exactly the same as os.Rename, and on Windows hosts this will do its best but is
inherently non-Posix in nature (versus doing nothing at all in the case of such
non-Posix hosts).
** When one starts adding functionality such as the ability to move across filesystems
it quickly complicates things while breaking the guarantee of atomicity across the
board. If one wants cross-filesystem functionality (where atomic (or nearly-atomic)
behavior is not expected at all), one would use os.Copy() or os.Move() - which are
notable in their absence in Go. That's another wheel that people seem to reinvent a lot,
but it's outside of the scope of this issue except for the point that once you start
adding that level of functionality you are effectively creating those functions as well.
Better to keep that extended effort separate.
@anacrolix

This comment has been minimized.

Copy link
Contributor

@anacrolix anacrolix commented Oct 10, 2014

Comment 3:

What package and function name do you propose?
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Oct 10, 2014

Comment 4 by MartyMacGyver:

Package os, function signature the same as for Rename(), e.g.,
    func Replace(oldpath, newpath string) error
@bradfitz bradfitz modified the milestone: Go1.5 Dec 16, 2014
@bradfitz bradfitz removed the release-go1.5 label Dec 16, 2014
lvarvel pushed a commit to lvarvel/cacheddownloader that referenced this issue Feb 24, 2015
Fix file moving to be safe on Windows. A shim was added
that calls into the Win32 API / MoveFileEx that prevents an error
occurring on windows if the destination already exists. This shim might
not be needed once os.Replace lands, possibly in Go 1.5.

See: golang/go#8914

[#88907658]
mavenraven pushed a commit to lvarvel/cacheddownloader that referenced this issue Feb 25, 2015
Fix file moving to be safe on Windows. A shim was added
that calls into the Win32 API / MoveFileEx that prevents an error
occurring on windows if the destination already exists. This shim might
not be needed once os.Replace lands, possibly in Go 1.5.

See: golang/go#8914

[#88907658]
maqroll pushed a commit to maqroll/redux that referenced this issue Apr 4, 2015
@rsc rsc removed accepted labels Apr 14, 2015
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented May 6, 2015

I don't think there's consensus on a plan here (or any pending CL), and Go 1.5 was frozen as of last Friday. So I'm bumping this to milestone Go 1.6.

Note that commit 92c5736 did change os.Rename's behavior on Windows, though:

commit 92c57363e0b4d193c4324e2af6902fe56b7524a0
Author: Daniel Theophanes <kardianos@gmail.com>
Date:   Thu Feb 26 12:10:11 2015 -0800

    os: windows Rename should overwrite destination file.

    Rename now uses MoveFileEx which was previously not available to
    use because it is not supported on Windows 2000.
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 5, 2015

This bug was originally created asking for an os.Replace with consistent behavior on Windows, because #3366 (fix os.Rename) was closed as impossible.

I'm changing the topic of this bug to fix os.Rename, effectively replacing #3366. I don't believe package os should be inventing new functionality, but I do think that it should strive for consistent functionality in what's there. (There is for example another open bug for making os.RemoveAll work with read-only files on Windows.)

Based on http://stackoverflow.com/questions/167414/is-an-atomic-file-rename-with-overwrite-possible-on-windows it looks like it may be possible to just fix os.Rename on Windows. In particular, it sounds like ReplaceFile is the right call to make.

If we decide that it is truly impossible to implement os.Rename on Windows then we could fall back to a discussion of a new function, but I'd rather take all the existing code using os.Rename and make it start working than make it obsolete.

@rsc rsc changed the title os: Create an os.Replace() function that works consistently across all platforms os: make Rename atomic on Windows Nov 5, 2015
@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Nov 6, 2015

This is difficult to determine the correct winapi to call here. There are typically three winapi calls suggested:

MoveFileEx is what go currently has in go1.5 and tip. When this bug was written go used "MoveFile".

MoveFileTransacted has two strikes against it: 1) It is only supported on Vista+ and 2) MS says it may remove all the transacted APIs, including this one.

Is ReplaceFile atomic? ReplaceFile has error code ERROR_UNABLE_TO_MOVE_REPLACEMENT which states:

The replacement file could not be renamed. If lpBackupFileName was specified, the replaced and replacement files retain their original file names. Otherwise, the replaced file no longer exists and the replacement file exists under its original name.

It also only works on files. However a research paper from MS claims that ReplaceFile is atomic: http://research.microsoft.com/pubs/64525/tr-2006-45.pdf . I suspect that this is a mistake.

MoveFileEx has many flags. Of interest here are two in particular:
MOVEFILE_REPLACE_EXISTING and MOVEFILE_COPY_ALLOWED. Currently in go we only use MOVEFILE_REPLACE_EXISTING and thus Rename will move if attempted between different volumes. However only the MOVEFILE_COPY_ALLOWED flag suggests multiple operations are used.

I think what we currently do is the correct thing to do on Windows.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Nov 9, 2015

I'm changing the topic of this bug to fix os.Rename, effectively replacing #3366.

@kardianos didn't you fix issue #3366 with CL 6140? If I am not mistaken, you have even added new TestRenameOverwriteDest that tests the problem raised in issue #3366.

@rsc what exactly do you want us to do?

Alex

@kardianos

This comment has been minimized.

Copy link
Contributor

@kardianos kardianos commented Nov 9, 2015

@alexbrainman That was my impression as well. I just wanted to be through in my analysis.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 24, 2015

The original report says:

In Windows this does not work correctly as designed and fails when replacing an existing file.

I interpreted the original report as saying that

os.Create("x")
os.Create("y")
os.Rename("x", "y")

fails on Windows because y already exists.

If that is true, then I think we should fix that. If it's not true, then I think there is nothing to do here. I admit that I took the report at face value and did not investigate whether that is true.

I agree @alexbrainman that TestRenameOverwriteDest seems to suggest this example already works. Given that, I think we can close the bug. Sorry for the confusion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.