go: proposal: errors: add Close method for defer io.Closer

description

The io.Closer interface presents a Close method that returns an error. Unfortunately the syntax of defer makes it difficult to consume this error:

func do() (err error) {
        var c io.Closer
        // ...
        defer func() {
                if e := c.Close(); err == nil {
                        err = e
                }
        }()
        // ...
}

Either for this reason, or because canonically most io.Closers never give actionable errors, we usually see this:

defer c.Close()

I propose we define a helper function to assist with this:

package errors

func Close(err *error, c io.Closer) {
        if e := c.Close(); err == nil {
                *err = e
        }
}

func do() (err error) {
        var c io.Closer
        // ...
        defer errors.Close(&err, c)
        // ...
}

costs

This would have no costs to the language spec, but would add api surface to the errors package in the standard library. It also requires that the calling function have a named error parameter, which was a caveat in #32437, but this is required for any error handling within a defer, so is more an issue with the language spec. Furthermore, some may find the errors.Close interface non-intuitive, but I think there is a win to having a canonical way to do this.

alternatives

We could accept that Close should be a void method and fix it in Go2. This is going to break a lot of things, and may not be worth the hassle, but it is how the method is usually used. This could also be related to questions about idempotency in io.Closer, see further reading below.

Since this blindly returns the error, and thus bares resemblance to #32437, it may be useful to add a formatter variant. I think that a func variant is overkill since at that point you probably have as much code as the original manual defer.

package errors

func Close(err *error, c io.Closer) { Closef(err, c, "") }

func Closef(err *error, c io.Closer, format string, a ...interface{}) {
        CloseFunc(err, c, func(err error) error {
                return fmt.Errorf(format+": %w", append(a, err)...)
        })
}
func CloseFunc(err *error, c io.Closer, f func(err error) error) {
        if e := c.Close(); err == nil {
                *err = f(e)
        }
}

Technically it is still possible to ignore the result of io.Closer.Close with this implementation, given the function is already are returning an error. We can either pick one, above I select the returned error, as that should have more useful information, but we could say Close is more important, though this means that the last deferred close wins. Alternatively, it may be better to conjoin the errors, though errors.Unwrap does not allow multiple inheritance:

package errors

func Close(err *error, c io.Closer) {
        e := c.Close()
        switch {
        case err != nil && e != nil:
                e = fmt.Errorf("ret: %w, close: %v", err, e)
                fallthrough
        case e != nil:
                *err = e
        }
}

About this issue

Original URL
State: closed
Created 4 years ago
Reactions: 6
Comments: 23 (13 by maintainers)

Most upvoted comments

While that may be true for os.File, io.Closer does not document this, and many implementations do not maintain this invariant. If this viewpoint is prevailing, I think it worth noting in the godoc and pursuing my Go2 suggestion.

For instance, cloud.google.com/go/storage.Writer.Write notes:

// Since writes happen asynchronously, Write may return a nil
// error even though the write failed (or will fail). Always
// use the error returned from Writer.Close to determine if
// the upload was successful.

As such, storage.Writer.Close is the only way to detect errors, and will change program behaviour, something that is confirmed by its godoc:

// Close completes the write operation and flushes any buffered data.

Side note, this is usually a thorn for me because most linters require that io.Closer errors be explicitly handled. It alone is not a reason to make this change, but suggests that the community supports the direction.

urandom2 on Jul 30, 2020

Closing this issue should only suggest that this specific proposal is not accepted.

The issue tracker isn’t a good place to brainstorm a solution for a problem. It doesn’t handle discussions well. I recommend that you use a forum such as golang-nuts.

ianlancetaylor on Feb 4, 2021

but I am not sure I would want to encourage that behavior. Really such code should be wrapping the error with extra context.

2c2
<     return err
---
>     return fmt.Errorf("closing a file: %w", err)

is clearly OK for readers.

This is demonstrably incorrect. Say I have a reader that, like the gcloud sdk, holds open resources until closure, and the call to io.Closer.Close fails, leaving the resources dangling, and causing a memory leak. (This is based upon real xp.) One has no way of knowing what is causing the problem.

For writers you need to check the result, but often if the result is a failure you want to react to the fact that you didn’t actually write the thing you thought you wrote. Hiding the possible error inside a helper (which makes it look like the error is “taken care of”) only compounds the problem.

errors.Close, as proposed, does not necessarily hide the error, but I agree that if something else errors, it could be masked. IMO, this is an argument to allow something like fmt.Errorf("doing a thing: %w and %w", err0, err1) or type Multi []error that makes multi errors trivial to use. Many libraries that parallelise work are force do do this already, and I am happy if adding Close is prerequisite on something like Multi in the stdlib.

It’s just unclear how much the library routine would help versus encourage bad code.

I am more than open to alternatives, this was the best bad option I could come up with.

carnott-snap on Jan 20, 2021

These are both reasons not to use defers for code that might fail and influence error handling. So rather than try to fix these problems by adding defer-use-helpers, don’t use defer here.

Not using defer will make the code look even worse, violate line of sight style, and gets exponentially more complex with each additional error)

func WriteAndClose(w io.WriteCloser, a ...interface{}) error {
    _, err0 := fmt.Fprint(w, a...)
    /* 20 lines of other work, none of which can early return */
    err10 := w.Close()
    if err0 != nil {
        return err0
    }
    /* 9 other errN != nil  checks*/
    return err10
}

carnott-snap on Jan 20, 2021