It is possible for dispatchRequest and broadcastErr to both send a
result down the inflight[] channel which has a buffer of 1, so would
work for one of them then block on the other. If broadcastErr went
first, then dispatchRequest would block causing the client to hang.
After commit 506f3a7 which removed clientConn mutex around
conn.sendPacket, it is now possible for conn.Close and conn.Write to
race (https://travis-ci.org/pkg/sftp/jobs/219048782#L1838). The problem
is really hard to reproduce, but I believe this patch fixes it.
Now clientConn.Mutex protects 'inflight' map only, and conn.Mutex
serializes connection writes/close.
Signed-off-by: Pavel Borzenkov <pavel.borzenkov@gmail.com>
Client + Server might deadlock while being connected via io.Pipe with
high request concurrency. The scenario is as follows:
- Server is trying to send reply to a previous client's packet and is
waiting on Pipe's condvar for the client to read it:
3 0x0000000000460e39 in sync.(*Cond).Wait
at /usr/lib/go/src/sync/cond.go:57
4 0x0000000000464519 in io.(*pipe).write
at /usr/lib/go/src/io/pipe.go:90
5 0x0000000000464a4c in io.(*PipeWriter).Write
at /usr/lib/go/src/io/pipe.go:157
6 0x00000000007a0b66 in go.(*struct { io.Reader; io.WriteCloser }).Write
at <autogenerated>:246
7 0x0000000000790cf6 in github.com/pkg/sftp.(*conn).Write
at <autogenerated>:3
8 0x000000000073a68d in github.com/pkg/sftp.sendPacket
at ./packet.go:130
9 0x000000000073338c in github.com/pkg/sftp.(*conn).sendPacket
at ./conn.go:31
10 0x00000000007374b4 in github.com/pkg/sftp.(*packetManager).maybeSendPackets
at ./packet-manager.go:87
- Client is sending a new packet and is waiting on Pipe's condvar for
the server to read it, while holding clientConn mutex:
5 0x0000000000464a4c in io.(*PipeWriter).Write
at /usr/lib/go/src/io/pipe.go:157
6 0x0000000000790cf6 in github.com/pkg/sftp.(*conn).Write
at <autogenerated>:3
7 0x000000000073a68d in github.com/pkg/sftp.sendPacket
at ./packet.go:130
8 0x000000000073338c in github.com/pkg/sftp.(*conn).sendPacket
at ./conn.go:31
9 0x0000000000733bfb in github.com/pkg/sftp.(*clientConn).dispatchRequest
at ./conn.go:105
10 0x0000000000733a0b in github.com/pkg/sftp.(*clientConn).sendPacket
at ./conn.go:97
- Client's receive loop is processing a reply from the server and is
trying to acquire clientConn mutex:
4 0x000000000046108d in sync.(*Mutex).Lock
at /usr/lib/go/src/sync/mutex.go:87
5 0x00000000007336cb in github.com/pkg/sftp.(*clientConn).recv
at ./conn.go:69
6 0x000000000073350d in github.com/pkg/sftp.(*clientConn).loop
at ./conn.go:49
7 0x000000000045ab51 in runtime.goexit
at /usr/lib/go/src/runtime/asm_amd64.s:2197
In this scenario neither server nor client can progress. Fix this by
releasing clienConn mutex *before* trying to send the packet.
Signed-off-by: Pavel Borzenkov <pavel.borzenkov@gmail.com>
github issue #145
A deadlock will occur in cases when using client.Write or client.ReadFrom,
it has 2 network issues in a row, and it is starting a new loop to send
more concurrent packets.
Reading and Writing are less coupled in the sftp client and server than
they may be over a traditional ReadWriter. This mirrors the full duplex
/ half close nature of a TCP connection.
In essence conn.Reader cannot be closed locally, it can only close in
response to the remote side using conn.WriteCloser.Close(), or a network
error, which is effectively the same thing.
Make this behaviour super clear by no longer smooshing the type into a
ReadWriteCloser.
Taizeng Wu
John Eikenberry
Pavel Borzenkov
Dave Cheney