]> Cypherpunks repositories - gostls13.git/commitdiff
net, os: don't wait for Close in blocking mode
authorIan Lance Taylor <iant@golang.org>
Wed, 13 Dec 2017 14:32:06 +0000 (06:32 -0800)
committerIan Lance Taylor <iant@golang.org>
Thu, 14 Dec 2017 02:02:10 +0000 (02:02 +0000)
Updates #7970
Updates #21856
Updates #23111

Change-Id: I0cd0151fcca740c40c3c976f941b04e98e67b0bf
Reviewed-on: https://go-review.googlesource.com/83715
Reviewed-by: Russ Cox <rsc@golang.org>
src/internal/poll/fd_unix.go
src/net/fd_unix.go
src/os/file_unix.go
src/os/pipe_test.go

index 7d95c8d68f8977bcaa10660aeb4727af98fb8e98..2bbf60216f24dd62dc62f23fdd6f5ba122a73e78 100644 (file)
@@ -40,6 +40,9 @@ type FD struct {
 
        // Whether this is a file rather than a network socket.
        isFile bool
+
+       // Whether this file has been set to blocking mode.
+       isBlocking bool
 }
 
 // Init initializes the FD. The Sysfd field should already be set.
@@ -76,18 +79,26 @@ func (fd *FD) Close() error {
        if !fd.fdmu.increfAndClose() {
                return errClosing(fd.isFile)
        }
+
        // Unblock any I/O.  Once it all unblocks and returns,
        // so that it cannot be referring to fd.sysfd anymore,
        // the final decref will close fd.sysfd. This should happen
        // fairly quickly, since all the I/O is non-blocking, and any
        // attempts to block in the pollDesc will return errClosing(fd.isFile).
        fd.pd.evict()
+
        // The call to decref will call destroy if there are no other
        // references.
        err := fd.decref()
+
        // Wait until the descriptor is closed. If this was the only
-       // reference, it is already closed.
-       runtime_Semacquire(&fd.csema)
+       // reference, it is already closed. Only wait if the file has
+       // not been set to blocking mode, as otherwise any current I/O
+       // may be blocking, and that would block the Close.
+       if !fd.isBlocking {
+               runtime_Semacquire(&fd.csema)
+       }
+
        return err
 }
 
@@ -100,6 +111,16 @@ func (fd *FD) Shutdown(how int) error {
        return syscall.Shutdown(fd.Sysfd, how)
 }
 
+// SetBlocking puts the file into blocking mode.
+func (fd *FD) SetBlocking() error {
+       if err := fd.incref(); err != nil {
+               return err
+       }
+       defer fd.decref()
+       fd.isBlocking = true
+       return syscall.SetNonblock(fd.Sysfd, false)
+}
+
 // Darwin and FreeBSD can't read or write 2GB+ files at a time,
 // even on 64-bit systems.
 // The same is true of socket implementations on many systems.
index 352010c17d31c34c78500f26a787ae04b4861ba4..87486578af46d71800f9500f3c0302ab25a2d251 100644 (file)
@@ -313,7 +313,7 @@ func (fd *netFD) dup() (f *os.File, err error) {
        // This also puts the old fd into blocking mode, meaning that
        // I/O will block the thread instead of letting us use the epoll server.
        // Everything will still work, just with more threads.
-       if err = syscall.SetNonblock(ns, false); err != nil {
+       if err = fd.pfd.SetBlocking(); err != nil {
                return nil, os.NewSyscallError("setnonblock", err)
        }
 
index 39171cdc3514d2352b1d81edc10bc6be9060a33e..8c95f49daee32562180f7f76339660252a86bf59 100644 (file)
@@ -66,7 +66,7 @@ func (f *File) Fd() uintptr {
        // opened in blocking mode. The File will continue to work,
        // but any blocking operation will tie up a thread.
        if f.nonblock {
-               syscall.SetNonblock(f.pfd.Sysfd, false)
+               f.pfd.SetBlocking()
        }
 
        return uintptr(f.pfd.Sysfd)
index 9d79d84575d40e8c2894ab535fb5b1f5afc95c33..50e11f1de5ae44a95a755a582c438b0d0fc3d384 100644 (file)
@@ -17,6 +17,7 @@ import (
        "runtime"
        "strconv"
        "strings"
+       "sync"
        "syscall"
        "testing"
        "time"
@@ -220,3 +221,70 @@ func TestReadNonblockingFd(t *testing.T) {
                t.Errorf("child process failed: %v", err)
        }
 }
+
+// Test that we don't let a blocking read prevent a close.
+func TestCloseWithBlockingRead(t *testing.T) {
+       r, w, err := os.Pipe()
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer r.Close()
+       defer w.Close()
+
+       c1, c2 := make(chan bool), make(chan bool)
+       var wg sync.WaitGroup
+
+       wg.Add(1)
+       go func(c chan bool) {
+               defer wg.Done()
+               // Give the other 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 {
+                       t.Error(err)
+               }
+               close(c)
+       }(c1)
+
+       // Calling Fd will put the file into blocking mode.
+       _ = r.Fd()
+
+       wg.Add(1)
+       go func(c chan bool) {
+               defer wg.Done()
+               var b [1]byte
+               _, err = r.Read(b[:])
+               close(c)
+               if err == nil {
+                       t.Error("I/O on closed pipe unexpectedly succeeded")
+               }
+       }(c2)
+
+       for c1 != nil || c2 != nil {
+               select {
+               case <-c1:
+                       c1 = nil
+                       // r.Close has completed, but the blocking Read
+                       // is hanging. Close the writer to unblock it.
+                       w.Close()
+               case <-c2:
+                       c2 = nil
+               case <-time.After(1 * time.Second):
+                       switch {
+                       case c1 != nil && c2 != nil:
+                               t.Error("timed out waiting for Read and Close")
+                               w.Close()
+                       case c1 != nil:
+                               t.Error("timed out waiting for Close")
+                       case c2 != nil:
+                               t.Error("timed out waiting for Read")
+                       default:
+                               t.Error("impossible case")
+                       }
+               }
+       }
+
+       wg.Wait()
+}