Merge pull request #427 from ooni/sandbox-sbs

refactor(errorsx): directly return ErrWrapper when needed

internal/errorsx/dialer.go

@@ -11,55 +11,67 @@ type Dialer interface {
 	DialContext(ctx context.Context, network, address string) (net.Conn, error)
 }
 
-// ErrorWrapperDialer is a dialer that performs error wrapping.
+// ErrorWrapperDialer is a dialer that performs error wrapping. The connection
+// returned by the DialContext function will also perform error wrapping.
 type ErrorWrapperDialer struct {
+	// Dialer is the underlying dialer.
 	Dialer
 }
 
 // DialContext implements Dialer.DialContext.
 func (d *ErrorWrapperDialer) DialContext(ctx context.Context, network, address string) (net.Conn, error) {
 	conn, err := d.Dialer.DialContext(ctx, network, address)
-	err = SafeErrWrapperBuilder{
-		Error:     err,
-		Operation: ConnectOperation,
-	}.MaybeBuild()
 	if err != nil {
-		return nil, err
+		return nil, &ErrWrapper{
+			Failure:    toFailureString(err),
+			Operation:  ConnectOperation,
+			WrappedErr: err,
+		}
 	}
 	return &errorWrapperConn{Conn: conn}, nil
 }
 
 // errorWrapperConn is a net.Conn that performs error wrapping.
 type errorWrapperConn struct {
+	// Conn is the underlying connection.
 	net.Conn
 }
 
 // Read implements net.Conn.Read.
-func (c *errorWrapperConn) Read(b []byte) (n int, err error) {
+func (c *errorWrapperConn) Read(b []byte) (int, error) {
-	n, err = c.Conn.Read(b)
+	count, err := c.Conn.Read(b)
-	err = SafeErrWrapperBuilder{
+	if err != nil {
-		Error:     err,
+		return 0, &ErrWrapper{
-		Operation: ReadOperation,
+			Failure:    toFailureString(err),
-	}.MaybeBuild()
+			Operation:  ReadOperation,
-	return
+			WrappedErr: err,
+		}
+	}
+	return count, nil
 }
 
 // Write implements net.Conn.Write.
-func (c *errorWrapperConn) Write(b []byte) (n int, err error) {
+func (c *errorWrapperConn) Write(b []byte) (int, error) {
-	n, err = c.Conn.Write(b)
+	count, err := c.Conn.Write(b)
-	err = SafeErrWrapperBuilder{
+	if err != nil {
-		Error:     err,
+		return 0, &ErrWrapper{
-		Operation: WriteOperation,
+			Failure:    toFailureString(err),
-	}.MaybeBuild()
+			Operation:  WriteOperation,
-	return
+			WrappedErr: err,
+		}
+	}
+	return count, nil
 }
 
 // Close implements net.Conn.Close.
-func (c *errorWrapperConn) Close() (err error) {
+func (c *errorWrapperConn) Close() error {
-	err = c.Conn.Close()
+	err := c.Conn.Close()
-	err = SafeErrWrapperBuilder{
+	if err != nil {
-		Error:     err,
+		return &ErrWrapper{
-		Operation: CloseOperation,
+			Failure:    toFailureString(err),
-	}.MaybeBuild()
+			Operation:  CloseOperation,
-	return
+			WrappedErr: err,
+		}
+	}
+	return nil
 }

internal/errorsx/dialer_test.go

@@ -18,66 +18,171 @@ func TestErrorWrapperDialerFailure(t *testing.T) {
 		},
 	}}
 	conn, err := d.DialContext(ctx, "tcp", "www.google.com:443")
+	var ew *ErrWrapper
+	if !errors.As(err, &ew) {
+		t.Fatal("cannot convert to ErrWrapper")
+	}
+	if ew.Operation != ConnectOperation {
+		t.Fatal("unexpected operation", ew.Operation)
+	}
+	if ew.Failure != FailureEOFError {
+		t.Fatal("unexpected failure", ew.Failure)
+	}
+	if !errors.Is(ew.WrappedErr, io.EOF) {
+		t.Fatal("unexpected underlying error", ew.WrappedErr)
+	}
 	if conn != nil {
 		t.Fatal("expected a nil conn here")
 	}
-	errorWrapperCheckErr(t, err, ConnectOperation)
-}
-
-func errorWrapperCheckErr(t *testing.T, err error, op string) {
-	if !errors.Is(err, io.EOF) {
-		t.Fatal("expected another error here")
-	}
-	var errWrapper *ErrWrapper
-	if !errors.As(err, &errWrapper) {
-		t.Fatal("cannot cast to ErrWrapper")
-	}
-	if errWrapper.Operation != op {
-		t.Fatal("unexpected Operation")
-	}
-	if errWrapper.Failure != FailureEOFError {
-		t.Fatal("unexpected failure")
-	}
 }
 
 func TestErrorWrapperDialerSuccess(t *testing.T) {
+	origConn := &net.TCPConn{}
 	ctx := context.Background()
 	d := &ErrorWrapperDialer{Dialer: &netxmocks.Dialer{
 		MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
-			return &netxmocks.Conn{
+			return origConn, nil
-				MockRead: func(b []byte) (int, error) {
-					return 0, io.EOF
-				},
-				MockWrite: func(b []byte) (int, error) {
-					return 0, io.EOF
-				},
-				MockClose: func() error {
-					return io.EOF
-				},
-				MockLocalAddr: func() net.Addr {
-					return &net.TCPAddr{Port: 12345}
-				},
-			}, nil
 		},
 	}}
