38 ms·

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

• K0nserv 2y ago
  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.sync_all https://doc.rust-lang.org/std/fs/struct.File.html#method.syn...
  • kzrdude 2y ago
    An ownership consuming close(self) would make sense, but has not been added, there must be some good reason for that?
    • masklinn 2y ago
      That nobody has gone through the effort of collating its requirements and writing an RFC after https://github.com/rust-lang/rfcs/pull/770 https://github.com/rust-lang/rfcs/pull/770 was closed (back in 2015). I assume a big issue is that this is full of edge cases up the ass, and the value is somewhat limited in the sense that if you know you want durable writes you'll sync() and know you're fucked if you get an error, but close() does not guarantee a sync to disk, as the linux man page indicates: > A successful close does not guarantee that the data has been successfully saved to disk, as the kernel uses the buffer cache to defer writes. So you'd need a "close", and a "close_sync", and possibly also a "close_datasync" (if you're ok with discarding metadata). And one could argue at that point `close` has essentially no value beyond hopefully getting rid of the fd / handle, and drop already does a fine job of that.
    • Arch485 2y ago
      IIRC the Rust book talks about files automatically being closed when dropped, and how that's better than having a close method. That's probably why it's not a separate method, even though it suppresses errors.
• tux3 2y ago
  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.
  • silon42 2y ago
    Surely File.close will clear the fd on success...
    • tux3 2y ago
      You're right, looks like it does!
    • MartinodF 2y ago
      Yeah but is it safe for concurrent use?
      • masklinn 2y ago
        If you’re performing concurrent unprotected closing of an fd, you’re deep into the wild lands of nonsense already.
      • lionkor 2y ago
        (pretty much) only channels are safe for concurrent use in go
• adrianmsmith 2y ago
  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.
  • iainmerrick 2y ago
    You're not wrong, but that ship sailed away a dozen years ago.
  • delusional 2y ago
    If you've never seen somebody type 'catch (Exception e) { logger.log("should never happen", e);}' then sure. In the real world people will often explicitly ignore the error, even when they are confronted with it.
  • devjab 2y ago
    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 previous decades of throwing exceptions and how terribly that scales in terms of maintainability. Even in the “old world” like with Java you have Kotlin which does both.
    • saurik 2y ago
      I feel like you might be claiming that Haskell does something like Go does, but it actually doesn't: it supports monads, and so uses a monad to hide the error semantics entirely, providing exception-like syntax with automatic propagation. (I might misunderstand your use of "though", though? It could be that you were just noting in passing how Haskell disagrees with all of these supposedly-"modern" languages, and instead leaned into the sane happy path semantics, thanks to monads.) (edit: to be clear, though... I do not think exceptions solve this. I wrote a comment elsewhere on this thread about the semantics issue, but a few other people also wrote similar things while I was trying to type my overly-verbose reply ;P.)
• masklinn 2y ago
  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
  • dist-epoch 2y ago
    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?
    • masklinn 2y ago
      > So if I do fopen/fwrite/fsync/fclose, that is not enough? That is my understanding. > Also, how many levels of parents do you need to fsync? Only one, at least if you didn't create the parent directory (if you did then you might have to fsync its parent, recursively). The fsync on the parent directory ensures the dir entry for your new file is flushed to disk.
      • magicalhippo 2y ago
        I've never heard about this in my years of programming. I just tried to read through the Win32 documentation, as I've done several times over the years, and it mentions a lot of edge cases but not this that I could see. Is this some Linux/Unix specific thing? Am I blind?
        • masklinn 2y ago
          I am talking about posix semantics yes, I have no idea how things work on windows.
• trashburger 2y ago
  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/fb0028a0d7b43a2a5dd05f075ded22746f92faf6/lib/std/posix.zig#L263 https://github.com/ziglang/zig/blob/fb0028a0d7b43a2a5dd05f07...
  • AndyKelley 2y ago
    Note that the "unreachable" there is equivalent to assert(error != EBADF), so really it's not even an exception, it's just helpful to crash there when debugging if you get that error. Important to understand that EBADF is not a catchable error because the kernel may have already reused that file descriptor for something else, in which case you wouldn't get EBADF, you would close an unrelated file descriptor.
• deleted 2y ago
  [deleted]
