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

[urandom2]

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
        }
}

further reading

16019

20705

25408

27741

27750

[ianlancetaylor]

I just want to note that there is often no need to check the result of Close of a file opened for reading. If all the data has been read, the program behavior won't change if Close returns an error.

[urandom2]

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.

[nhooyr]

I just want to note that there is often no need to check the result of Close of a file opened for reading. If all the data has been read, the program behavior won't change if Close returns an error.

This is true but makes the linters that check for this like errcheck very noisy as they'll tag the harmless invocations too of which there may now be many.

It may still be useful to log the error as there shouldn't be any error. i.e it may be the result of some insidious bug somewhere else.

So I'd personally be in favour of something like this to make defer error checking less verbose.

And yes you could define such a function in your own code and use it throughout but it does feel like something every Go programmer encounters and as such deserves a spot in the stdlib.

[carnott-snap]

@ianlancetaylor, sorry for the call out, but is there any reason this proposal has not been discussed in the weekly meetings? If it is a capacity problem that is fine, just wanted to make sure this issue did not get dropped.

[ianlancetaylor]

@carnott-snap As far as I can tell it's just a capacity problem. It's in the incoming list at https://github.com/golang/go/projects/1, along with quite a few other proposals that haven't been looked at it yet. Sorry.

[carnott-snap]

Not a problem, thanks for the update, sorry to pester, and great work on generics!

[Dynom]

And yes you could define such a function in your own code and use it throughout but it does feel like something every Go programmer encounters and as such deserves a spot in the stdlib.

I also think it's highly opinionated, in terms of writing stuff to metric of log dashboards in case an err is non-nil. I realise that this will all still be possible with this proposal, but since it's not a one-size-fits-all, I don't think it should be part of the stdlib.

just my 2 cents.

[rsc]

People do write code that does

if err := f.Close(); err != nil {
    return err
}

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

And to be clear,

defer f.Close()

is clearly OK for readers.

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.

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

[carnott-snap]

There are two problems I have with the current state of things. First, it is non-trivial to extract an error from a defer: (as if err != nil { was not bad enough, now we need two more lines for the defer)

defer func() {
    if err := f.Close(); err != nil {
        log.Println(err)
    }
}()

Second, after you extract it, if you want to interact with the error (and not just log it as above), you must use a named return and decide which error is more important:

func WriteAndClose(w io.WriteCloser, a ...interface{}) (err error) {
    defer func() {
        if err0 := w.Close(); err == nil  && err0 != nil{
            err = err0
        }
    }()
    _, err := fmt.Fprint(w, a...)
    return err
}

[carnott-snap]

I also think it's highly opinionated, in terms of writing stuff to metric of log dashboards in case an err is non-nil. I realise that this will all still be possible with this proposal, but since it's not a one-size-fits-all, I don't think it should be part of the stdlib.

I completely agree, and somewhat regret posing this as a proposal, since what I want to resolve is the root problem and there are a bunch of ways to do this. Unfortunately, github issues are not a good place to brainstorm.

[rsc]

@carnott-snap

First, it is non-trivial to extract an error from a defer ... \ Second, ... you must use a named return

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.

[carnott-snap]

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]

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.

[bcmills]

@carnott-snap

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.)

Such an implementation of Close is incorrect, full-stop. The Close method of an io.Closer must not fail to release underlying resources, because it is explicitly not safe for the caller of such a method to retry the call on failure (see #25390).

[carnott-snap]

The next sentence states that Specific implementations may document their own behavior., so that is patently false. If we need to change this wording, I am game, but the use case remains valid.

Also, the idea of needing to respond to an error is orthogonal to calling Close again. I could have an interface where, if err == ErrsSentinel then you need to call Foo.Cleanup.

[bcmills]

@carnott-snap, if an implementation requires behavior that is not safe (or not supported) for io.Closer implementations in general, then that implementation cannot be used as an io.Closer in general. (It fails the Liskov substitution principle.)

In that case, the implementation should take care not to implement the io.Closer interface so that it is not accidentally used as one. So it's a defect in the implementation either way.

[carnott-snap]

Maybe we use the term undefined [behaviour] differently, but based upon the fact that io.Closer states that one can define this behaviour, it does not seem apocryphal or "unsafe". If your opinion is how the docs should be read, imo, it needs to be rewritten. (And use the term "unsafe" and "disallowed").

However, the concept of calling Close is orthogonal to responding to the error. The docs say nothing about the error being meaningless or ignorable. And if this is the case, the signature should be interface{ Close() }.

[rsc]

Based on the discussion above, this proposal seems like a likely decline. — rsc for the proposal review group

[rsc]

No change in consensus, so declined. — rsc for the proposal review group

[urandom2]

@rsc: since there is a need described in this ticket and you consider my proposed solution unacceptable, can you offer alternatives? Currently there is no ergonomic way to interacted with a deferred error (or io.Closer).

[ianlancetaylor]

I don't think there is a single solution to the general issue.

[carnott-snap]

Then can we drive towards multiple solutions for the major issue types? I understand that closing this proposal may be your way of signaling this this one idea is not accepted, but it also suggests that there is no need for any fix.

[ianlancetaylor]

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.