HN

Comments (301)

• Menu

  masklinn
  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 can not portably retry fsync after failure: the earlier error may have invalidated the IO buffers but IO errors may not be sticky, so a later fsync will have nothing to write and report success
• Menu

  nickcw
  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 an existing error, writes the error from f.Close() in there.
• Menu

  K0nserv
  Rust has the same problem. Files are closed in `Drop` when the value goes out of scope, but all errors are silently ignored. To solve this there's `sync_all`[0].Generally, relying on defer in Go or Drop in Rust for anything that can fail seems like an anti-pattern to me.0: https://doc.rust-lang.org/std/fs/struct.File.html#method.syn...
• Menu

  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)
• Menu

  yyyfb
  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.
• Menu

  adrianmsmith
  Surely this would all go away if Go had an exception handling mechanism like most mainstream languages do?You'd just concentrate on the "happy path", you'd close the file, there'd be nothing to forget or write blog posts about because the exception would be propagated, without needing to write any lines of code.
• cryptonector
  You don't want to just `fsync()`, but also flush whatever is buffered and then `fsync()`.Another thing is that if you're holding an flock on that file, it's nice that closing it will drop the lock. Generally you want to drop the lock as soon as you're done writing to the file. Deferring the dropping of the lock to the deferred close might cause the lock to be held longer than needed and hold back other processes/threads -- this doesn't really apply in TFA's example since you're returning when done writing, but in other cases it could be a problem.Do not risk closing twice if the interface does not specifically allow it! Doing so is a real good way to create hard-to-find corruption bugs. In general if you see `EBADF` when closing any fd values other than `-1`, that's a really good clue that there is a serious bug in that code.
• Menu

  nivyed
  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`): defer func() { cerr := f.Close() err = errors.Join(err, cerr) }()
• Menu

  trashburger
  Arguably, one should call `flush()` on the file first. Resource deallocation must always succeed; otherwise a lot of invariants break. This is why Zig's close method[0] ignores errors (with the exception of `EBADF`).[0]: https://github.com/ziglang/zig/blob/fb0028a0d7b43a2a5dd05f07...
• Menu

  russdill
  On production systems, it may often be better to completely ignore the problem at this level. On modern hardware if a disk is throwing an io error on write you are having a bad day. I can almost guarantee that while you might happily modify your code so it properly returns the error, there almost certainly aren't test cases for ensuring such situations are handled "correctly", especially since the error will almost certainly not occur in isolation.It may often be better to handle the issue as a system failure with fanotify. https://docs.kernel.org/admin-guide/filesystem-monitoring.ht...
• Ferret7446
  More generally, don't ignore errors that you need to handle. This problem isn't exclusive to this very specific case on Go.
• SPBS
  I think anything other than deferring Close() and then calling Close() again explicitly is overengineering. Like anything that requires creating a cleanup function, capturing a named return value, requires contorting in unnatural ways to handle a very common scenario. Just... defer Close() the soonest you can (after checking for errors), then call Close() again at the end. Most sane providers of Close() should handle being called multiple times (I know os.File does, as well as sql.Tx ).
• Menu

  jrockway
  I defer these AND check the errors. https://pkg.go.dev/go.uber.org/multierr#hdr-Deferred_Functio... has a nice API.I wrote my own version for $WORK package (as our policy is that all errors must be wrapped with a message), used like: func foo() (retErr error) { x := thing.Open(name) defer errors.Close(&retErr, x, "close thing %v", name) ... } You can steal the code from here: https://github.com/pachyderm/pachyderm/blob/master/src/inter...Finally, errcheck can detect errors-ignored-on-defer now, though not when run with golangci-lint for whatever reason. I switched to nogo a while ago and it found all the missed closes in our codebase, which were straightforward to fix. (Yes, I wish the linter auto-ignored cases where files opened for read had their errors ignored. It doesn't, so we just check them.)Multi-errors are nice.
• Menu

  Agingcoder
  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.
• Menu

  pansa2
  AFAIK Python/C# use a similar approach to Go - instead of `defer`, they have `using`/`with` statements. Go's `defer` seems more flexible though - it can execute custom code each time it's used, whereas `using`/`with` always call `__exit__`/`Dispose`.How does the Python/C# approach compare to Go's in this situation? How are errors in `__exit__`/`Dispose` handled?
• anon
  undefined
• snatchpiesinger
  "Commit pending writes" and "discard file handle" should ideally be separate operations, with the former potentially returning errors and the latter being infallible. "Discard file handle" should be called from defer/destructiors/other language RAII constructs, while "commit pending writes" should be called explicitly in the program's control flow, with its error appropriately handled or passed up the stack.Whether if it's allowed to write again after a "commit pending writes" operation or not is a separate design decision. If not, then in some languages it can be expressed as an ownership taking operation that still allows for handling the error.
• Menu

  tux3
  Don't call close() twice if you're not using pidfds, this is racy. The fd could be reused in between the two calls. You'll risk closing random things a frustratingly small fraction of the time, creating very hard bugs for yourself.
• Menu

  im3w1l
  Didn't I see this thread the other day including comments? Investigating, Algolia search shows this thread as being posted 2 days ago, and the memorable comments too.
• Menu

  rollulus
  Can someone tell me what’s going on here? This very post including the comments appeared on HN two days as well. I thought I was getting crazy but Google confirms.
• joeshaw
  OP here. I appreciate the comments I've read here, and it might inspire a new blog post: "Defer is for resource cleanup, not error handling."
• Menu

  weinzierl
  Here is a related question that has been on my mind for a while, but I have yet to find a good answer for:If I write to file on a reasonably recent Linux and a sane file system like ext or zfs, at which point do I have the guarantee that when I read the same file back, that it is consistent and complete?Do I really need to fsync or is Linux smart enough to give me back the buffer cache? Does it make a difference if reader and and writer are in the same thread or same process?
• almostdeadguy
  Confused why this is displayed as being 9 hours old when I remember seeing it on the front page a couple days ago. Search also seems to confirm this: https://hn.algolia.com/?dateRange=pastWeek&page=0&prefix=fal...
• slaymaker1907
  Best practice IMO would be to wrap the closable thing in a wrapper object that handles the case where Close is called multiple times and then defer close that one as well as closing at the end of the function. Another idea would be to define a small lambda that either returns the input value or returns error if Close returns an error if you want to use early returns.
• Menu

  iudqnolq
  The end of the article has my favorite solution.This is also how I'd solve it in Rust, except the defer would be implicit. func doSomething() error { f, err := os.Create("foo") if err != nil { return err } defer func(){ _ = f.Close() }() if _, err := f.Write([]byte("bar"); err != nil { return err } return f.Close() }
• w10-1
  Java's Closeable.close() has declared multiple invocations as safe for 20 years, when it first became an interface.
• Menu

  corytheboyd
  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?
• Menu

  spease
  Isn’t what the article suggests (with defer and then WriteString) technically a race condition? Is there no way that the closer can get called before WriteString executes?
• sedatk
  Never thought that `Close` could fail, as `CloseHandle` on Windows never fails. I wonder how .NET behaves in the same scenario on Linux.
• meling
  The following from the first update:if err := f.Close(); err != nil { return err } return nilIs equivalent toreturn f.Close()
• Menu

  cccbbbaaa
  The snippet in the first update is very wrong. The manpage for Linux’s implementation of close() explicitly says that it should not be called again if it fails. Apparently, this is the same under FreeBSD.
• Menu

  zabzonk
  how, when and why can close() fail? and what can you do about it if it does?
• anon
  undefined
• anon
  undefined
• Menu

  karmakaze
  TL;DR - defer Close ignores the error from Close, so don't.