Don't defer Close() on writable files

[-]

Pesthuf@reddit

Every article I read makes me appreciate everything sqlite does for me more.

[-]

cat_in_the_wall@reddit

i used sqlite for a little bit of bookkeeping. this bookkeeping was simple, but critical. the "db" is probably 4k at most.

there was a tad of an argument about sqlite being overkill, and i just asked them how to hand weird error cases like i/o problems (which we had seen from time to time). they had some ideas that wound up pretty complicated, and eventually i interrupted with "or we just use sqlite and we never worry about that stuff ever again". i love sqlite.

[-]

_shulhan@reddit

Here is alternative flow for handling error (on mobile, sorry for plain formatting),

    f, err := os.Open(...)
    ...
    err = f.Write()
    if err != nil {
        goto fail
    }
    err= otherOperation()
    ...
fail:
    errClose := f.Close()
    return errors.Join(err, errClose)

This "goto x" pattern is quite common in C languange.

[-]

LIGHTNINGBOLT23@reddit

Dijkstra made sure that goto could never be used without strenuous justifications.

[-]

Uristqwerty@reddit

The goto of that era was globally scoped. C's goto is function-scope only, so it could even be weakly considered structured control flow itself!

[-]

tsimionescu@reddit

I think this goto fail pattern is very popular in C even today.

[-]

LIGHTNINGBOLT23@reddit

Anecdotally, I see it used more by embedded developers when writing C code than anyone else writing C. I think it's a popular style in the Linux kernel codebase as well. It's usually cleaner than block repetition or temporary macros to cover them up.

[-]

Tersphinct@reddit

That's kinda like C++ exceptions, right?

[-]

beephod_zabblebrox@reddit

not really, exceptions can traverse between function calls while goto can't

[-]

elrata_@reddit

This really asks for defer func() {if retErr != nil ... }()

[-]

Southern-Reveal5111@reddit

The PR reviewers and the smart kid who became an expert yesterday would like to have a meeting with you about the code quality.

[-]

knome@reddit

This is kind of right, but also mostly wrong and not something to really care about.

There's not a huge benefit to ever checking close.

Errors can happen after you close, so it's not actually telling you if there was a problem writing the file, just if there was a problem yet. And if you already fsync'd, it doesn't matter what close as to say. Your file was already written. Whatever caused close to get upset isn't relevant to your concerns.

I see the author touched on this in their second update, where they note the safest pattern to be deferring an unchecked close while returning the result of fsync'ing.

Related, if you're in C or otherwise using linux syscalls directly, NEVER RETRY CLOSE. close ALWAYS closes the fd, it can just also return a purely advisory error.

if you close again, you're in a race condition to either to get either 'not such fd' or finding some other part of your program opened a new file over the just closed fd and slamming it shut from a different thread. or an eventfd, or epollfd or whatever happened to hit there.

Heisenbugs, ahoy!

[-]

XNormal@reddit

EIO on close should be rare, and when it does happen, there usually isn't much you can do locally to handle it gracefully. Either exit immediately and noisily or send a noisy warning and try to continue. In either case the local code doesn't have much to say about it.

So handle it centrally. Write a wrapper for the file class that handles EIO in one of the aforementioned ways, use it like the original file object and defer away your Closes.

hard to handle gracefully. So unless you are writing a specialized application like a database, you want to

[-]

edgmnt_net@reddit

The caller could abort the operation, clean up intermediate resources and return an error. Some code above may decide to report the error to the user mentioning what really failed. I don't see much point trying to avoid normal error handling here, it just makes the whole thing more brittle. Lack of a deferred close is a minor issue in comparison, that's all local code and stays together.

[-]

chucker23n@reddit

deferring a function call ignores its return value, and the Close() method can return errors.

Why is Go so terrible about ~~everything~~ error handling?

[-]

Brilliant-Sky2969@reddit

Rust also ignore errors on finaliser, most languages do.

[-]

chucker23n@reddit

I wouldn't describe defer as a finalizer (nor as a destructor), though.

[-]

TonTinTon@reddit

Agree in general, although in this case you can defer a lambda that closes and checks err in only 4 lines of code

[-]

edgmnt_net@reddit

It might not be obvious, but pretty much any application has to use fsync in some form or another to avoid stuffing garbage into writable files in case of a system crash. Many times you also need to use atomic renames to replace files without losing old data. Some filesystems make this less common but at some level some kind of syncing still has to happen. So it's not really optional.

[-]

robvdl@reddit

Yeah I noticed Goland warngs about doing defer f.Close() "hey, you're ignoring the returned error".

[-]

guest271314@reddit

That applies to WICG File System Access in Chromium-based browsers, too. Close the FileSystemWritableFileStream or you might wind up with orphan .crswap files.