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

proposal: os: add a package to atomically create or replace a file #22397

Open
stapelberg opened this issue Oct 23, 2017 · 34 comments
Open

proposal: os: add a package to atomically create or replace a file #22397

stapelberg opened this issue Oct 23, 2017 · 34 comments

Comments

@stapelberg
Copy link
Contributor

I propose adding a package to atomically create or replace a file to the golang.org/x/ namespace.

Moving the required logic into a Go package slightly reduces code length and duplication across programs, but more importantly documents the intricacies of this pattern in a central place.

An implementation of such a package can be found at https://go-review.googlesource.com/c/exp/+/72379

@gopherbot gopherbot added this to the Proposal milestone Oct 23, 2017
@rasky
Copy link
Member

rasky commented Oct 23, 2017

There are a few implementations in the wild, I think they should be analyzed and mentioned in the proposal.

@ianlancetaylor
Copy link
Contributor

Yes, this seems like something that should start as an external package. https://golang.org/doc/faq#x_in_std

@ianlancetaylor
Copy link
Contributor

But perhaps we could add something to golang.org/x/tools or golang.org/x/sync.

@stapelberg
Copy link
Contributor Author

stapelberg commented Mar 13, 2018

I analyzed a number of implementations I could find using GitHub’s search feature:

os.Rename + chmod:

os.Rename + chmod + chown:

These implementations unconditionally sync before renaming:

This implementation syncs the containing directory to persist the rename:

These implementations have Windows-specific code:

Action items:

  • Verify whether Chown needs a sync call, too (like Chmod).
  • Find out which of the two windows syscalls is preferable for replacing os.Rename on Windows.

@rasky Does that satisfy your concern? Did you have any particular implementations in mind that you wanted to include in the analysis?

@rasky
Copy link
Member

rasky commented Mar 13, 2018

@stapelberg thanks that's a good start. What I miss from the above comment is why those libraries are not "good enough". You seem to know the problem space well, but are not explaining it. When you say for instance "os.Rename + chmod + chown". I'm not sure if that's correct, or a bug, or just a different way of doing it.

I suggest a structure that you can use to edit your proposal above, to provide more information. To clarify, I'm not able to approve your proposal, I'm just trying to help you providing enough information for a maintainer to make a call on this. This is just a suggestion.

  • Problem statement
    Atomically create or update a file is important for [...]. (I think everybody knows that, so just a short concise paragraph).
    A basic implementation is often buggy because [common bugs or side cases not handled] [complex issues about filesystems working differently from system to system]
    Thus, I propose to add a standard implementation somewhere in "x/".

  • Existing options
    There are N main libraries with at least X stars on GitHub. They have the following problems:

    • A does this but has this bug
    • B does that but has this other bug
    • ecc.
  • Proposal
    I propose to write a library that does this and that, and fixes all the above problems. A prototype is here (link).

@bradfitz
Copy link
Contributor

bradfitz commented Apr 2, 2018

Sounds like we're still waiting on @stapelberg here.

@stapelberg
Copy link
Contributor Author

Yes. I’ll follow up with the requested write-up as soon as I find a moment of time for it.

@stapelberg
Copy link
Contributor Author

stapelberg commented Apr 12, 2018

Problem statement

Atomically creating or replacing files is a technique used to prevent intermediate (regular operation) or corrupted (when crashing) files from appearing in e.g. a webserver directory.

Side note: while related, the concept of durability is not covered by this proposal at all. In other words, if you would like to ensure your data made it to your disk(s), you’ll need to call Sync() yourself. The proposal only concerns itself with ensuring that what has been written was written atomically (i.e. either the old data is present, or the new data is present, but never intermediate/corrupt data).

A naive approach to the problem is to create a temporary file followed by a call to os.Rename. However, there are a number of subtleties which make the correct sequence of operations hard to identify:

  • The temporary file should be removed when an error occurs, but a remove must not be attempted if the rename succeeded, as a new file might have been created with the same name. This renders a throwaway defer os.Remove(t.Name()) insufficient; state must be kept.

  • The temporary file must be created on the same file system (same mount point) for the rename to work, but the TMPDIR environment variable should still be respected, e.g. to direct temporary files into a separate directory outside of the webserver’s document root but on the same file system.

  • On POSIX operating systems, the fsync system call must be used to ensure that the os.Rename() call will not result in a 0-length file.