• nickcw 2y ago
  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.
  • Belphemur 2y ago
    I still question why defer doesn't support doing exactly that. After all it's like the go language provide us with a cleanup function that in 99% of the time shouldn't be used unless we manually wrap what it's calling to properly handle error. In the end, what's the point of defer ?
    • randomdata 2y ago
      > I still question why defer doesn't support doing exactly that. When would it ever be useful? You'd soon start to hate life if you actually tried using the above function in anything beyond a toy application. > 99% of the time shouldn't be used 1. 99% of the time it is fine to use without further consideration. Even if there are errors, they don't matter. The example from the parent comment is a perfect case in point. Who cares if Close fails? It doesn't affect you in any way. 2. 0.999% of the time if you have a function that combines an operation that might fail in a manner you need to deal with along with cleanup it will be designed to allow being called more than once, allowing you, the caller, to separate the operation and cleanup phases in your code. 3. 0.001% you might have to be careful about its use if a package has an ill-conceived API. If you can, fix the API. The chances of you encountering this is slim, though, especially if you don't randomly import packages written by a high school student writing code for the first time ever.
      • bryancoxwell 2y ago
        The whole point of this post is that an error returned from file.Close DOES matter
• pansa2 2y ago
  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?
  • d0mine 2y ago
    > Exceptions that occur during execution of this method will replace any exception that occurred in the body of the with statement https://docs.python.org/3/library/stdtypes.html#contextmanager.__exit__ https://docs.python.org/3/library/stdtypes.html#contextmanag...
    • pansa2 2y ago
      Thanks!
  • Too 2y ago
    From the application point of view, exceptions are more natural to catch, so if the close inside the exit-function throws, the nearest catch block will not be far up the stack. Compared to go, where you are much more unlikely to have a recover block, because it is considered such a rare case.
• cccbbbaaa 2y ago
  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.
  • klodolph 2y ago
    The implementation of `os.File.Close()` in Go clears the file descriptor. https://pkg.go.dev/os#File.Close https://pkg.go.dev/os#File.Close “Close will return an error if it has already been called.” An os.File is a data structure containing a file descriptor and some other fields. It is safe to call Close() multiple times, because it will only call the underlying syscall close() once.
• weinzierl 2y ago
  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?
  • fanf2 2y ago
    Normally, immediately when write(2) returns. It’s more complicated if the computer shut down in between, depending on how clean the shutdown was.
• karmakaze 2y ago
  TL;DR - defer Close ignores the error from Close, so don't.
  • klodolph 2y ago
    Eh, the real answer is close twice (as in the article update): func f(filename string) error { fp, err := os.Create(filename) if err != nil { return err } defer fp.Close() if _, err := fmt.Fprintln(fp, "Hello, world!"); err != nil { return err } return fp.Close() // safe to call multiple times } The .Close() method is safe to call multiple times. This behavior is documented on the os.File
  • jagrsw 2y ago
    It might be well implementation-dependent. On Linux, the close() system call rarely fails unless you provide an invalid file descriptor. L.Torvalds has stated that on Linux, close() immediately removes the file descriptor from the process, regardless of the underlying implementation's success or failure. Any errors related to the actual closing of the file are handled within the kernel and won't affect user-space programs. I know that go Close is not posix/linux close, but in majority of cases it'll boil down to it. To quote: Retrying the close() after a failure return is the wrong thing to do, since this may cause a reused file descriptor from another thread to be closed. This can occur because the Linux kernel always releases the file descriptor early in the close operation, freeing it for reuse; the steps that may return an error, such as flushing data to the filesystem or device, occur only later in the close operation. Many other implementations similarly always close the file descriptor (except in the case of EBADF, meaning that the file descriptor was invalid) even if they subsequently report an error on return from close(). POSIX.1 is currently silent on this point, but there are plans to mandate this behavior in the next major release of the standard.
• Ferret7446 2y ago
  More generally, don't ignore errors that you need to handle. This problem isn't exclusive to this very specific case on Go.
• im3w1l 2y ago
  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.
  • im3w1l 2y ago
    @dang did the timestamps get messed up?
    • fragmede 2y ago
      https://news.ycombinator.com/item?id=26998308 https://news.ycombinator.com/item?id=26998308
      • im3w1l 2y ago
        Why would the comments have changed timestamps though?
        • jsnell 2y ago
          The posting timestamp is temporarily adjusted, such that people don't complain about a week day old submission being on the front page. The comment timestamps are temporarily adjusted to be consistent with the adjusted posting times, such that readers aren't confused by the comments predating the apparent posting time. The timestamps will revert back to their original values in a few days. What I don't understand is why this went into the second chance pool if the original submission made it to the front page and got >100 points.
  • tome 2y ago
    I noticed this before: https://news.ycombinator.com/item?id=40049170 https://news.ycombinator.com/item?id=40049170
• deleted 2y ago
  [deleted]
• deleted 2y ago
  [deleted]
• Agingcoder 2y ago
  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.
  • jsndnnd 2y ago
    I don't understand how that could happen, since the original file handle would have been invalidated. Which operating system did you experience this under and was it the operating system, your Libc or what else in the stack which caused this?
    • dwattttt 2y ago
      The scenario is that after the original file handle is closed, a new open occurs and the file is assigned the same handle value. Then code acting on the stale handle of the first file closes it, and accidentally closes the new file instead.
  • dlock17 2y ago
    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.
    • Agingcoder 2y ago
      Older versions of go (1.0 for example ) were much less safe. I had a look at the code, and it closes the file directly, and marks it as unusable. However, if you do concurrent operations, you can race and close twice the underlying fd - which I think was my bug ( I shouldn’t have been closing things twice anyway !)
• snatchpiesinger 2y ago
  "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.
• SPBS 2y ago
  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 ).
