os: consistently return ErrClosed for closed file

Catch all the cases where a file operation might return ErrFileClosing,
and convert to ErrClosed. Use a new method for the conversion, which
permits us to remove some KeepAlive calls.

Change-Id: I584178f297efe6cb86f3090b2341091b412f1041
Reviewed-on: https://go-review.googlesource.com/41793
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/os/file.go b/src/os/file.go
index 271197a90ea2adb5333219be8213e5a29e7d25ec..b5a1bb8c0df35a767df2258d2c617b564c870c38 100644 (file)
--- a/src/os/file.go
+++ b/src/os/file.go
@@ -101,17 +101,7 @@ func (f *File) Read(b []byte) (n int, err error) {
                return 0, err
        }
        n, e := f.read(b)
-       if e != nil {
-               if e == poll.ErrFileClosing {
-                       e = ErrClosed
-               }
-               if e == io.EOF {
-                       err = e
-               } else {
-                       err = &PathError{"read", f.name, e}
-               }
-       }
-       return n, err
+       return n, f.wrapErr("read", e)
 }
 
 // ReadAt reads len(b) bytes from the File starting at byte offset off.
@@ -130,11 +120,7 @@ func (f *File) ReadAt(b []byte, off int64) (n int, err error) {
        for len(b) > 0 {
                m, e := f.pread(b, off)
                if e != nil {
-                       if e == io.EOF {
-                               err = e
-                       } else {
-                               err = &PathError{"read", f.name, e}
-                       }
+                       err = f.wrapErr("read", e)
                        break
                }
                n += m
@@ -161,10 +147,7 @@ func (f *File) Write(b []byte) (n int, err error) {
 
        epipecheck(f, e)
 
-       if e != nil {
-               err = &PathError{"write", f.name, e}
-       }
-       return n, err
+       return n, f.wrapErr("write", e)
 }
 
 // WriteAt writes len(b) bytes to the File starting at byte offset off.
@@ -182,7 +165,7 @@ func (f *File) WriteAt(b []byte, off int64) (n int, err error) {
        for len(b) > 0 {
                m, e := f.pwrite(b, off)
                if e != nil {
-                       err = &PathError{"write", f.name, e}
+                       err = f.wrapErr("write", e)
                        break
                }
                n += m
@@ -206,7 +189,7 @@ func (f *File) Seek(offset int64, whence int) (ret int64, err error) {
                e = syscall.EISDIR
        }
        if e != nil {
-               return 0, &PathError{"seek", f.name, e}
+               return 0, f.wrapErr("seek", e)
        }
        return r, nil
 }
@@ -279,3 +262,16 @@ func fixCount(n int, err error) (int, error) {
        }
        return n, err
 }
+
+// wrapErr wraps an error that occurred during an operation on an open file.
+// It passes io.EOF through unchanged, otherwise converts
+// poll.ErrFileClosing to ErrClosed and wraps the error in a PathError.
+func (f *File) wrapErr(op string, err error) error {
+       if err == nil || err == io.EOF {
+               return err
+       }
+       if err == poll.ErrFileClosing {
+               err = ErrClosed
+       }
+       return &PathError{op, f.name, err}
+}

diff --git a/src/os/file_posix.go b/src/os/file_posix.go
index 98c87ee4cdbd2794f9115a4c915ea704184223c8..6ee7eeb2dad5f924c8ce6a79cb4d537f43bf007b 100644 (file)
--- a/src/os/file_posix.go
+++ b/src/os/file_posix.go
@@ -7,7 +7,6 @@
 package os
 
 import (
-       "runtime"
        "syscall"
        "time"
 )
@@ -62,9 +61,8 @@ func (f *File) Chmod(mode FileMode) error {
                return err
        }
        if e := f.pfd.Fchmod(syscallMode(mode)); e != nil {
-               return &PathError{"chmod", f.name, e}
+               return f.wrapErr("chmod", e)
        }
-       runtime.KeepAlive(f)
        return nil
 }
 
@@ -95,9 +93,8 @@ func (f *File) Chown(uid, gid int) error {
                return err
        }
        if e := f.pfd.Fchown(uid, gid); e != nil {
-               return &PathError{"chown", f.name, e}
+               return f.wrapErr("chown", e)
        }
-       runtime.KeepAlive(f)
        return nil
 }
 
@@ -109,9 +106,8 @@ func (f *File) Truncate(size int64) error {
                return err
        }
        if e := f.pfd.Ftruncate(size); e != nil {
-               return &PathError{"truncate", f.name, e}
+               return f.wrapErr("truncate", e)
        }
-       runtime.KeepAlive(f)
        return nil
 }
 
@@ -123,9 +119,8 @@ func (f *File) Sync() error {
                return err
        }
        if e := f.pfd.Fsync(); e != nil {
-               return &PathError{"sync", f.name, e}
+               return f.wrapErr("sync", e)
        }
-       runtime.KeepAlive(f)
        return nil
 }
 
@@ -153,9 +148,8 @@ func (f *File) Chdir() error {
                return err
        }
        if e := f.pfd.Fchdir(); e != nil {
-               return &PathError{"chdir", f.name, e}
+               return f.wrapErr("chdir", e)
        }
