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.
benchmarks were broken due to double closing of a channel in some tests.
I just commented out the problematic closes which probably means it's
leaking in the tests but at least they run now. Should probably try to
figure out what happened here at some point.
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.
netPipe provides a pair of io.ReadWriteClosers connected to each other.
The functions is identical to os.Pipe with the exception that netPipe
provides the Read/Close guarentees that os.File derrived pipes do not.
- no need to initalise mutexes, their zero value is valid
- make NewServer simpler, 90% of use cases already had an
io.ReadWriteCloser, in the single case that it it is easy to provide a
simple wrapper.
Fixes#69
io.Pipe is unbuffered and provides a poor replacement for a net.Conn, use an os.Pipe instead.
Also skip some tests which don't work under the Go server as it runs in process.
Previously recvPacket would be invoked in several goroutines. This meant
that when multiple concurrent requests were in flight there were N
goroutines each waiting on recvPacket. For optimal throughput the goal
is to send a new request as quickly as possible once a response is
received. The previous mechanism worked counter to this because the
goroutine sending new requests would be competing against N recvPacket
goroutines that may become runnable as data streams in. Having a single
goroutine responsible for recvPacket means that the recv and send
goroutines will ping-pong back and forth optimizing throughput.
This changes shows a ~10-25% increase in throughput in the the *Delay*
benchmark tests.
$ go test -bench=. -integration
PASS
BenchmarkRead1k 2 840068631 ns/op 12.48 MB/s
BenchmarkRead16k 20 72968548 ns/op 143.70 MB/s
BenchmarkRead32k 30 56871347 ns/op 184.38 MB/s
BenchmarkRead128k 100 34150953 ns/op 307.05 MB/s
BenchmarkRead512k 100 15730685 ns/op 666.59 MB/s
BenchmarkRead1MiB 200 10462421 ns/op 1002.24 MB/s
BenchmarkRead4MiB 200 7325236 ns/op 1431.47 MB/s
BenchmarkRead4MiBDelay10Msec 10 186893765 ns/op 56.11 MB/s
BenchmarkRead4MiBDelay50Msec 2 907127114 ns/op 11.56 MB/s
BenchmarkRead4MiBDelay150Msec 1 2708025060 ns/op 3.87 MB/s
BenchmarkWrite1k 1 1623940932 ns/op 6.46 MB/s
BenchmarkWrite16k 10 174293843 ns/op 60.16 MB/s
BenchmarkWrite32k 10 120377272 ns/op 87.11 MB/s
BenchmarkWrite128k 20 54592205 ns/op 192.08 MB/s
BenchmarkWrite512k 50 66449591 ns/op 157.80 MB/s
BenchmarkWrite1MiB 50 70965660 ns/op 147.76 MB/s
BenchmarkWrite4MiB 50 69234861 ns/op 151.45 MB/s
BenchmarkWrite4MiBDelay10Msec 5 276624260 ns/op 37.91 MB/s
BenchmarkWrite4MiBDelay50Msec 1 1318396552 ns/op 7.95 MB/s
BenchmarkWrite4MiBDelay150Msec 1 3918416658 ns/op 2.68 MB/s
BenchmarkCopyDown10MiBDelay10Msec 10 152240808 ns/op 68.88 MB/s
BenchmarkCopyDown10MiBDelay50Msec 2 715003188 ns/op 14.67 MB/s
BenchmarkCopyDown10MiBDelay150Msec 1 2116878801 ns/op 4.95 MB/s
BenchmarkCopyUp10MiBDelay10Msec 10 192748258 ns/op 54.40 MB/s
BenchmarkCopyUp10MiBDelay50Msec 2 691486538 ns/op 15.16 MB/s
BenchmarkCopyUp10MiBDelay150Msec 1 1997162991 ns/op 5.25 MB/s
BenchmarkMarshalInit 2000000 644 ns/op
BenchmarkMarshalOpen 3000000 562 ns/op
BenchmarkMarshalWriteWorstCase 20000 75166 ns/op
BenchmarkMarshalWrite1k 500000 3862 ns/op
ok github.com/pkg/sftp 71.174s
This method returns a Client from a pair of pipes. This can be used for
connecting to an SFTP server over TCP/TLS or by using the system's ssh
client program (e.g. via exec.Command).
An Example function is added, and the client integration test uses the
function.
* European sentence punctuation, consistent with package docs
* Match os.SEEK_* constants instead of numeric constants from io.Seeker
* Consistent (shorter) receiver name for quickcheck value
Test that reads at the seeked offset of sftp.File are consistent with
the reads of offset of os.File for the same file.
Error conditions and undefined seeks are not tested.
John Eikenberry
Catalin Tanasescu
Dave Cheney
Mark Sheahan
Matt Layher
Matthew Sykes
DrJosh9000
Glenn Griffin
Maximilian Pachl
Alexander Neumann
Sean Treadway