Existing options

There are 4 notable libraries on GitHub:

  • https://github.com/facebookgo/atomicfile (51 stars) has a decent design, but:
    • Does not default to Sync() after Chmod(), i.e. requires an ordered file system without explicitly stating it.
    • Forcibly does a Chmod() even when not required.
    • Does not respect the TMPDIR environment variable.
    • Does not work on Windows.
  • https://github.com/natefinch/atomic (72 stars)
    • Uses the signature WriteFile(string, io.Reader), which makes some use-cases cumbersome or impossible to implement, e.g. streaming data into different parts of the temporary file, or calling Sync() on the file for durability.
    • Forcibly does a chmod, copying the permissions from the file which is being replaced, if any.
    • Does not respect the TMPDIR environment variable.
  • https://github.com/dchest/safefile (76 stars)
    • Looks very decent. Only found this in early 2019, well after I published renameio.
    • Does not respect the TMPDIR environment variable.
  • https://github.com/google/renameio (273 stars)
    • Disclaimer: I authored this.
    • Seems to get everything right so far.

There are a number of other ad-hoc implementations in applications (as opposed to libraries): #22397 (comment)

Some of these conflate durability with atomicity, get the order of operations wrong, or exhibit issues similar to the ones listed in the 2 libraries above.

Proposal

I propose to add a package to the x/ namespace which offers atomic file creation/replacement, using explicit names in its method signatures (accompanied by detailed doc comments), addressing all the shortcomings listed above.

Find the prototype at https://go-review.googlesource.com/c/exp/+/72379

@rsc
Copy link
Contributor

rsc commented Apr 16, 2018

/cc @aclements @bradfitz @bcmills

@rsc
Copy link
Contributor

rsc commented Apr 16, 2018

See also #17869.

@bcmills
Copy link
Contributor

bcmills commented Apr 16, 2018

The temporary file must be created on the same file system (same mount point) for the rename to work, but the TMPDIR environment variable should still be respected

What should happen if TMPDIR is not on the destination file system?

@stapelberg
Copy link
Contributor Author

What should happen if TMPDIR is not on the destination file system?

An error will occur, and the user must set TMPDIR to point to the destination file system.

I’m not sure whether erroring at rename time (more efficient when things go right) or adding explicit checking (more user-friendly when things go wrong) is better. In case the writes are small, there isn’t much difference, so I’m thinking about cases where writes take a long time.

Maybe we could use sync.Once under the assumption that TMPDIR doesn’t change at runtime to remove performance considerations from an explicit check.

@pam4
Copy link

pam4 commented Apr 16, 2018

I think that TMPDIR is more of a user/admin preference, and it is not uncommon to have it point to its own filesystem. Maybe we have different use cases in mind, but requiring the program user to change TMPDIR seems odd to me.

As the caller I can think of two cases:

  • I want it to just work (use same directory as the destination or TMPDIR with automatic fallback to same directory)
  • I know better (pass explicit directory)

In the second case I would prefer to be able to pass it, rather then change TMPDIR from inside the Go program, so that I don't have to worry about the implication of changing the environment (concurrent access, child processes).

A minor note about your prototype: in the commented out example you use defer and log.Fatal, but deferred functions will not run on log.Fatal, you probably intended return err.

@bcmills
Copy link
Contributor

bcmills commented Apr 16, 2018

You've enumerated some good problems, so it would be nice if we can find a way to address them, but I think the API as proposed still needs work. (I left some comments on the CL, but I wanted to give a higher-level overview here.)

The current draft seems both too subtle and too simple: too subtle because it still requires the caller to know whether they need to call Sync before renaming (and whether to use your Chmod or os.Chmod directly), but too simple because it makes some pretty strong assumptions about how to choose the temporary directory.

Is there some way we can address those issues within the API itself? (For example, some standard way to detect whether a given filesystem is ordered and/or uses write barriers?)

@pam4
Copy link

pam4 commented Apr 17, 2018