-       runtime.KeepAlive(f)
        return nil
 }
 

diff --git a/src/os/file_unix.go b/src/os/file_unix.go
index 847316492b34f6cf16f630b570ce94dff3f6bd51..c65cfb6d3711044ea16321307902e5856a3e4650 100644 (file)
--- a/src/os/file_unix.go
+++ b/src/os/file_unix.go
@@ -186,6 +186,9 @@ func (file *file) close() error {
        }
        var err error
        if e := file.pfd.Close(); e != nil {
+               if e == poll.ErrFileClosing {
+                       e = ErrClosed
+               }
                err = &PathError{"close", file.name, e}
        }
 

diff --git a/src/os/file_windows.go b/src/os/file_windows.go
index a6cdb3ff478d31efe3be919f354e849af4507ddb..c5b83b5dfeae3504011097bb71349f1ea9f6a9fe 100644 (file)
--- a/src/os/file_windows.go
+++ b/src/os/file_windows.go
@@ -188,6 +188,9 @@ func (file *file) close() error {
        }
        var err error
        if e := file.pfd.Close(); e != nil {
+               if e == poll.ErrFileClosing {
+                       e = ErrClosed
+               }
                err = &PathError{"close", file.name, e}
        }
 

diff --git a/src/os/os_test.go b/src/os/os_test.go
index c0c8875363a40b1a4301318448e8ce6f4e7d84ce..8e2cd14ddf1b48c29c6b94c3fbf01a227971bcfb 100644 (file)
--- a/src/os/os_test.go
+++ b/src/os/os_test.go
@@ -2178,3 +2178,23 @@ func TestPipeThreads(t *testing.T) {
                }
        }
 }
+
+func TestDoubleCloseError(t *testing.T) {
+       path := sfdir + "/" + sfname
+       file, err := Open(path)
+       if err != nil {
+               t.Fatal(err)
+       }
+       if err := file.Close(); err != nil {
+               t.Fatalf("unexpected error from Close: %v", err)
+       }
+       if err := file.Close(); err == nil {
+               t.Error("second Close did not fail")
+       } else if pe, ok := err.(*PathError); !ok {
+               t.Errorf("second Close returned unexpected error type %T; expected os.PathError", pe)
+       } else if pe.Err != ErrClosed {
+               t.Errorf("second Close returned %q, wanted %q", err, ErrClosed)
+       } else {
+               t.Logf("second close returned expected error %q", err)
+       }
+}

diff --git a/src/os/pipe_test.go b/src/os/pipe_test.go
index a7bd41ff40c2221a426b09e2ec1524f2a03fb3ad..eb26b68f85e32c9f42defa4d43fd0fa529ebad7e 100644 (file)
--- a/src/os/pipe_test.go
+++ b/src/os/pipe_test.go
@@ -114,7 +114,7 @@ func TestStdPipeHelper(t *testing.T) {
        os.Exit(0)
 }
 
-func TestClosedPipeRace(t *testing.T) {
+func testClosedPipeRace(t *testing.T, read bool) {
        switch runtime.GOOS {
        case "freebsd":
                t.Skip("FreeBSD does not use the poller; issue 19093")
@@ -128,25 +128,46 @@ func TestClosedPipeRace(t *testing.T) {
        defer w.Close()
 
        // Close the read end of the pipe in a goroutine while we are
-       // writing to the write end.
+       // writing to the write end, or vice-versa.
        go func() {
-               // Give the main goroutine a chance to enter the Read call.
-               // This is sloppy but the test will pass even if we close
-               // before the read.
+               // Give the main goroutine a chance to enter the Read or
+               // Write call. This is sloppy but the test will pass even
+               // if we close before the read/write.
                time.Sleep(20 * time.Millisecond)
 
-               if err := r.Close(); err != nil {
+               var err error
+               if read {
+                       err = r.Close()
+               } else {
+                       err = w.Close()
+               }
+               if err != nil {
                        t.Error(err)
                }
        }()
 
-       if _, err := r.Read(make([]byte, 1)); err == nil {
-               t.Error("Read of closed pipe unexpectedly succeeded")
+       // A slice larger than PIPE_BUF.
+       var b [65537]byte
+       if read {
+               _, err = r.Read(b[:])
+       } else {
+               _, err = w.Write(b[:])
+       }
+       if err == nil {
+               t.Error("I/O on closed pipe unexpectedly succeeded")
        } else if pe, ok := err.(*os.PathError); !ok {
-               t.Errorf("Read of closed pipe returned unexpected error type %T; expected os.PathError", pe)
+               t.Errorf("I/O on closed pipe returned unexpected error type %T; expected os.PathError", pe)
        } else if pe.Err != os.ErrClosed {
                t.Errorf("got error %q but expected %q", pe.Err, os.ErrClosed)
        } else {
-               t.Logf("Read returned expected error %q", err)
+               t.Logf("I/O returned expected error %q", err)
        }
 }
+
+func TestClosedPipeRaceRead(t *testing.T) {
+       testClosedPipeRace(t, true)
+}
+
+func TestClosedPipeRaceWrite(t *testing.T) {
+       testClosedPipeRace(t, false)
+}