Many Unix assuptions have been made when testing.
Previously running tests on windows threw a panic
part way through the tests. After these changes
many more tests pass when the fix was isolated
to the test itself, and none panic.
Some tests are skipped because they do make sense
on windows (chmod, chown), while others are skipped
just to how the test was implemented.
Lastly, some of the external executables were hard coded.
Change these to look paths in TestMain first. This is done
to better support windows, where openssh may be installed and
sftp and sftp-server may both be in the PATH.
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.
match contained lots of cut-n-pasted code from filepath/match.go in the
core libraries. Particularly Match() and Join() were changed in such a
way that made them functionally equivalent to the versions in
path/match.go. This removes the duplicate code and just calls the path
versions.
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
Nicola Murino
Tommie Gannert
Daniel Theophanes
Urjit Singh Bhatia
John Eikenberry
John Eikenberry
Iain Wade
Catalin Tanasescu
Dave Cheney
Mark Sheahan
Matt Layher
Matthew Sykes
DrJosh9000
Glenn Griffin
Maximilian Pachl
Alexander Neumann