Live data from Hacker News

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

joeshaw.org

271–280 of 311 posts

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

#271
post #172

Boggles my mind that after more than 60 years of computer science, we still design tools (programming languages) where the simplest tasks are full of gotchas and footguns. This is a great example.

I think this post overhypes the issue. So many writes we do just aren’t that important (e.g. logs, cli config, blah), Close fails rarely, and it’s pretty standard for casually developed application software to misbehave once the disk is full or breaking.

This is a classic safety / performance trade off that was properly selected in favor of performance.

The defer Close() is still quite useful as a way to avoid fd leaks.

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

#272

Earlier quoted context omitted.

A file not able to opened is expected, always! accept is no exception here. Your application should not be crashing because of it. If I recall, Kubernetes performs health checks over HTTP, so presumably your application is using the standard library's http server to provide that? If so, accept is full abstracted away. So, if that's crashing, that's a bug in Go. Is that for you to debug, or is it best passed on to the…

There isn't a bug, it's resource exhaustion. You open a bunch of files and they fail to close. You don't log errors on the close, so you have no idea it's happening. Now your app is failing to open new file descriptors to accept HTTP connections. You get a fixed number of fds per app; ulimit -n. If you don't close files you've read, the descriptor is gone. The bug in this case is in the filesystem that hangs on close…

The bug of which we speak is in that your app is crashing. Exhausting open file handles is expected behaviour! Expected behaviour should not lead to a crash. Crashing is only for exceptional behaviour.

The filesystem hanging is unlikely to be a bug. The filesystems you'd realistically use in conjunction with Kubernetes are pretty heavily tested. More likely it is supposed to hang under whatever conditions has lead that to happen.

And, sure, maybe you'll eventually want to determine why the filesystem has moved into that failure state, but most pressing is that your app is crashing. All that work you put into gracefully handling the failing situation going to waste.

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

#273
post #237
post #36

Earlier quoted context omitted.

You interpret Go using a different pattern for error handling as a criticism of or challenge to exceptions. Consider that languages can take different approaches, without having to get into “best” or “worse.” Every approach to error handling has advantages and shortcomings. The Go designers have explained and talked about this decision many times. Some people don’t like it and maybe choose a different language. No on…

> Every approach to error handling has advantages and shortcomings If under ‘every approach’ you explicitly exclude go’s terrible errno syntax sugar, and include exceptions and sum types, then yeah. There is zero advantage to go’s error handling compared to the proper sum typed solutions.

You don't like how Go implements error handling. We get it. Lots of people complain about that. Use a "proper" language more to your liking.

Go's designers have explained their decisions many times, I won't repeat their justifications here. Obviously they had to make choices in line with their overall vision for Go. They feel strongly that programmers should handle or at least acknowledge the possibility of every error, explicitly, right where it gets reported. Exceptions and sum types don't enforce that.

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

#274

Earlier quoted context omitted.

There isn't a bug, it's resource exhaustion. You open a bunch of files and they fail to close. You don't log errors on the close, so you have no idea it's happening. Now your app is failing to open new file descriptors to accept HTTP connections. You get a fixed number of fds per app; ulimit -n. If you don't close files you've read, the descriptor is gone. The bug in this case is in the filesystem that hangs on close…

The bug of which we speak is in that your app is crashing. Exhausting open file handles is expected behaviour! Expected behaviour should not lead to a crash. Crashing is only for exceptional behaviour. The filesystem hanging is unlikely to be a bug. The filesystems you'd realistically use in conjunction with Kubernetes are pretty heavily tested. More likely it is supposed to hang under whatever conditions has lead th…

You're really hung up on Kubernetes but it was an incidental comment in a hypothetical story.

"You wake up and find out that Heroku's staff is anxiously awaiting your departure from your apartment to tell you that your app is down."

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

#275

Earlier quoted context omitted.

That should be only for creating files, and maybe updating their metadata (not sure about that one). The confusion stems from people thinking that files and directories are more different than they are. Both are inodes, and both are basically containers for data. File inodes are containers for actual data, while directory inodes are containers for other inodes. All inodes need to be fsynced when you write to them. Fo…

> 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

A tree is a specific kind of graph.

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

#276

Earlier quoted context omitted.

Stringly typed errors are also idiomatic in go, if you follow the stdlib. Like, are you doing anything with TLS? String matching: https://github.com/golang/go/issues/35234 Using the stdlib ssh stuff? String matching: https://github.com/golang/go/issues/45207 / https://github.com/golang/go/issues/39259 Want to parse an address + port? netip.ParseAddrPort only returns strings ('errors.New' errors). http/http2 is also a…

