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

151–160 of 311 posts

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

#151

Post by sgarland »

eatonphil wrote: post #121
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,…

Can anyone provide links to learn more about these misconceptions and the bugs they caused? Edit: talking about filesystem misconceptions, not fsyncgate.

EDIT: realized you were asking about the ext4 bugs, not the parent comment of that one. Oh well, keeping this up anyway for others.

Postgres has a whole wiki page [0] about it, it's quite a read. They also link a [1] MySQL commit to fix the same issue.

[0]: https://wiki.postgresql.org/wiki/Fsync_Errors

[1]: https://github.com/mysql/mysql-server/commit/8590c8e12a3374e...

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

#152

Post by kccqzy »

dist-epoch wrote: post #13
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…

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?

You don't need even to bother with fsync unless you are developing a database or use the file system like a database.

There's a reason Apple made fsync useless and introduced F_FULLSYNC. They know developers have an incentive to overestimate the importance of their own files at the detriment to system responsiveness and power draw.

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

#153

Post by randomdata »

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.

That's not it. You can't write to a file handle returned by os.Open.

https://pkg.go.dev/os#Open

    func Open(name string) (*File, error)
    Open opens the named file for reading. If successful, methods on the returned file can be used for reading; the associated file descriptor has mode O_RDONLY. If there is an error, it will be of type *PathError.

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

#154

Post by [deleted] »

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,…

[deleted]

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

#155

Post by gwd »

xg15 wrote: post #22
devjab wrote: post #10

Earlier quoted context omitted.

Explicit error handling is a choice and implicit error handling through exceptions is not necessarily a feature. Both have advantages and disadvantages, I’d say the more “modern” approach actually the opposite to what you state here, and is in my opinion the way to Go (pun int intended), though it’s also how Haskell does it. You’ll find the same philosophy in Rust, Zig, Swift and others which all build on the previou…

> Explicit error handling is a choice Yeah, it is - a bad choice IMO. I know the "if err != nil" pattern is spoken up as some sort of cultural idiosyncrasy of Go, similar to the whitespace formatting in python. But so far, I haven't seen any actual data (or even arguments) why it is superior to exceptions, or which inherent problems of exceptions it solves. (The classical example of "it makes control flow more obviou…

> But so far, I haven't seen any actual data (or even arguments)

OK, here's an argument.

- In order to write resilient software, programs must handle not only the "happy path" when things succeed, but the path where things might fail.

- Thus it is important for developers to 1) be aware of which operations may fail fail, and b) think about what the program should do in that case.

- Exceptions make it easier for the programmer to forget that something might fail, and to avoid thinking about what to do if it does fail.

- Go's error handling idiom makes it clear that an operation might fail, and prompts programmers to think about what to do in that case. (They may of course choose not to think about it, but at least they made a conscious choice at some level.)

Thus Go's error handling idiom nudges developers towards more resilient software than exception-based workflows.

Or to put it differently: Programming systems which may fail simply is ugly: there are an exponential number of ways a system may fail, and each one must be handled correctly. Exceptions hide this ugliness, but by doing so make it more likely that there will be cases not handled correctly. By exposing this ugliness, Go makes it more likely that most cases will be handled correctly.

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

#157

Post by jryan49 »

weinzierl wrote: post #116
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…

"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." And if you need this in Java you still have resort to ugly hacks. https://github.com/apache/lucene/issues/7231

This says the bug is fixed? https://bugs.openjdk.org/browse/JDK-8066915

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

#158

Post by throwaway173920 »

nivyed wrote: post #149

The article suggests using a named return value `err` to allow the return value of `Close` to be be propagated - unless doing so would overwrite an earlier error: defer func() { cerr := f.Close() if err == nil { err = cerr } }() Wouldn't it be better to use `errors.Join` in this scenario? Then if both `err` and `cerr` are non-nil, the function will return both errors (and if both are `nil`, it will return `nil`): def…

IMO the formatting of the error string returned by errors.Join is atrociously opinionated and not very logging-friendly - it adds a newline between each error message. I know I'm not the only one that has this opinion

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

#159

Post by admax88qqq »

nivyed wrote: post #149

The article suggests using a named return value `err` to allow the return value of `Close` to be be propagated - unless doing so would overwrite an earlier error: defer func() { cerr := f.Close() if err == nil { err = cerr } }() Wouldn't it be better to use `errors.Join` in this scenario? Then if both `err` and `cerr` are non-nil, the function will return both errors (and if both are `nil`, it will return `nil`): def…

Go error handling is brutal.

I miss exceptions

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

#160

Post by joeshaw »

nivyed wrote: post #149

The article suggests using a named return value `err` to allow the return value of `Close` to be be propagated - unless doing so would overwrite an earlier error: defer func() { cerr := f.Close() if err == nil { err = cerr } }() Wouldn't it be better to use `errors.Join` in this scenario? Then if both `err` and `cerr` are non-nil, the function will return both errors (and if both are `nil`, it will return `nil`): def…

OP here. Another commenter pointed out that `errors.Join` didn't exist when I wrote this, but I wouldn't have changed my guidance if it had.

The core issue here is that you want to deal with errors as soon as possible. The async nature of writes to files makes this more challenging. Part of the purpose of this post was to inform people that you don't necessarily always see write errors at write-time, sometimes you see them at close-time and should handle them there. (And sometimes you don't see them at all without fsync.)

Even if you do handle errors in your deferred close, you may be taking other actions that you'd rather not have until the file i/o is completed, and this could leave your program in an inconsistent state. Side effects in Go are common, so this is a practical concern and it is hard to spot and debug.

Post reply on HN