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."
Don't defer Close() on writable files (2017)
161–170 of 311 posts
Re: Don't defer Close() on writable files (2017)
#162how, when and why can close() fail? and what can you do about it if it does?
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.
Re: Don't defer Close() on writable files (2017)
#163The 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
There was [1].
1 - https://go.googlesource.com/proposal/+/master/design/go2draf...
Re: Don't defer Close() on writable files (2017)
#164The 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
Wouldn't you use slog if you want logging friendly?
Re: Don't defer Close() on writable files (2017)
#165Can 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.
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
Re: Don't defer Close() on writable files (2017)
#166Earlier quoted context omitted.
The whole point of this post is that an error returned from file.Close DOES matter
I think from a Go point of view, the lesson to be drawn from that is "don't defer a function call if you need to check its error value", rather than "defer needs to support checking of function return values". In the example at hand, it really makes more sense to call Close() as soon as possible after the file is written. It's more of an issue with the underlying OS file API making error checking difficult. In 99% of…
[deleted]
Re: Don't defer Close() on writable files (2017)
#167TL;DR - defer Close ignores the error from Close, so don't.
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.Re: Don't defer Close() on writable files (2017)
#168Earlier 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.
I have found a blog post from 2009 about this issue[1]. I think the "recent Ubuntu bug [that] has gotten slashdotted" is [2], so I think the most technical discussion about this would be [2].
I also found an interesting "scar tissue" from that bug in the current ext4 docs[0]:
"If auto_da_alloc is enabled, ext4 will detect the replace-via-rename and replace-via-truncate patterns and force that any delayed allocation blocks are allocated such that at the next journal commit, in the default data=ordered mode, the data blocks of the new file are forced to disk before the rename() operation is committed. This provides roughly the same level of guarantees as ext3, and avoids the “zero-length” problem that can happen when a system crashes before the delayed allocation blocks are forced to disk."
[0]https://docs.kernel.org/admin-guide/ext4.html
[1] https://thunk.org/tytso/blog/2009/03/12/delayed-allocation-a...
[2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/317781
Re: Don't defer Close() on writable files (2017)
#169The 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…
[deleted]
Re: Don't defer Close() on writable files (2017)
#170The 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
It's a trivial function: 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;
}
In the scenario described in this article, errors.Join() would most often reduce to that (in terms of what Error() string would produce).