Live data from Hacker News

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

joeshaw.org

191–200 of 311 posts

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

#192

I’m a bit new to Golang, but not good programming practices. Isn’t ignoring the error value returned by a function a very bad practice in general? Regardless of if the function call returning it is used in defer? Not just for file write operations?

Sometimes there is nothing you can do when there is an error, in that case there is no point in adding several layers of error forwarding until you ignore it somewhere higher up.

Is… NOT ignoring errors just not an option? I don’t get it. If you propagate errors up but not all the way to being handled, haven’t you failed in a very simple, easy to fix way? Should you have a linter catching these things?

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

#193
post #76
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 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, you may have just gotten your application into an inconsistent state and it's very hard to see in code why this might be.

Can you give an example case of how this could happen?

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

#194

Earlier quoted context omitted.

> 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

SQLite doesn't do any magic other than fsync. Using it to deal with power failures is nonsense.

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

#195

Earlier quoted context omitted.

I’m not well versed enough on the subject to know if there’s important nuance here that I’m missing… but that doesn’t sound like a contradiction to me. That’s effectively a map with O(n) access, right? I think I would more generally refer to that as a “collection”, but it certainly contains inodes as you describe it.

IMO it's better to think of a directory as "containing" only the named references to inodes. Other directories on the same filesystem may contain other named references to the same inodes ("hard links"). The inodes themselves are more like free floating anonymous objects independent of any particular directory, and might not have a named reference at all (O_TMPFILE; or the named reference was deleted but a file descr…

This definitely seems like a more meaningful distinction to me. It’s also closer to the mental model I had coming into the discussion, FWIW.

And I think it makes the collection/container terminology distinction sharper too. Depending on context, I think it’s usually reasonable (if imprecise) to describe a bucket of references or pointers to things as a collection of those things. But I don’t think it makes as much sense to call it a container of those things, except in a really abstract sense.

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

#196

Earlier quoted context omitted.

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.

You can't safely assume that, but what are you going to do about it when it does fail? There is nothing you can do. There isn't usually a CloseClose function to use when Close fails. If Close fails, that's it. You're otherwise out of luck.

Certainly, in the write failure case where there is write failure you'd want to try writing to something else (ideally), notify someone that an operation didn't happen (as a last resort), or something to that effect in order to recover.

But in this case there is no need to try again and nobody really cares. Everything you needed the resources for is already successfully completed. If there is failure when releasing those resources, so what? There is nothing you can do about it.

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

#197

Earlier quoted context omitted.

> It’s basically a graph. do you mean it's a basically a tree? Because if it were just a graph, you could still have edges from the grandparent to the grandchild in addition to the one from the former to the child

This ... depends. In normal POSIX land, hard links break the tree structure, for one, so you get a DAG but not a tree. I think some file systems do enforce tree structure, though - hard links are not supported everywhere. It used to be possible ages ago to hard link to directories, which meant that you could have actual cycles and a recursive tree-walking algorithm would never terminated. (As far as I know you can st…

Logically, ".." is a hard link to the parent directory. This may or may not actually be the case in the filesystem on the disk.

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

#198

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…

Write, fsync, rename then fsync directory if you need an ordering guarantee that the rename is a transaction barrier.

Of course, the fun part is that the filesystem can’t really guarantee fsync behavior if drives lie about it which many consumer drives do for benchmark reasons. Fun, no?

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

#199

Earlier quoted context omitted.

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

SQLite doesn't do any magic other than fsync. Using it to deal with power failures is nonsense.

Well generally OP is correct that SQLite will more likely have written fsync code correctly and be fairly robust against power failures due to the WAL. Additionally, it’s not doing any renaming which you would need with plain files.

So while it doesn’t do any other magic, it’s more likely to handle power failures correctly.

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

#200

Earlier quoted context omitted.

Go error handling is brutal. I miss exceptions

Exceptions aren't exceptional though; they are too expensive for not-exceptional errors, like failing writes. That said, a language feature where you can throw lightweight error values without generating a stack trace etc might be a middle ground. But it won't land in Go, given the discussion about alternative error handling some years ago. Anyway, in practice it's not that bad. A write can go wrong, you as a develop…

Exceptions are slow in some languages based upon how they are implemented. I'm not convinced that is fundamental to exceptions and rather a choice of how they were implemented. In Java exceptions arn't actually that slow, most of the cost is just allocating the exception object (and allocations in Java are fast).

> Exceptions lead a developer to miss errors, or to not handle them in a finegrained manner - e.g. generic "catch all, log error, maybe" style error handling.

I don't see how Go error handling makes people handle things any more explicitly than exceptions. Most people just `if err != nil { return err }`, which to be honest is the _correct_ logic in many cases, and it's pretty easy to forget to check if err and keep on trucking. At least with exceptions if you don't catch it your thread terminates making unhandled exceptions

Exception bubbling means its easier to catch the error at the level that makes sense, and because they are real objects type checking is easier as opposed to the performance of `errors.Is()` which is surprisingly slow.

Post reply on HN