Fixes#234
Similar to #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.
to support actual rename() operations rather than the link() call
effectively mandated by the sftp v3/draft-2 requirement that targets
not be overwritten.
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.
Fixes issue #167
File read/write client methods were breaking out of the packet handling
loops with packets still 'in flight' (packets sent without reply from
server) when errors occured. This caused problems after the fact with
the returning packets interfearing with later calls.
This change makes sure all the in flight packets are processed (even if
just discarded) before breaking out of the loop and returning.
Fixes github issue #145
Increase the buffer of channel for dispatchRequest() responses to be
equal to the maximum possible difference between desiredInFlight minus
inFlight, as this is the maximum possible error returns it would get
before moving on to channel handling code.
Cisco SSH 2.0 server doesn't fill out the "message" field,
even though it returns valid status codes. This makes
File.Read method error out without actually reading the
file due to unmarshalStatus returning a "packet too short"
error.
When writer returns an error, the code would mess up inFlight counter
and the loop will never finish as the result.
We switch to only use inFlight to count requests to sftp server -
we delete from pendingWrites now so we can use len(pendingWrites) to
count.
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.
sendRequest was not returning a value to the channel if there was
_not_ an error. How did this even work ?!?
Also remove server_test.go because it was wrong, it was testing that
sending a properly formed, but invalid (ie, requested wrong extended
packet) packet would cause an error during send ... which is wrong.
sendRequest will only return an error if the client connection was
closed.
Fixes#55
c.sendRequest always does the right thing, but a few fast paths were not
constructing a buffered channel which would cause c.dispatchRequest to
block on error.
Previously sftp.Close() would only shutdown the write side of the
connection and return. The sftp server would then shutdown the read side
of the connection at which point the recv goroutine would receive a
failure and return. When using a local sftp server in the intergration
tests we first call sftp.Close() followed by cmd.Wait(). cmd.Wait()
calls close on stdout while the sftp recv goroutine is still reading
from it. The fix is to block in sftp.Close() until the sftp server has
closed the receive end of the connection and the recv goroutine has
subsequently exited.
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 is an attempt to permit overlapping requests.
For each request sent, we spin off a goroutine to send the request for us
then handle _a_ response. That goroutine will try to dispatch the request
to the original sender. As every request is initiated from the client, there
will always be a matching number of replies from the server so eventally
every worker goroutine will process a request and exit.
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.
Taizeng Wu
gm42
Nick Craig-Wood
Urjit Singh Bhatia
John Eikenberry
Cody Jones
Paul
Iain Wade
unclejack
Allan Feid
Andrew Kupasrimonkol
Paul Komkoff
Carl Jackson
Dave Cheney
akupas
Matt Layher
xiu
Matthew Sykes
Philip Linde
Josh Deprez
Mark Sheahan
Glenn Griffin
Maximilian Pachl
Javier Blazquez
Otto Bretz
Alexander Neumann
Matthieu Rakotojaona
Andy Hochhaus
Donavan Pantke
Sean Treadway