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

os: codifying behavior of *os.File Close() more than once #20705

Closed
joeshaw opened this issue Jun 16, 2017 · 7 comments
Closed

os: codifying behavior of *os.File Close() more than once #20705

joeshaw opened this issue Jun 16, 2017 · 7 comments

Comments

@joeshaw
Copy link
Contributor

@joeshaw joeshaw commented Jun 16, 2017

(Current as of Go version 1.9 beta 1)

I wrote a blog post recently discussing whether it's appropriate to defer f.Close() on writable files. In discussion that followed afterward, there was the suggestion to both defer f.Close() to ensure that things were cleaned up, but also to explicitly to call f.Close() at the end and check errors there. I'm going to call this pattern the "double-close." It'd look something like this:

func helloWorld() error {
    f, err := os.Create("/tmp/notes.txt")
    if err != nil {
        return err
    }
    defer f.Close()

    if _, err io.WriteString(f, "hello world"); err != nil {
        return err
    }

    return f.Close()
}

In practice this works well, but it's not well-defined in the docs. The docs for io.Closer say, "The behavior of Close after the first call is undefined. Specific implementations may document their own behavior." The docs for *os.File Close() say nothing.

Under the covers, calling Close() first checks to see if the underlying file descriptor is -1 and returns syscall.EINVAL in that case. Otherwise it closes the file descriptor and sets it to -1. This makes it safe (though not idempotent) to call Close multiple times, and since it's deferred the second time, it's fine to discard the syscall.EINVAL error value.

From the discussion that followed my post, it would seem that this is already in fairly wide use in the wild. I would like for the documentation to call this fact out, as it'd suggest that the double-close is a valid practice and unlikely to break in the future.

@slrz

This comment has been minimized.

Copy link

@slrz slrz commented Jun 17, 2017

I don't think this should be encouraged. People will cargo-cult the practice to other io.Closers which might not handle it well.

@joeshaw

This comment has been minimized.

Copy link
Contributor Author

@joeshaw joeshaw commented Jun 26, 2017

My main counterpoint to that is that the io.Closer docs are already explicit about the behavior generally. My hope is that for implementations where calling it more than once is not safe, it would panic(), as doing so would indicate a programmer error. In most cases, though, I'd expect it to do what *os.File() does, which is return an implementation-specific error that can (usually) be safely ignored.

@adam-azarchs

This comment has been minimized.

Copy link
Contributor

@adam-azarchs adam-azarchs commented Jul 19, 2019

The issue is that the alternative to not double-closing a file is either unsafe (ignoring the error on Close, or potentially failing to close the file on some return paths) or verbose to the point where the temptation to take shortcuts becomes too high, e.g.

func helloWorld() error {
    f, err := os.Create("/tmp/notes.txt")
    if err != nil {
        return err
    }
    defer func() {
        if f != nil {
            f.Close()
        }
    }()

    if _, err := io.WriteString(f, "hello world"); err != nil {
        return err
    }
    err = f.Close()
    f = nil
    return err
}

As was pointed out by a coworker, common lisp has a good specification for what happens in this case:

It is permissible to close an already closed stream, but in that case the result is implementation-dependent.

@adam-azarchs

This comment has been minimized.

Copy link
Contributor

@adam-azarchs adam-azarchs commented Jul 19, 2019

To elaborate: there are two kinds of behavior which would result in that double-close pattern being broken:

  1. If the second call to Close would panic.
  2. If the second call to Close might close the same file descriptor again, which might now be pointing at a different file, thus resulting in all kinds of horrible brokenness which languages like Go are supposed to allow us to avoid thinking about.

Documenting that it will return an error rather than causing bugs or a panic seems pretty harmless, especially since, as @joeshaw points out, the documentation on Closer is explicit about the behavior being undefined in general. People who read the documentation for File.Close() at that level of detail can probably be expected to read the documentation for io.Closer as well.

@joeshaw

This comment has been minimized.

Copy link
Contributor Author

@joeshaw joeshaw commented Jul 19, 2019

I don't think we should generalize, this issue is specifically about *os.File.

Calling Close() multiple times on *os.File doesn't panic, and it already prevents the underlying file descriptor number from being closed more than once. It returns syscall.EINVAL on its own. These are all good behaviors! I am advocating that it should be codified in the documentation as behavior that can be relied upon. (And indeed, plenty of code out there already does, as I learned from responses to my blog post.)

@joeshaw

This comment has been minimized.

Copy link
Contributor Author

@joeshaw joeshaw commented Jul 19, 2019

Looks like it was already done in #32427 and https://golang.org/cl/180438. Going to close this then.

@joeshaw joeshaw closed this Jul 19, 2019
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 19, 2019

Thanks for noticing.

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

Successfully merging a pull request may close this issue.

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