• rollulus 2y ago
  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.
  • Izkata 2y ago
    If a post doesn't get much attention, there's a small chance of it getting a boost a few days later. If this happens, the timestamps are all adjusted to make it look like it was just posted. More details: https://news.ycombinator.com/item?id=26998308 https://news.ycombinator.com/item?id=26998308
• iudqnolq 2y ago
  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() }
  • klodolph 2y ago
    Yes, note you’ll also see: defer f.Close() Instead of, defer func(){ _ = f.Close() }() I think this is likely a code style difference due to working with a linter that alarms on discarded error returns, but I’m not sure. Both options have the same behavior, unless you reassign f (defer f.Close() will use the original value, defer func() ... will use the current value).
    • iudqnolq 2y ago
      Yup, I wrote it that way because I have a linter for unused error returns with a handful of exceptions (like closing http request bodies).
      • klodolph 2y ago
        I think I would add an exception for this too, because it crops up a lot.
        • iudqnolq 2y ago
          I can't decide because in this pattern you ignore once and check once and I like the lint for the check. Ideally the linter could recognize this pattern. Even better would be if the linter could catch when you never close, I've made that mistake a few times.
• spease 2y ago
  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?
  • assbuttbuttass 2y ago
    defer gets executed at the end of the current function
    • spease 2y ago
      Thanks! Not a Go programmer, so I saw the parentheses after the cloaure def and assume it translated to “execute this on a goroutine in the background immediately”
  • tomohawk 2y ago
    This is the way. func doSomething() (err error) { var f *os.File f, err = os.Create("foo") if err != nil { return } defer func(){ if nil != f { f.Close() } }() _, err = f.Write([]byte("bar") if err != nil { return } err = f.Close() f = nil return } EDIT: fixed the bug
• nivyed 2y ago
  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) }()
  • jfindley 2y ago
    This is from 2017, errors.Join did not exist at the time. But yes, today you'd do it differently.
  • throwaway173920 2y ago
    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
    • Matl 2y ago
      Wouldn't you use slog if you want logging friendly?
    • kjksf 2y ago
      It's a trivial function: https://cs.opensource.google/go/go/+/refs/tags/go1.23.1:src/errors/join.go;l=19 https://cs.opensource.google/go/go/+/refs/tags/go1.23.1:src/... You could write your own errorsJoin() and change Error() method to suit your needs. But really in this particular scenario you would be better served by something like: func errorsConcat(err1 error, err2 error) error { if (err1) { return err1; } return err2; } And then do: err = errorsConcat(err, f.Close()) In the scenario described in this article, errors.Join() would most often reduce to that (in terms of what Error() string would produce).
  • admax88qqq 2y ago