-	conn, err := d.DialContext(ctx, "tcp", "www.google.com")
+	conn, err := d.DialContext(ctx, "tcp", "www.google.com:443")
 	if err != nil {
 		t.Fatal(err)
 	}
-	if conn == nil {
+	ewConn, ok := conn.(*errorWrapperConn)
-		t.Fatal("expected non-nil conn here")
+	if !ok {
+		t.Fatal("cannot cast to errorWrapperConn")
+	}
+	if ewConn.Conn != origConn {
+		t.Fatal("not the connection we expected")
 	}
-	count, err := conn.Read(nil)
-	errorWrapperCheckIOResult(t, count, err, ReadOperation)
-	count, err = conn.Write(nil)
-	errorWrapperCheckIOResult(t, count, err, WriteOperation)
-	err = conn.Close()
-	errorWrapperCheckErr(t, err, CloseOperation)
 }
 
-func errorWrapperCheckIOResult(t *testing.T, count int, err error, op string) {
+func TestErrorWrapperConnReadFailure(t *testing.T) {
-	if count != 0 {
+	c := &errorWrapperConn{
-		t.Fatal("expected nil count here")
+		Conn: &netxmocks.Conn{
+			MockRead: func(b []byte) (int, error) {
+				return 0, io.EOF
+			},
+		},
+	}
+	buf := make([]byte, 1024)
+	cnt, err := c.Read(buf)
+	var ew *ErrWrapper
+	if !errors.As(err, &ew) {
+		t.Fatal("cannot cast error to ErrWrapper")
+	}
+	if ew.Operation != ReadOperation {
+		t.Fatal("invalid operation", ew.Operation)
+	}
+	if ew.Failure != FailureEOFError {
+		t.Fatal("invalid failure", ew.Failure)
+	}
+	if !errors.Is(ew.WrappedErr, io.EOF) {
+		t.Fatal("invalid wrapped error", ew.WrappedErr)
+	}
+	if cnt != 0 {
+		t.Fatal("expected zero here", cnt)
+	}
+}
+
+func TestErrorWrapperConnReadSuccess(t *testing.T) {
+	c := &errorWrapperConn{
+		Conn: &netxmocks.Conn{
+			MockRead: func(b []byte) (int, error) {
+				return len(b), nil
+			},
+		},
+	}
+	buf := make([]byte, 1024)
+	cnt, err := c.Read(buf)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if cnt != len(buf) {
+		t.Fatal("expected len(buf) here", cnt)
+	}
+}
+
+func TestErrorWrapperConnWriteFailure(t *testing.T) {
+	c := &errorWrapperConn{
+		Conn: &netxmocks.Conn{
+			MockWrite: func(b []byte) (int, error) {
+				return 0, io.EOF
+			},
+		},
+	}
+	buf := make([]byte, 1024)
+	cnt, err := c.Write(buf)
+	var ew *ErrWrapper
+	if !errors.As(err, &ew) {
+		t.Fatal("cannot cast error to ErrWrapper")
+	}
+	if ew.Operation != WriteOperation {
+		t.Fatal("invalid operation", ew.Operation)
+	}
+	if ew.Failure != FailureEOFError {
+		t.Fatal("invalid failure", ew.Failure)
+	}
+	if !errors.Is(ew.WrappedErr, io.EOF) {
+		t.Fatal("invalid wrapped error", ew.WrappedErr)
+	}
+	if cnt != 0 {
+		t.Fatal("expected zero here", cnt)
+	}
+}
+
+func TestErrorWrapperConnWriteSuccess(t *testing.T) {
+	c := &errorWrapperConn{
+		Conn: &netxmocks.Conn{
+			MockWrite: func(b []byte) (int, error) {
+				return len(b), nil
+			},
+		},
+	}
+	buf := make([]byte, 1024)
+	cnt, err := c.Write(buf)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if cnt != len(buf) {
+		t.Fatal("expected len(buf) here", cnt)
+	}
+}
+
+func TestErrorWrapperConnCloseFailure(t *testing.T) {
+	c := &errorWrapperConn{
+		Conn: &netxmocks.Conn{
+			MockClose: func() error {
+				return io.EOF
+			},
+		},
+	}
+	err := c.Close()
+	var ew *ErrWrapper
+	if !errors.As(err, &ew) {
+		t.Fatal("cannot cast error to ErrWrapper")
+	}
+	if ew.Operation != CloseOperation {
+		t.Fatal("invalid operation", ew.Operation)
+	}
+	if ew.Failure != FailureEOFError {
+		t.Fatal("invalid failure", ew.Failure)
+	}
+	if !errors.Is(ew.WrappedErr, io.EOF) {
+		t.Fatal("invalid wrapped error", ew.WrappedErr)
+	}
+}
+
+func TestErrorWrapperConnCloseSuccess(t *testing.T) {
+	c := &errorWrapperConn{
+		Conn: &netxmocks.Conn{
+			MockClose: func() error {
+				return nil
+			},
+		},
+	}
+	err := c.Close()
+	if err != nil {
+		t.Fatal(err)
 	}
-	errorWrapperCheckErr(t, err, op)
 }

internal/errorsx/errorsx.go

@@ -105,6 +105,8 @@ func toFailureString(err error) string {
 	// The list returned here matches the values used by MK unless
 	// explicitly noted otherwise with a comment.
 
+	// TODO(bassosimone): we need to always apply this rule not only here
+	// when we're making the most generic conversion.
 	var errwrapper *ErrWrapper
 	if errors.As(err, &errwrapper) {
 		return errwrapper.Error() // we've already wrapped it