> Stringly typed errors are also idiomatic in go, if you follow the stdlib. Realistically, you can't follow the standard library, except perhaps the newest additions. Idioms emerge and evolve with use. Much of the standard library was written before Go saw much use, being largely in place before the world got to see Go for the first time. Also, thanks to the Go1 guarantee, cannot be changed now. If the aforementioned…

netip is one of the newer additions to the stdlib (added in ~2022), and follows the venerable stringly typed error idiom.

I always interpreted the preference for stringly typed errors as a way to keep the Go language simpler. Good error handling is complicated and hard to read, and one of Go's values is that programs should be easy to read. As such, if you want good error handling, you should use a different language, like Java or Haskell or C++. This also helps keep people who might demand complicated things like generics away from the language, further keeping it simple.

My understanding was that many of the go idioms are there to scare off programming language theorists, who have a tendency to unnecessarily complicate everything with type theory, and error handling also seems to mostly be in that vein.

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

#277
post #244

Earlier quoted context omitted.

Sure, that's an alternative, although it means there will be some code paths where the error returned by f.Close() becomes the error returned by the entire function and others where it is ignored (though you could easily log it). That might be fine, but you also might want to handle all the cases explicitly and return a combined error in a case where, say, a non-file-related operation fails and then the file also fai…

> becomes the error returned by the entire function If you find the error returned by f.Close to be significant, are you sure returning again it is the right course of action? Most likely you want to do something more meaningful with that state, like retrying the write with an alternate storage device. Returning the error is giving up, and giving up just because a file didn't close does not make for a very robust sys…

It obviously depends on the context. There's no general right or wrong answer to that question.

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

#278
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…

I think a better solution is to write smaller, single-purpose functions. To refer to your example downthread, you should have one function that only writes the file, and another that does the "whole" operation -- calling the function to write the file, checking for errors, and then updating the database.

Then you can use defer in the file-writing function if you so please, and not bother to close at the end explicitly, without issue. A more robust example might be to even include the sync call in the deferred function (and even clean up the file itself on error). To re-use your example from your blog post:.

    func helloNotes() (err error) {
        var f *os.File
        f, err = os.Create("/home/joeshaw/notes.txt")
        if err != nil {
            return
        }

        defer func() {
            if err == nil {
                err = f.Sync()
            }

            cerr := f.Close()
            if err == nil {
                err = cerr
            }

            if err != nil {
                os.Remove("/home/joeshaw/notes.txt")
            }
        }()

        err = io.WriteString(f, "hello world")
        return
    }
I would probably move that out into a helper, though, so I could do something like

    defer SafeClose(f, &err)
instead, and be able to use it elsewhere. Hell, even without defer, it's nice to have a helper that will sync and close for you so you can avoid the boilerplate, if you have lots of different bits of code that writes files.

FWIW, I'm not sure why you are so negative on named return values, but I'm at best a novice Go programmer, so perhaps I don't fully understand why they aren't great (I guess it does look weird to me to have bare `return` statements that do actually return a value even though it doesn't look like it). Your argument about the return value possibly being modified after the core function finishes being unintuitive doesn't really strike me as a big deal either.

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

#279
post #172

Boggles my mind that after more than 60 years of computer science, we still design tools (programming languages) where the simplest tasks are full of gotchas and footguns. This is a great example.

It's hardly a footgun. Close may be able to report some additional errors with getting the data on persistent storage but it won't report all of them anyway. For most applications, ignoring the return of close is perfectly fine in practice.

the close() manpage says that it shouldn't be retried anyway, because one might end up closing a file that meanwhile had been opened with the same handle by a different thread.

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

#280
post #257
post #172

Boggles my mind that after more than 60 years of computer science, we still design tools (programming languages) where the simplest tasks are full of gotchas and footguns. This is a great example.

The problem is that the filesystem primitives are garbage, so it is impossible to make something safe and reasonably performant. This is not a case of "speed at all costs" where huge footguns are added for marginal performance, this is avoiding 10x and up slowdowns that would be required to be safe due to the anemic primitives. If the filesystem had better primitives/APIs, like barriers and proper asynchronous comple…

To be honest, you sound like someone who has no experience with designing a filesystem and thinks he can do better because doesn't understand the problems at all.
Post reply on HN