Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
call Sync() before Close() #279
Conversation
gabriel-samfira
changed the title from
disable disk cache and write buffer
to
call Sync() before Close()
May 5, 2017
babbageclunk
approved these changes
May 17, 2017
The Sync() and Close() changes make sense - I think that since the file gets closed before calling change in AtomicWriteFileAndChange we should make the change func just take the file name.
| + } | ||
| + if err := f.Close(); err != nil { | ||
| + return err | ||
| + } |
babbageclunk
May 17, 2017
Member
This seems wrong - you're closing the file before calling change with it? I guess that works ok in the case of AtomicWriteFile since the change func is just doing a chmod. But it doesn't seem right in general. Is there a reason for not just doing the sync and close after change?
babbageclunk
May 17, 2017
Member
Ok, from talking in IRC I understand why you're doing this - the change func might fail on Windows if the file isn't closed. (Depending on what the change function is.)
babbageclunk
May 17, 2017
Member
In that case though the interface should really be changed - the change func should be passed the file name instead of a closed *File. There's only one use of AtomicWriteFileAndChange in the whole codebase, so it's probably ok to update it.
gabriel-samfira
May 17, 2017
Member
made the change. Also did a recursive grep throughout the code. The AtomicWriteFileAndChange function is only used in this package.
PTAL.
babbageclunk
May 18, 2017
Member
Sorry, I missed this - your change here looks great though. Thanks!
| @@ -155,6 +155,12 @@ func createAndFill(filePath string, mode int64, content io.Reader) error { | ||
| if err != nil { | ||
| return fmt.Errorf("cannot set proper mode on file %q: %v", filePath, err) | ||
| } | ||
| + if err := fh.Sync(); err != nil { |
babbageclunk
May 17, 2017
Member
I started to say that it would be better to put this and the close into the defer and make them avoid clobbering any other errors being returned while still returning any errors from them, but actually it's unwieldy enough that I don't think it's worth doing.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-utils |
gabriel-samfira commentedMay 5, 2017
•
Edited 1 time
-
gabriel-samfira
May 5, 2017
On windows, disk cache and buffers are enabled by default. In the event of a power failure any file that has not yet flushed its contents to disk will become corupt. This leads to state files and agent configs being filled with garbage data.
fixes bug: https://bugs.launchpad.net/juju/+bug/1691193