Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve benchmarks and errors #421

Merged
merged 7 commits into from
Mar 20, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
.PHONY: integration integration_w_race benchmark

integration:
go test -integration -v ./...
go test -testserver -v ./...
Expand All @@ -14,4 +16,9 @@ integration_w_race:
go test -race -testserver -allocator -v ./...
go test -race -integration -allocator -testserver -v ./...

COUNT ?= 1
BENCHMARK_PATTERN ?= "."

benchmark:
go test -integration -run=NONE -bench=$(BENCHMARK_PATTERN) -benchmem -memprofile memprofile.out -count=$(COUNT)
go tool pprof -svg -output=memprofile.svg memprofile.out
puellanivis marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 4 additions & 4 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@ type ClientOption func(*Client) error
func MaxPacketChecked(size int) ClientOption {
return func(c *Client) error {
if size < 1 {
return errors.Errorf("size must be greater or equal to 1")
return errors.New("size must be greater or equal to 1")
}
if size > 32768 {
return errors.Errorf("sizes larger than 32KB might not work with all servers")
return errors.New("sizes larger than 32KB might not work with all servers")
}
c.maxPacket = size
return nil
Expand All @@ -65,7 +65,7 @@ func MaxPacketChecked(size int) ClientOption {
func MaxPacketUnchecked(size int) ClientOption {
return func(c *Client) error {
if size < 1 {
return errors.Errorf("size must be greater or equal to 1")
return errors.New("size must be greater or equal to 1")
}
c.maxPacket = size
return nil
Expand All @@ -90,7 +90,7 @@ func MaxPacket(size int) ClientOption {
func MaxConcurrentRequestsPerFile(n int) ClientOption {
return func(c *Client) error {
if n < 1 {
return errors.Errorf("n must be greater or equal to 1")
return errors.New("n must be greater or equal to 1")
}
c.maxConcurrentRequests = n
return nil
Expand Down
130 changes: 70 additions & 60 deletions client_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package sftp
import (
"bytes"
"crypto/sha1"
"encoding"
"errors"
"io"
"io/ioutil"
Expand Down Expand Up @@ -1490,26 +1489,48 @@ func TestClientReadFrom(t *testing.T) {
var errFakeNet = errors.New("Fake network issue")

func TestClientReadFromDeadlock(t *testing.T) {
clientWriteDeadlock(t, 1, func(f *File) {
b := make([]byte, 32768*4)
content := bytes.NewReader(b)
_, err := f.ReadFrom(content)
if err != errFakeNet {
t.Fatal("Didn't recieve correct error:", err)
}
})
for i := 0; i < 5; i++ {
clientWriteDeadlock(t, i, func(f *File) {
b := make([]byte, 32768*4)
content := bytes.NewReader(b)
_, err := f.ReadFrom(content)
if !errors.Is(err, errFakeNet) {
t.Fatal("Didn't recieve correct error:", err)
}
})
}
}

// Write has exact same problem
func TestClientWriteDeadlock(t *testing.T) {
clientWriteDeadlock(t, 1, func(f *File) {
b := make([]byte, 32768*4)
for i := 0; i < 5; i++ {
clientWriteDeadlock(t, i, func(f *File) {
b := make([]byte, 32768*4)

_, err := f.Write(b)
if err != errFakeNet {
t.Fatal("Didn't recieve correct error:", err)
}
})
_, err := f.Write(b)
if !errors.Is(err, errFakeNet) {
t.Fatal("Didn't recieve correct error:", err)
}
})
}
}

type timeBombWriter struct {
count int
w io.WriteCloser
}

func (w *timeBombWriter) Write(b []byte) (int, error) {
if w.count < 1 {
return 0, errFakeNet
}

w.count--
return w.w.Write(b)
}

func (w *timeBombWriter) Close() error {
return w.w.Close()
}

// shared body for both previous tests
Expand All @@ -1534,48 +1555,45 @@ func clientWriteDeadlock(t *testing.T, N int, badfunc func(*File)) {
}
defer w.Close()

// Override sendPacket with failing version
// Replicates network error/drop part way through (after 1 good packet)
count := 0
sendPacketTest := func(w io.Writer, m encoding.BinaryMarshaler) error {
count++
if count > N {
return errFakeNet
}
return sendPacket(w, m)
// Override the clienConn Writer with a failing version
// Replicates network error/drop part way through (after N good writes)
wrap := sftp.clientConn.conn.WriteCloser
sftp.clientConn.conn.WriteCloser = &timeBombWriter{
count: N,
w: wrap,
}
sftp.clientConn.conn.sendPacketTest = sendPacketTest
defer func() {
sftp.clientConn.conn.sendPacketTest = nil
}()

// this locked (before the fix)
badfunc(w)
}

// Read/WriteTo has this issue as well
func TestClientReadDeadlock(t *testing.T) {
clientReadDeadlock(t, 1, func(f *File) {
b := make([]byte, 32768*4)
for i := 0; i < 3; i++ {
clientReadDeadlock(t, i, func(f *File) {
b := make([]byte, 32768*4)

_, err := f.Read(b)
if err != errFakeNet {
t.Fatal("Didn't recieve correct error:", err)
}
})
_, err := f.Read(b)
if !errors.Is(err, errFakeNet) {
t.Fatal("Didn't recieve correct error:", err)
}
})
}
}

func TestClientWriteToDeadlock(t *testing.T) {
clientReadDeadlock(t, 2, func(f *File) {
b := make([]byte, 32768*4)
for i := 0; i < 3; i++ {
clientReadDeadlock(t, i, func(f *File) {
b := make([]byte, 32768*4)

buf := bytes.NewBuffer(b)
buf := bytes.NewBuffer(b)

_, err := f.WriteTo(buf)
if err != errFakeNet {
t.Fatal("Didn't recieve correct error:", err)
}
})
_, err := f.WriteTo(buf)
if !errors.Is(err, errFakeNet) {
t.Fatal("Didn't recieve correct error:", err)
}
})
}
}

func clientReadDeadlock(t *testing.T, N int, badfunc func(*File)) {
Expand Down Expand Up @@ -1611,22 +1629,14 @@ func clientReadDeadlock(t *testing.T, N int, badfunc func(*File)) {
}
defer r.Close()

// Override sendPacket with failing version
// Replicates network error/drop part way through (after 1 good packet)
count := 0
sendPacketTest := func(w io.Writer, m encoding.BinaryMarshaler) error {
count++
if count > N {
return errFakeNet
}
return sendPacket(w, m)
// Override the clienConn Writer with a failing version
// Replicates network error/drop part way through (after N good writes)
wrap := sftp.clientConn.conn.WriteCloser
sftp.clientConn.conn.WriteCloser = &timeBombWriter{
count: N,
w: wrap,
}

sftp.clientConn.conn.sendPacketTest = sendPacketTest
defer func() {
sftp.clientConn.conn.sendPacketTest = nil
}()

// this locked (before the fix)
badfunc(r)
}
Expand Down Expand Up @@ -2463,11 +2473,11 @@ func benchmarkWriteTo(b *testing.B, bufsize int, delay time.Duration) {
f.Write(data)
f.Close()

buf := new(bytes.Buffer)
puellanivis marked this conversation as resolved.
Show resolved Hide resolved

b.ResetTimer()
b.SetBytes(int64(size))

buf := new(bytes.Buffer)

for i := 0; i < b.N; i++ {
buf.Reset()

Expand Down
8 changes: 2 additions & 6 deletions conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ type conn struct {
// this is the same allocator used in packet manager
alloc *allocator
sync.Mutex // used to serialise writes to sendPacket
// sendPacketTest is needed to replicate packet issues in testing
sendPacketTest func(w io.Writer, m encoding.BinaryMarshaler) error
}

// the orderID is used in server mode if the allocator is enabled.
Expand All @@ -29,9 +27,7 @@ func (c *conn) recvPacket(orderID uint32) (uint8, []byte, error) {
func (c *conn) sendPacket(m encoding.BinaryMarshaler) error {
c.Lock()
defer c.Unlock()
if c.sendPacketTest != nil {
return c.sendPacketTest(c, m)
}

return sendPacket(c, m)
}

Expand Down Expand Up @@ -91,7 +87,7 @@ func (c *clientConn) recv() error {
// This is an unexpected occurrence. Send the error
// back to all listeners so that they terminate
// gracefully.
return errors.Errorf("sid not found: %v", sid)
return errors.Errorf("sid not found: %d", sid)
}

ch <- result{typ: typ, data: data}
Expand Down
2 changes: 1 addition & 1 deletion example_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ func ExampleClient_Mkdir_parents() {
fi, err = client.Stat(parents)
if err == nil {
if !fi.IsDir() {
return fmt.Errorf("File exists: %s", parents)
return fmt.Errorf("file exists: %s", parents)
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,12 @@ golang.org/x/crypto v0.0.0-20201221181555-eec23a3978ad/go.mod h1:jdWPYTVW3xRLrWP
golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
golang.org/x/sys v0.0.0-20191026070338-33540a1f6037/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/sys v0.0.0-20210119212857-b64e53b001e4 h1:myAQVi0cGEoqQVR5POX+8RR2mrocKqNN1hmeMqhX27k=
golang.org/x/sys v0.0.0-20210119212857-b64e53b001e4/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
golang.org/x/term v0.0.0-20201117132131-f5c789dd3221 h1:/ZHdbVpdR/jk3g30/d4yUL0JU9kksj8+F/bnQUVLGDM=
golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c h1:dUUwHk2QECo/6vqA44rthZ8ie2QXMNeKRTHCNY2nXvo=
gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
6 changes: 3 additions & 3 deletions packet.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ func marshalPacket(m encoding.BinaryMarshaler) (header, payload []byte, err erro
func sendPacket(w io.Writer, m encoding.BinaryMarshaler) error {
header, payload, err := marshalPacket(m)
if err != nil {
return errors.Errorf("binary marshaller failed: %v", err)
return errors.Wrap(err, "binary marshaller failed")
}

length := len(header) + len(payload) - 4 // subtract the uint32(length) from the start
Expand All @@ -146,12 +146,12 @@ func sendPacket(w io.Writer, m encoding.BinaryMarshaler) error {
binary.BigEndian.PutUint32(header[:4], uint32(length))

if _, err := w.Write(header); err != nil {
return errors.Errorf("failed to send packet: %v", err)
return errors.Wrap(err, "failed to send packet")
}

if len(payload) > 0 {
if _, err := w.Write(payload); err != nil {
return errors.Errorf("failed to send packet payload: %v", err)
return errors.Wrap(err, "failed to send packet payload")
}
}

Expand Down
Loading