Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Don't defer Close() on writable files (2017)

joeshaw.org

Post reply on HN
Print view View original

181–190 of 311 posts

Re: Don't defer Close() on writable files (2017)

#181

Post by Cthulhu_ »

stbenjam wrote: post #124
joeshaw wrote: post #76

Earlier quoted context omitted.

I caution against this approach, as you are not really dealing with the error when it occurs. If the work you do after the defer has other side effects, you may have just gotten your application into an inconsistent state and it's very hard to see in code why this might be. `defer` is really not well-suited for error handling, its benefit is mainly in resource cleanup where failure is impossible or doesn't matter. (T…

> If the work you do after the defer has other side effects Defer is by definition the last work you do in a function, there won't be more work except by the caller who will get the error returned to them. If you are structuring a function that writes a file, and then does something with it, defer isn't appropriate, since you should close it before you do any more work.

It's possible to have multiple defers in a function though (so you have multiple "last work in a function"; nowhere is it dictated that a function should only have one operation that needs to clean something up at the end. Think for example copying one file to another.

Re: Don't defer Close() on writable files (2017)

#182

Post by nextaccountic »

praptak wrote: post #113
masklinn wrote: post #7

There are more wrinkles with this: - if you are creating a file, to ensure full synchronisation you also need to fsync the parent directory, otherwise the file can be fsynced but the update to the directory lost - if sync fails, you can not assume anything about the file, whether on-disk or in memory , critically one understanding which got dubbed "fsyncgate" and lead to many RDBMS having to be updated is that you ca…

> otherwise the file can be fsynced but the update to the directory lost It also goes the other way - the update to the directory can be fsynced but the file lost. This can break the "create temp file, write, close, rename to current" scenario (when the intention is to replace file contents atomically). POSIX doesn't guarantee the order in which data hits the disk, so the above scenario can become "create temp file,…

> It also goes the other way - the update to the directory can be fsynced but the file lost. This can break the "create temp file, write, close, rename to current" scenario (when the intention is to replace file contents atomically).

> POSIX doesn't guarantee the order in which data hits the disk, so the above scenario can become "create temp file, write [contents still in memory only], rename to current [written to disk], power failure".

Wait.. is there a way to do this correctly? At which points is fsync warranted, and how many fsyncs do we need for the whole "write file then mv it on top of current" to not lose data on power failure?

Re: Don't defer Close() on writable files (2017)

#183

Post by garrettr_ »

Verdex wrote: post #178
dist-epoch wrote: post #13

Earlier quoted context omitted.

So if I do fopen/fwrite/fsync/fclose, that is not enough? That is crazy, I think 90% of apps don't fsync the parent directory. Also, how many levels of parents do you need to fsync?

Great another case of simple thing everyone knows is simple but turns out to be horrifyingly complicated and heavily system dependent, but only on occasion so you can get 80% of the way through your career before encountering the gaps in your knowledge. I guess I'll add it to the list. Of course on the other hand, I was already thinking that I should just use SQLite for all my file handling needs. This little nugget…

w.r.t SQLite, the only horrifying revelation I’ve had is that it allows NULLs in composite primary keys, which I’ve seen lead to some nasty bugs in practice.

Re: Don't defer Close() on writable files (2017)

#184

Post by Cthulhu_ »

red_admiral wrote: post #100
nickcw wrote: post #23

Here is my favorite solution to this problem // CheckClose is a utility function used to check the return from // Close in a defer statement. func CheckClose(c io.Closer, err *error) { cerr := c.Close() if *err == nil { *err = cerr } } Use like this - you must name the error return func whatever() (err error) { f, err := os.Open(blah) // ... defer CheckClose(f, &err) // ... } This closes the file and if there wasn't…

I wouldn't mind the language adding some syntax for this, based on the java try-with-resource but using error values rather than exceptions. Something like f, err = with(os.Open(thing)) { // do stuff } // at this point err has the same semantics as your CheckClose

I believe your example doesn't introduce any new syntax though. Go is highly resistant to adding syntax (and that's a good thing, it keeps the tooling fast and barrier to entry low).

Re: Don't defer Close() on writable files (2017)

#185

Post by Cthulhu_ »

randomdata wrote: post #145
bryancoxwell wrote: post #107

