Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
arnottcr opened this issue Jul 29, 2020 · 2 comments
Open

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

arnottcr opened this issue Jul 29, 2020 · 2 comments
Labels
Projects
Milestone

Comments

@arnottcr
Copy link

@arnottcr arnottcr commented Jul 29, 2020

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

@arnottcr arnottcr changed the title io.Closer error is canonically ignored proposal: errors: add Close method for defer io.Closer Jul 29, 2020
@gopherbot gopherbot added this to the Proposal milestone Jul 29, 2020
@gopherbot gopherbot added the Proposal label Jul 29, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jul 30, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 30, 2020

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.

@arnottcr
Copy link
Author

@arnottcr arnottcr commented Jul 30, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.