• zabzonk 2y ago
  how, when and why can close() fail? and what can you do about it if it does?
  • defrost 2y ago
    Fails if (say) OS can't write out pending cache and confirm data written to device. Causes include memory failure, drive cable melted, network cable pulled, etc. What to do? How important is the data being written? Is the only copy of just aquired data from a $10 million day geophysical survey? How much time and resources can you spend on work arounds, multiple copies, alternative storage paths, etc. In aquisition you flush often, worst case lose a minute rather than a day. In, say, seismic quisition, you might aquire audio data from microphone array and multi track raw audio to SEGY tape banks AND split raw data to thermal plotter AND processing WHERE RAW DATA -> (digitally to DAT AND hard drives) and through processing WHERE COOKED DATA -> digital storage. In processing pipelines a failed write() or close() isn't so bad, you flag that it happened and you can try to repipe the raw data to get a savable second result. Ultimately you want human operator control on what and when to do something - it's a hardware problem or resource starvation at the root.
    • russdill 2y ago
      Handling it this way in a user process is insane and essentially cargo culting. If your data is that valuable, you have redundant systems.
      • defrost 2y ago
        Custom hardware, custom real time kernel (acquisition|processing DSP boards) + loadable RT firmware, custom kernel + comms + window manager on main terminal. Other than the recording redundancies described (raw analog logged, raw digital logged, raw paper chart created, cooked data logged, cooked paper chart, (raw | cooked each on tape, disk, paper) what are these "redundant systems" that you speak of? Keeping in mind, of course, that the client has raw data, etc. on the contract as deliverables. Do you imagine two full ships pulling two full microphone arrays to offset a rare (but happens) recoring failure? Now you've doubled the per dium costs and halved the area that can be covered in a typically short season. Do you imagine one ship pulling two arrays that magically don't tangle? It doesn't work that way. The goal here, of course, is to do all that as feasibly possible upfront in order to minimise aquisition time on the water and to ensure that all pings | booms | etc and their returns to multiple mic's recorded so the ship doesn't have to do a repass. Expand on your non cargo culting non insane design ideas for 1970-1990s offshore seismic exploration by all means as what you intend isn't clear in your terse comment. Keep in mind your design will need to be moved on and off arbitrary ships and will operate in places like the North Sea, Spratly Islands, etc. and will have to survive the pitch and toss of stormy weather (eg: attention to card fit in bus backbone).
• joeshaw 2y ago
  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."
• corytheboyd 2y ago
  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?
  • Woansdei 2y ago
    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.
    • corytheboyd 2y ago
      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?
      • deergomoo 2y ago
        In this case the issue is that defer is a very good way to ensure you don’t forget to close the file in any branches, but a bad way to return values (you have to set the value of a named return variable, which is one of Go’s odder features). > Should you have a linter catching these things? JetBrains’ GoLand will in fact warn you of this. If the error truly is immaterial you can instead do defer func() { _ = f.Close() }() which is verbose but explicit in its intent to ignore the error.
        • corytheboyd 2y ago
          > […] you have to set the value of a named return variable Ahhhh okay I see it now. I definitely prefer to not use that feature as well, and I’m surprised it’s even there given how well the rest of the language adheres to “only one way to do things”. Doubly agree that it’s a strange “hack” for forwarding the deferred return value… oof > JetBrains’ GoLand will in fact warn you of this Heh yeah that’s what prompted me to ask, as I noticed (and very much appreciated) these hints. 100% agree with the verbose-but-explicit example you gave, and do that myself.
• yyyfb 2y ago
  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.
  • erikaww 2y ago
    Funny thing is that there is a near footgun with this go: if you defer and set a non named return in a defer, like cErr, that won’t actually set that variable. Not sure what actually happens in that case but godbolt would tell you. In that case, the error would get swallowed
  • account42 2y ago
    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.
    • cflewis 2y ago
      Agreed. Just log it and move on. The code _probably_ wrote what it needed to even if it didn't close. If truly cared that you got everything out correctly, you'd need to do more work than a blind `defer Close()` anyway and you'd never have written the code like this.
    • guappa 2y ago
      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.
  • akira2501 2y ago
    > the simplest tasks The tasks seem simple from 30,000 feet up in the air. Once you get down into the dirt you realize there's absolutely nothing simple about what you're proposing. A filesystem is a giant shared data structure with several contractual requirements and zero guarantees. That people think a programming language could "solve" this is what is boggling to me.
• nextaccountic 2y ago
  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)
  • surajrmal 2y ago
    I actually really like this idea. Is there somewhere I can read more about it?
  • masklinn 2y ago
    > 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. Is it though? It ensures the fd is closed which is what you want, and if you have some form of unwinding in the language you can't really ask for more. And aborts are, if anything, worse. It also works perfectly well for reading, there's no value to close errors then. > Closing a file can raise errors but you can't reasonably treat errors on drop. It's mostly useless anyway, since close does not guarantee that the data has been durably saved. If you want to know that, you need to sync the file, and in that case errors on close are mostly a waste of time: - if you've opened the file for reading you don't care (errors are not actionable, since you can't retry closing on error) - if you've flushed a write, you don't care (for the same reason as above) The one case where it matters is if you care but missed it, in which case we'd need a bunch of different things: - a version of must_use for implicit drops - a consuming flush-and-close method on writeable files - a separate type for readable and writeable files in order to hook both, and a suite of functions to convert back and forth because even if rust had the subtyping for you don't want to move from write to read without either flushing or explicitly opting out of it as you're moving into a "implicit drop is normal" regime > 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 That is nonsensical, the entire point of drop is that it's a hook into default / implicit behaviour. How do you "handle errors" when a drop is called during a panic? It also doesn't make sense from the simple consideration that you can get drops in completely drop-unaware code. Consuming methods is what you're looking for.
• almostdeadguy 2y ago
  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=false&query=defer&sort=byPopularity&type=story https://hn.algolia.com/?dateRange=pastWeek&page=0&prefix=fal...
• russdill 2y ago
  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.html https://docs.kernel.org/admin-guide/filesystem-monitoring.ht...
  • cryptonector 2y ago
    Please no. Just handle errors from `close()` when you're writing files. > On modern hardware if a disk is throwing an io error on write you are having a bad day. And how would you know you're having a bad day if apps ignore those errors?
    • russdill 2y ago
      I'm arguing that the proper thing to do here is to kill the process along with whatever else is using the block device. Whether you handle the error or immediately or if you allow the error to occur after a defer, you still are almost certainly not handling it properly and are taking a speed hit for your troubles.
      • Too 2y ago
        Absolutely not. Imagine a text editor failing when I hit save. Do you really want that to crash the application? No. You want a fallback to ctrl+A ctrl+C, paste into a email and send to yourself. Giving the user a choice to save on a different drive is also a possibility, maybe your thumb drive just got a nudge and lost connection for a second. This is really all normal use cases under normal conditions.
      • cryptonector 2y ago
        Using signals for this really sucks. Anyways, you don't get to do this now because of backwards-compatibility. Just handle the error.
• cryptonector 2y ago
  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.
• meling 2y ago
  The following from the first update: if err := f.Close(); err != nil { return err } return nil Is equivalent to return f.Close()
• w10-1 2y ago
  Java's Closeable.close() has declared multiple invocations as safe for 20 years, when it first became an interface.
• jrockway 2y ago
  I defer these AND check the errors. https://pkg.go.dev/go.uber.org/multierr#hdr-Deferred_Functions 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/internal/errors/multi.go#L26 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.
  • randomdata 2y ago
    With your package, how do you suggest users of your function handle the error upstream? Like this? switch { case strings.Contains(err.Error(), "close thing foo"): // deal with foo close error case strings.Contains(err.Error(), "close thing bar"): // deal with bar close error } And if so, what lead you or your organization to prefer "stringly-typed" errors over the idiomatic approach?
    • TheDong 2y ago
      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 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/45207 / https://github.com/golang/go/issues/39259 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 minefield of half-exported errors. The go authors say to use 'errors.Is' and 'errors.As', but the go stdlib also defines an idiom, and the idiom it defines is that somewhere around 30% of all errors should be stringly typed, including many where you may want to have specific handling for them.
• sedatk 2y ago
  Never thought that `Close` could fail, as `CloseHandle` on Windows never fails. I wonder how .NET behaves in the same scenario on Linux.
• slaymaker1907 2y ago
  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.