Earlier quoted context omitted.

The whole point of this post is that an error returned from file.Close DOES matter

You seem confused. The article is about writing a file where it does matter, but the comment example, which is what we're talking about, only reads a file. If close fails after read, who gives a shit? What difference is it going to make? All your read operations are complete already. Close isn't going to trigger a time machine that goes back and time and undos the reads you've performed. It is entirely inconsequentia…

Only if you can safely assume the OS, file system, or std lib cleans up any open file handles that failed to close; I'm 99% sure this is the case in 99% of cases, but there may be edge cases (very specific filesystems or hardware?) where it does matter? I don't know.

Re: Don't defer Close() on writable files (2017)

#186

Post by Cthulhu_ »

red_admiral wrote: post #101
randomdata wrote: post #54

Earlier quoted context omitted.

>>> f, err := os.Open(blah) // ... defer CheckClose(f, &err) What knowledge do you hope to gain of f.Close fails here?

If I understand the OP correctly, if Close() fails then you can't trust that the data was written, even if the previous Write() succeeded.

Exactly that; for critical operations like e.g. a database, if a write fails you've got corrupted data and you have a Major Issue.

That said, I'm not sure how they would handle a file close failure, wouldn't the file be corrupted anyway because some of the bits may have been written? Then again, at least you can raise the alarms if Close fails, because silent failures are worse than failures.

Re: Don't defer Close() on writable files (2017)

#187

Post by TickleSteve »

jiggawatts wrote: post #21
dist-epoch wrote: post #13

Earlier quoted context omitted.

So if I do fopen/fwrite/fsync/fclose, that is not enough? That is crazy, I think 90% of apps don't fsync the parent directory. Also, how many levels of parents do you need to fsync?

Crazy is the right term. File system APIs in general have too many sharp edges and need a ground-up rethink. Consider S3-like protocols: these recognise that 99% of the time applications just want “create file with given contents” or “read back what they’ve previously written.” The edge cases should be off the beaten path, not in your way tripping you up to when you want the simple scenario.

Is the filesystem the correct abstraction? For most applications, a database-like API is more appropriate, hence SQLite.

Re: Don't defer Close() on writable files (2017)

#188

Post by nextaccountic »

This is also why, in Rust, relying on drop to close a file (which ironically is the poster child for RAII) is a bad pattern. Closing a file can raise errors but you can't reasonably treat errors on drop.

What we really need is a way to handle effects in drop; one way to achieve that is to have the option to return Result in a drop, and if you do this then you need to handle errors at every point you drop such a variable, or the code won't compile. (This also solves the async drop issue: you would be forced to await the drop handling, or the code wouldn't compile)

Re: Don't defer Close() on writable files (2017)

#189

Post by dlock17 »

Agingcoder wrote: post #125

I dislike the multiple close pattern - I was bitten by this behavior years ago, when the second close() ended up closing another file which had been opened between the first and second close ( I think they were actually sockets ). It was a bona fide bug on my side , but it made for unpleasant memories, and a general distrust of such idioms on my side unless there's a language wide guarantee somewhere in the picture.

In the stdlib Go code for file.Close, it doesn't actually do the syscall after the first call to Close, so there's your language guarantee.

That is a scary sounding error though.

Re: Don't defer Close() on writable files (2017)

#190

Post by paulsutter »

nextaccountic wrote: post #182
praptak wrote: post #113

Earlier quoted context omitted.

> otherwise the file can be fsynced but the update to the directory lost It also goes the other way - the update to the directory can be fsynced but the file lost. This can break the "create temp file, write, close, rename to current" scenario (when the intention is to replace file contents atomically). POSIX doesn't guarantee the order in which data hits the disk, so the above scenario can become "create temp file,…

> It also goes the other way - the update to the directory can be fsynced but the file lost. This can break the "create temp file, write, close, rename to current" scenario (when the intention is to replace file contents atomically). > POSIX doesn't guarantee the order in which data hits the disk, so the above scenario can become "create temp file, write [contents still in memory only], rename to current [written to…

Sqlite is much better than raw files to keep data intact on power failures, and be sure to study the options carefully

If you truly need to use files you can take other steps such as mv the old file to .bck before mv the new file, but I really think you want sqlite

Post reply on HN