A few more considerations:

  • When the temp and target files are on the same directory, it may help to prepend a dot to the temp name: some file servers are configured to ignore dot-files and some glob pattern mechanisms will not match dot-files unless the pattern starts with a literal dot.
  • We should use os.TempDir instead of accessing TMPDIR directly.
  • I don't know of a portable way to check for "same filesystem"; under unix you would compare syscall.Stat_t.Dev.
  • I agree that subtleties like whether Syncs are required or not, should be hidden from the caller (which often is not in the position to make assumptions about the filesystem anyway).
    I would start by just forcing a Sync before Rename, and think about optimization if necessary.
    Is the performance impact really significant?
    If it is, maybe we could recognize common filesystem configurations and avoid Sync in those cases, if a more accurate determination is too difficult.
  • If the Chmod serves a purpose other than just changing mode (fchmod hack), then the api shuold call it for you if you don't, and take care of everything.
    Does it make a difference if Chmod is called before/between/after the writes, or if the new mode is the same as the old?

@stapelberg
Copy link
Contributor Author

Thanks for the feedback, everyone.

I have uploaded a new revision of my change which, I believe, addresses all the concerns brought up so far.

A key insight came from discussion with Theodore Ts'o (ext2/3/4 lead developer), who stated:

data=ordered only guarantees the avoidance of stale data (e.g., the previous
contents of a data block showing up after a crash, where the previous data
could be someone's love letters, medical records, etc.). Without the fsync(2)
a zero-length file is a valid and possible outcome after the rename.

The same holds for file systems with write barriers.

The conclusion from our discussion is that the only way to achieve atomicity in a cross-filesystem, cross-platform way, is to rely on the fsync.

If one wants to optimize the fsync away, it seems like one would need to rely on the specific version and mount options of a specific file system, and even then, you wouldn’t get guaranteed atomicity, just a reduced likelihood of crashes violating atomicity.

With all my assumptions being proven wrong, this simplifies the package quite a bit. The Chmod method, and the (incorrect) caveats are gone. I changed the signature of TempFile such that callers can specify a temporary directory.

One remaining assumption we’re making is that people use journaling file systems. If we don’t want to make that assumption, we’d need to also fsync the directory containing the file we wrote, to cover the case where the old inode and the new inode are in separate blocks and hence might be flushed out at different times.

@rsc
Copy link
Contributor

rsc commented Jul 9, 2018

It seems like this can easily belong outside the standard library, or outside golang.org/x, at least until we know how to implement it. I'm concerned that the implementation is still changing, and what about systems without fsync?

@stapelberg
Copy link
Contributor Author

It seems like this can easily belong outside the standard library,

Definitely.

or outside golang.org/x, at least until we know how to implement it.

I thought x/exp (where I’m suggesting to add the package for the time being) was a staging ground for packages in x. Am I mistaken?

I'm concerned that the implementation is still changing, and what about systems without fsync?

Which systems lack fsync? It seems like the primary and only portable guarantee for both atomicity and durability.

@ianlancetaylor
Copy link
Contributor

The Go team nominally maintains the packages in x/exp. We're suggesting putting it in github.com/stapelberg (or wherever) for now.

To the best of my knowledge neither Windows nor Plan 9 support fsync. But maybe you are using different mechanisms on those systems.

@stapelberg
Copy link
Contributor Author

The Go team nominally maintains the packages in x/exp. We're suggesting putting it in github.com/stapelberg (or wherever) for now.

My hope was that we could (eventually) reduce some duplication across the ecosystem by putting this package in a somewhat prominent place.

Is there any path forward where the package could eventually end up in x?

To the best of my knowledge neither Windows nor Plan 9 support fsync. But maybe you are using different mechanisms on those systems.

Plan 9 does seem to support fsync, or are you saying that the current os.File.Sync implementation in file_plan9.go is not sufficient?

go/src/os/file_plan9.go

Lines 222 to 241 in 6fe7b43

// Sync commits the current contents of the file to stable storage.
// Typically, this means flushing the file system's in-memory copy
// of recently written data to disk.
func (f *File) Sync() error {
if f == nil {
return ErrInvalid
}
var d syscall.Dir
d.Null()
var buf [syscall.STATFIXLEN]byte
n, err := d.Marshal(buf[:])
if err != nil {
return NewSyscallError("fsync", err)
}
if err = syscall.Fwstat(f.fd, buf[:n]); err != nil {
return NewSyscallError("fsync", err)
}
return nil
}

On Windows, it seems like we should use syscall.FlushFileBuffers(syscall.Handle(f.Fd())) (see e.g. https://www.humboldt.co.uk/fsync-across-platforms/).

@ianlancetaylor
Copy link
Contributor

As @rsc said, it could wind up in x/ when it is implemented and stable.

There is nothing magic about x/. godoc.org lists packages from all over the place. Of the 25 top popular packages that it lists, 3 are from x/.

@stapelberg
Copy link
Contributor Author

Okay. Should we leave this issue open, or should I open a new one once the package has stabilized?

@ianlancetaylor
Copy link
Contributor

I'll put the proposal on hold for now.

@stapelberg
Copy link
Contributor Author

FYI: the package is now published at https://github.com/google/go-write

@gopherbot
Copy link

Change https://golang.org/cl/146377 mentions this issue: cmd/go/internal/renameio: add package

gopherbot pushed a commit that referenced this issue Nov 29, 2018
renameio.WriteFile writes files atomically by renaming temporary files.

See the subsequent changes for usage examples.

Updates #26794
Updates #22397

Change-Id: I4bfe3125a53f58060587f98afbb4260bb1cc3d32
Reviewed-on: https://go-review.googlesource.com/c/146377
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Giovanni Bajo <rasky@develer.com>
Reviewed-by: Russ Cox <rsc@golang.org>
@bcmills
Copy link
Contributor

bcmills commented Feb 22, 2019

See previously #8868 and #17869.

@bcmills
Copy link
Contributor

bcmills commented May 31, 2019

It turns out that os.Rename is not currently atomic on Windows (see #32188), so an atomic-replace package would be useful there too.

@bcmills
Copy link
Contributor

bcmills commented Jun 4, 2019

The outcome of my investigation for #32188 was that it appears not to be possible to write a Windows atomic-rename library that avoids spurious errors in all cases: even using the recommended ReplaceFile system call, readers can still observe any of at least three errors under high write loads.

(That doesn't preclude such a library for POSIX systems, though.)

@tv42
Copy link

tv42 commented Jun 28, 2019

Reading https://github.com/google/renameio, the idea of doing a Rename to see whether we can later do a rename from /tmp to whereever seems weird, wasteful, and backwards. Just create the temp file in the same directory as the target; it's what everyone does, and the only thing guaranteed to work.

@stapelberg
Copy link
Contributor Author

This seems off-topic on this issue, so if you have any follow-up, please open an issue on https://github.com/google/renameio itself.

For https://manpages.debian.org/, we need to create the files outside of the www directory (to prevent rsync from picking them up), which we achieve by setting TMPDIR. Respecting TMPDIR seems like a good thing to me.

If this bothers you, you can override this behavior by just passing filepath.Dir(path) as dir parameter to renameio.TempFile.

@tv42
Copy link

tv42 commented Jun 29, 2019

On the contrary, it drives home to point that there's a ton of decisions in such a thing, that aren't carved in stone yet, and thus it's not necessarily stdlib time.

You could easily give rsync an exclude filter that makes it ignore temp files.

@stapelberg
Copy link
Contributor Author

Okay. Should we leave this issue open, or should I open a new one once the package has stabilized?

I'll put the proposal on hold for now.

It’s been almost two years since we put the proposal on hold, so I think it might be time to dust it off and take a fresh look :)

In the meantime:

  1. https://github.com/google/renameio has amassed 416 stars on GitHub, and is imported by 40 (known) packages. I interpret this as renameio being popular, and solving an issue that’s worth solving.

  2. https://github.com/google/renameio has been largely untouched since 2018, aside from a small bugfix in the “replace a symlink atomically” feature. I’d call it stable.

  3. @bcmills has added a version of renameio to src/cmd/go, so even the Go tool itself profits from this functionality. Historically, “we needed it for our toolchain” determined what went into the standard library, IIUC. I’m not saying we need to apply the same principle right now, but it certainly seems like another point in favor of the proposal.

With all of these points in favor, can we please move this proposal into the review process again and decide whether we want to move forward with having (a variant of?) renameio in the standard library?

Thank you,

@networkimprov
Copy link

networkimprov commented Jun 1, 2020

FWIW, I rolled an in-app implementation (for Windows, MacOS, Linux) of atomic create-file, replace-file, and append-to-file. I include a recovery step which runs on startup to complete viable unfinished transactions, and clean up temp files for non-viable ones.

I didn't even consider trying third party libraries, but I would have tried stdlib APIs. I might not have kept using them tho. Doing this correctly requires power-off-crash testing on all platforms (which I plan to do for my app) because ppl believe "atomic" means crash-proof.

For the create-file case, I first write targetless symlinks as placeholders, making it a variation of replace-file. @bcmills has been inadvertently trying to put a stop to this in #39183 (kidding :-)

The append-to-file case rewrites indexes at the end of the file, so it's not purely an append. I didn't devise an API for this case, because nothing leapt out as clearly correct, and I didn't have time to make a study of it.

Re os.Rename(), I make the assumption that it won't completely delete the file in a crash scenario; that you'll end up with the original name, the new one, or both.

@adonovan
Copy link
Member

adonovan commented Jun 14, 2023

See https://go.dev/cl/495800 for an example of a program that went through the rename(2), flock(2), "short writes are atomic" evolutionary process.

See also https://cs.opensource.google/go/x/tools/+/master:gopls/internal/filecache/filecache.go;l=242;drc=f872b3d6f05822d290bc7bdd29db090fd9d89f5c for a description of the design we settled on for a simple file-based atomic key/value store.

DennisRasey pushed a commit to DennisRasey/forgejo that referenced this issue Dec 25, 2023
- After stumbling upon golang/go#22397 and
reading the implementations I realized that Forgejo code doesn't have
`Sync()` and it doesn't properly error handle the `Close` function.
- (likely) Resolves https://codeberg.org/forgejo/forgejo/issues/1446

(cherry picked from commit 0efcb334c2f123d0869a30d684189eb31e8b983f)
(cherry picked from commit 04ef02c0dd98c7437acb39383d311c0901366508)
(cherry picked from commit 85f2065c9bc6ded9c21909ec76a9e8fc2d22f462)
(cherry picked from commit 8d36b5cce66864e190bad3c9b0973e37ca774a22)
(cherry picked from commit 378dc30fb5a88ffe185c54de7e69224289038bff)
(cherry picked from commit 2b28bf826e51b8ccb4a693001c03ffe6132f7842)
(cherry picked from commit d0625a001e5f8fe202865bec7aadcf0c551d556d)
(cherry picked from commit f161a4f60f1cde80a41bece4929836257b9e0423)
(cherry picked from commit 7430ca43e57683ca324fb20269a60e05cb393589)
(cherry picked from commit ab6d38daf7eeb1dc993bfc0ac1a326af65128168)
(cherry picked from commit 0f703fd02e69bdcf2f77e120ff8641f1b8089020)
(cherry picked from commit 6931a8f)
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this issue Jan 22, 2024
- After stumbling upon golang/go#22397 and
reading the implementations I realized that Forgejo code doesn't have
`Sync()` and it doesn't properly error handle the `Close` function.
- (likely) Resolves https://codeberg.org/forgejo/forgejo/issues/1446

(cherry picked from commit 0efcb334c2f123d0869a30d684189eb31e8b983f)
(cherry picked from commit 04ef02c0dd98c7437acb39383d311c0901366508)
(cherry picked from commit 85f2065c9bc6ded9c21909ec76a9e8fc2d22f462)
(cherry picked from commit 8d36b5cce66864e190bad3c9b0973e37ca774a22)
(cherry picked from commit 378dc30fb5a88ffe185c54de7e69224289038bff)
(cherry picked from commit 2b28bf826e51b8ccb4a693001c03ffe6132f7842)
(cherry picked from commit d0625a001e5f8fe202865bec7aadcf0c551d556d)
(cherry picked from commit f161a4f60f1cde80a41bece4929836257b9e0423)
(cherry picked from commit 7430ca43e57683ca324fb20269a60e05cb393589)
(cherry picked from commit ab6d38daf7eeb1dc993bfc0ac1a326af65128168)
(cherry picked from commit 0f703fd02e69bdcf2f77e120ff8641f1b8089020)
(cherry picked from commit 6931a8f)
(cherry picked from commit 5e2065c)
(cherry picked from commit 38c812a)
(cherry picked from commit 494874e)
(cherry picked from commit d396b7f)
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this issue Jan 28, 2024
- After stumbling upon golang/go#22397 and
reading the implementations I realized that Forgejo code doesn't have
`Sync()` and it doesn't properly error handle the `Close` function.
- (likely) Resolves https://codeberg.org/forgejo/forgejo/issues/1446

(cherry picked from commit 0efcb334c2f123d0869a30d684189eb31e8b983f)
(cherry picked from commit 04ef02c0dd98c7437acb39383d311c0901366508)
(cherry picked from commit 85f2065c9bc6ded9c21909ec76a9e8fc2d22f462)
(cherry picked from commit 8d36b5cce66864e190bad3c9b0973e37ca774a22)
(cherry picked from commit 378dc30fb5a88ffe185c54de7e69224289038bff)
(cherry picked from commit 2b28bf826e51b8ccb4a693001c03ffe6132f7842)
(cherry picked from commit d0625a001e5f8fe202865bec7aadcf0c551d556d)
(cherry picked from commit f161a4f60f1cde80a41bece4929836257b9e0423)
(cherry picked from commit 7430ca43e57683ca324fb20269a60e05cb393589)
(cherry picked from commit ab6d38daf7eeb1dc993bfc0ac1a326af65128168)
(cherry picked from commit 0f703fd02e69bdcf2f77e120ff8641f1b8089020)
(cherry picked from commit 6931a8f)
(cherry picked from commit 5e2065c)
(cherry picked from commit 38c812a)
(cherry picked from commit 494874e)
(cherry picked from commit d396b7f)
(cherry picked from commit 7babc6e)
6543 pushed a commit to 6543-forks/gitea that referenced this issue Feb 26, 2024
- After stumbling upon golang/go#22397 and
reading the implementations I realized that Forgejo code doesn't have
`Sync()` and it doesn't properly error handle the `Close` function.
- (likely) Resolves https://codeberg.org/forgejo/forgejo/issues/1446

(cherry picked from commit 0efcb334c2f123d0869a30d684189eb31e8b983f)
(cherry picked from commit 04ef02c0dd98c7437acb39383d311c0901366508)
(cherry picked from commit 85f2065c9bc6ded9c21909ec76a9e8fc2d22f462)
(cherry picked from commit 8d36b5cce66864e190bad3c9b0973e37ca774a22)
(cherry picked from commit 378dc30fb5a88ffe185c54de7e69224289038bff)
(cherry picked from commit 2b28bf826e51b8ccb4a693001c03ffe6132f7842)
(cherry picked from commit d0625a001e5f8fe202865bec7aadcf0c551d556d)
(cherry picked from commit f161a4f60f1cde80a41bece4929836257b9e0423)
(cherry picked from commit 7430ca43e57683ca324fb20269a60e05cb393589)
(cherry picked from commit ab6d38daf7eeb1dc993bfc0ac1a326af65128168)
(cherry picked from commit 0f703fd02e69bdcf2f77e120ff8641f1b8089020)
(cherry picked from commit 6931a8f6bbfa0fe4f68b462f88c4a3db7ea06306)
(cherry picked from commit 5e2065c)
(cherry picked from commit 38c812acfffe4c83099881a8b47489caba64b42a)
(cherry picked from commit 494874e23f2edb90beabe0827dadefa035e35a71)
(cherry picked from commit d396b7fd47c20d08844f8de1255a32fe0de25249)
(cherry picked from commit 7babc6efe11d0950ac47d3d4724bb5d414bdd6aa)
(cherry picked from commit 2d4dbbe7418d6c1a3a47c7e65350cf65fc9dca19)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests