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

Jump to bottom

os: document that WriteFile is not atomic #56173

Closed
mvdan opened this issue Oct 12, 2022 · 11 comments
Closed

os: document that WriteFile is not atomic #56173

mvdan opened this issue Oct 12, 2022 · 11 comments
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@mvdan
Copy link
Member

mvdan commented Oct 12, 2022

As I was replying to #56172, I was looking at the docs for https://pkg.go.dev/os#WriteFile, which say:

// WriteFile writes data to the named file, creating it if necessary.
// If the file does not exist, WriteFile creates it with permissions perm (before umask);
// otherwise WriteFile truncates it before writing, without changing permissions.
func WriteFile(name string, data []byte, perm FileMode) error {

I know that this operation isn't atomic: for example, if a write to the file fails half-way through, we may be left with a partial file. That is why alternatives like https://pkg.go.dev/github.com/google/renameio#WriteFile exist.

However, by just reading the docs as a new user, it could be an easy misconception. A lot of the other os APIs like Create, Mkdir, or Chmod are atomic at the system level - they only perform a single system call which modifies the filesystem, so we can't be left with partial changes.

Perhaps we should add a godoc note that the API is not atomic. That is, that if an error occurs, the file might be left behind. I imagine this won't matter for many cases, but the warning would be very useful for any Go program which cares about invalid or incomplete states.

cc @ianlancetaylor @stapelberg
@mvdan
Copy link
Member Author

mvdan commented Oct 12, 2022

Trying to think about what other os operations modify the filesystem in multiple steps. MkdirAll and RemoveAll come to mind, but I don't think we need to worry about RemoveAll because it is already a destructive operation by design.

@jacobishao
Copy link
Contributor

as far as I know, open the file in append mode, then call WriteFile to write data is atomic.

@mvdan
Copy link
Member Author

mvdan commented Oct 12, 2022

@jacobishao that doesn't make sense to me. WriteFile takes a path to a file, not an *os.File, so you can't choose to open the file in append mode. I also don't think opening the file in any one mode would result in atomic writes - the first write could fail, for example, leaving the file behind.

@ghost
Copy link

ghost commented Oct 12, 2022

Tbf I don't think this really needs to be documented. If you know how syscalls work it should be obvious that it can't be atomic because it needs an open syscall.

@mvdan
Copy link
Member Author

mvdan commented Oct 12, 2022 •
edited

@yujiri8 the API doesn't prevent the operation from being atomic; see https://pkg.go.dev/github.com/google/renameio#WriteFile. A Go user can read the code behind os.WriteFile and figure out that it's not atomic, but most users will just read the documentation.

I don't think there's a need to document whether every OS or I/O API is atomic or not, just like we don't document whether every API is safe for concurrent use, but I think in this case the current documentation is somewhat prone to being misunderstood.

@ianlancetaylor ianlancetaylor added this to the Backlog milestone Oct 12, 2022
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Oct 12, 2022
@ianlancetaylor
Copy link
Contributor

Updating the docs sounds good to me.

@SrijanSriv
Copy link

i would imagine that the docs require a clear heads up just to draw attention, which will be enough to connect the dots. should it discuss/link the alternatives?

@jacobishao
Copy link
Contributor

@jacobishao that doesn't make sense to me. WriteFile takes a path to a file, not an *os.File, so you can't choose to open the file in append mode. I also don't think opening the file in any one mode would result in atomic writes - the first write could fail, for example, leaving the file behind.

sorry. it's Write method of *os.File, not os.WriteFile. but if you do man open, you will see
image
but it has nothing to do with os.WriteFile we discussed here.

@KaithyRookie
Copy link

How about adding a comment like this:
// Note that WriteFile does not guarantee atomicity, if file writing fails halfway through, the file content will be incomplete

@ianlancetaylor
Copy link
Contributor

@SrijanSriv The docs for os.WriteFile should not discuss alternatives. That's not the place for that. Docs should be accurate but short.

@RichardoC RichardoC mentioned this issue Oct 17, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/443495 mentions this issue: os: document that WriteFile is not atomic

@golang golang locked and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

os: document that WriteFile is not atomic
6 participants
@ianlancetaylor @mvdan @jacobishao @gopherbot @KaithyRookie @SrijanSriv