Improve benchmarks and errors

client.go

@@ -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
@@ -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
@@ -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

client_integration_test.go

@@ -6,7 +6,6 @@ package sftp
 import (
 	"bytes"
 	"crypto/sha1"
-	"encoding"
 	"errors"
 	"io"
 	"io/ioutil"
@@ -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
@@ -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 < 5; 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 < 5; 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)) {
@@ -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)
 }
@@ -2444,6 +2454,28 @@ func BenchmarkReadFrom4MiBDelay150Msec(b *testing.B) {
 	benchmarkReadFrom(b, 4*1024*1024, 150*time.Millisecond)
 }
 
+// writeToBuffer implements the relevant parts of bytes.Buffer,
+// but does not release its internal buffer when Reset.
+//
+// Release its internal memory when Reset is good for avoiding memory leaks,
+// but not great for memory benchmarks, as this fills up a lot of irrelevant allocations.
+type writeToBuffer struct {
+	b []byte
+}
+
+func (w *writeToBuffer) Len() int {
+	return len(w.b)
+}
+
+func (w *writeToBuffer) Reset() {
+	w.b = w.b[:0]
+}
+
+func (w *writeToBuffer) Write(b []byte) (int, error) {
+	w.b = append(w.b, b...)
+	return len(b), nil
+}
+
 func benchmarkWriteTo(b *testing.B, bufsize int, delay time.Duration) {
 	size := 10*1024*1024 + 123 // ~10MiB
 
@@ -2466,7 +2498,9 @@ func benchmarkWriteTo(b *testing.B, bufsize int, delay time.Duration) {
 	b.ResetTimer()
 	b.SetBytes(int64(size))
 
-	buf := new(bytes.Buffer)
+	buf := &writeToBuffer{
+		b: make([]byte, 0, size),
+	}
 
 	for i := 0; i < b.N; i++ {
 		buf.Reset()

conn.go

@@ -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.
@@ -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)
 }
 
@@ -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}

example_test.go

@@ -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)
 						}
 					}
 				}

go.sum

@@ -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=

packet.go

@@ -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
@@ -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")
 		}
 	}