Fixes #181: client deadlock on server death
The client was deadlocking if you killed the server when a file transfer
was occuring. The problem occured when you had many requests sent and
were waiting on responses when the server dies. The errors for all those
pending requests get sent along the response channel which is also used
for the errors reported by the new requests being made. The new request
channel send is in the same loop as the channel reading, so when it
blocked due to all other errors filling the channel it deadlocked the
program.
There were 2 possible fixes, changing the new request error channel
usage to be in a separate goroutine to keep it from blocking the loop or
to increase the size of the buffer to handle all the errors. This
implements the latter.
Included is a test that reproduces the problem most of the time. Due to
the required timing of the issue, it was impossible to reproduce 100% of
the time.
diff --git a/client.go b/client.go
index 375d340..e539fc5 100644
--- a/client.go
+++ b/client.go
@@ -653,7 +653,9 @@
inFlight := 0
desiredInFlight := 1
offset := f.offset
- ch := make(chan result, 2)
+ // maxConcurrentRequests buffer to deal with broadcastErr() floods
+ // also must have a buffer of max value of (desiredInFlight - inFlight)
+ ch := make(chan result, maxConcurrentRequests)
type inflightRead struct {
b []byte
offset uint64
@@ -748,7 +750,8 @@
offset := f.offset
writeOffset := offset
fileSize := uint64(fi.Size())
- ch := make(chan result, 2)
+ // see comment on same line in Read() above
+ ch := make(chan result, maxConcurrentRequests)
type inflightRead struct {
b []byte
offset uint64
@@ -890,8 +893,8 @@
inFlight := 0
desiredInFlight := 1
offset := f.offset
- // chan must have a buffer of max value of (desiredInFlight - inFlight)
- ch := make(chan result, 2)
+ // see comment on same line in Read() above
+ ch := make(chan result, maxConcurrentRequests)
var firstErr error
written := len(b)
for len(b) > 0 || inFlight > 0 {
@@ -951,8 +954,8 @@
inFlight := 0
desiredInFlight := 1
offset := f.offset
- // chan must have a buffer of max value of (desiredInFlight - inFlight)
- ch := make(chan result, 2)
+ // see comment on same line in Read() above
+ ch := make(chan result, maxConcurrentRequests)
var firstErr error
read := int64(0)
b := make([]byte, f.c.maxPacket)
diff --git a/client_integration_test.go b/client_integration_test.go
index 6ba1d37..3bd4ad3 100644
--- a/client_integration_test.go
+++ b/client_integration_test.go
@@ -1711,6 +1711,35 @@
io.Copy(ioutil.Discard, f)
}
+// sftp/issue/181, abrupt server hangup would result in client hangs.
+// due to broadcastErr filling up the request channel
+// this reproduces it about 50% of the time
+func TestServerRoughDisconnect2(t *testing.T) {
+ if *testServerImpl {
+ t.Skipf("skipping with -testserver")
+ }
+ sftp, cmd := testClient(t, READONLY, NO_DELAY)
+ defer cmd.Wait()
+ defer sftp.Close()
+
+ f, err := sftp.Open("/dev/zero")
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer f.Close()
+ b := make([]byte, 32768*100)
+ go func() {
+ time.Sleep(1 * time.Millisecond)
+ cmd.Process.Kill()
+ }()
+ for {
+ _, err = f.Read(b)
+ if err != nil {
+ break
+ }
+ }
+}
+
// 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)
