fix client deadlock on server death during writes

eikenb · eikenb · commit 49488377fa2f · 2018-03-15T13:53:30.000-07:00
Fixes pkg#234 Similar to pkg#181 (which was about read methods), the client deadlocks on server drop on write methods (Write/ReadFrom). This is another case of broadcastErr() and dispatchRequest() deadlocking. The fix is to bump up the channel used to communicate about inflight packets to maxConcurrentRequests+1 (+1 is new). It seems that it can have the full set of packets going and hit the error (triggering broadcastErr) yet it still tries to call dispatchRequest again. More details in that ticket. I'm also bumping up the chan buffer in the read methods to keep them consistent. I can't reproduce the problem, but it looks like it should have it. So it might just be a much harder race to trigger. This also includes 2 tests which reprocuded the issue for ReadFrom and Write.
diff --git a/client.go b/client.go
@@ -726,7 +726,7 @@ func (f *File) Read(b []byte) (int, error) {
 	offset := f.offset
 	// maxConcurrentRequests buffer to deal with broadcastErr() floods
 	// also must have a buffer of max value of (desiredInFlight - inFlight)
-	ch := make(chan result, maxConcurrentRequests)
+	ch := make(chan result, maxConcurrentRequests+1)
 	type inflightRead struct {
 		b      []byte
 		offset uint64
@@ -822,7 +822,7 @@ func (f *File) WriteTo(w io.Writer) (int64, error) {
 	writeOffset := offset
 	fileSize := uint64(fi.Size())
 	// see comment on same line in Read() above
-	ch := make(chan result, maxConcurrentRequests)
+	ch := make(chan result, maxConcurrentRequests+1)
 	type inflightRead struct {
 		b      []byte
 		offset uint64
@@ -965,7 +965,7 @@ func (f *File) Write(b []byte) (int, error) {
 	desiredInFlight := 1
 	offset := f.offset
 	// see comment on same line in Read() above
-	ch := make(chan result, maxConcurrentRequests)
+	ch := make(chan result, maxConcurrentRequests+1)
 	var firstErr error
 	written := len(b)
 	for len(b) > 0 || inFlight > 0 {
@@ -1026,7 +1026,7 @@ func (f *File) ReadFrom(r io.Reader) (int64, error) {
 	desiredInFlight := 1
 	offset := f.offset
 	// see comment on same line in Read() above
-	ch := make(chan result, maxConcurrentRequests)
+	ch := make(chan result, maxConcurrentRequests+1)
 	var firstErr error
 	read := int64(0)
 	b := make([]byte, f.c.maxPacket)
diff --git a/client_integration_test.go b/client_integration_test.go
@@ -1761,6 +1761,68 @@ func TestServerRoughDisconnect2(t *testing.T) {
 	}
 }
 
+// sftp/issue/234 - abrupt shutdown during ReadFrom hangs client
+func TestServerRoughDisconnect3(t *testing.T) {
+	if *testServerImpl {
+		t.Skipf("skipping with -testserver")
+	}
+	sftp, cmd := testClient(t, READWRITE, NO_DELAY)
+	defer cmd.Wait()
+	defer sftp.Close()
+
+	rf, err := sftp.OpenFile("/dev/null", os.O_RDWR)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer rf.Close()
+	lf, err := os.Open("/dev/zero")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer lf.Close()
+	go func() {
+		time.Sleep(10 * time.Millisecond)
+		cmd.Process.Kill()
+	}()
+
+	io.Copy(rf, lf)
+}
+
+// sftp/issue/234 - also affected Write
+func TestServerRoughDisconnect4(t *testing.T) {
+	if *testServerImpl {
+		t.Skipf("skipping with -testserver")
+	}
+	sftp, cmd := testClient(t, READWRITE, NO_DELAY)
+	defer cmd.Wait()
+	defer sftp.Close()
+
+	rf, err := sftp.OpenFile("/dev/null", os.O_RDWR)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer rf.Close()
+	lf, err := os.Open("/dev/zero")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer lf.Close()
+	go func() {
+		time.Sleep(10 * time.Millisecond)
+		cmd.Process.Kill()
+	}()
+	b := make([]byte, 32768*200)
+	lf.Read(b)
+	for {
+		_, err = rf.Write(b)
+		if err != nil {
+			break
+		}
+	}
+
+	io.Copy(rf, lf)
+}
+
 // sftp/issue/26 writing to a read only file caused client to loop.
 func TestClientWriteToROFile(t *testing.T) {
 	sftp, cmd := testClient(t, READWRITE, NO_DELAY)
