Repository: decred/dcrd
Pull request #2208 (2020-05-22 07:32:17)

connmgr: Finish recent connmgr err type additions.
This continues the work which switched the errors in the `connmgr` package to be compatible with `errors.Is/As` introduced in go 1.13 to make it match the best practices.

The following is a high level overview of the changes:

- Change the name of the error kind type to `ErrorKind`
- Make error definitions constant
- Add comments to all exported functions and types as required by the code contribution guidelines and various linters
- Add a `MakeError` function to create the errors instead of creating the structs with unnamed fields which goes against standard Go recommendations
- Add full test coverage to ensure the definitions work as intended

This is a followup to #2206.


Patch:
diff --git a/connmgr/connmanager.go b/connmgr/connmanager.go
index 784f1502..4b15e258 100644
--- a/connmgr/connmanager.go
+++ b/connmgr/connmanager.go
@@ -672,11 +672,11 @@ func (cm *ConnManager) Run(ctx context.Context) {
 // Use Run to start listening and/or connecting to the network.
 func New(cfg *Config) (*ConnManager, error) {
 	if cfg.Dial == nil && cfg.DialAddr == nil {
-		return nil, Error{"dial cannot be nil", ErrDialNil}
+		return nil, MakeError(ErrDialNil, "dial cannot be nil")
 	}
 	if cfg.Dial != nil && cfg.DialAddr != nil {
-		return nil, Error{"cannot specify both Dial and DialAddr",
-			ErrBothDialsFilled}
+		return nil, MakeError(ErrBothDialsFilled,
+			"cannot specify both Dial and DialAddr")
 	}
 	// Default to sane values
 	if cfg.RetryDuration <= 0 {
diff --git a/connmgr/error.go b/connmgr/error.go
index 4b535871..5c9ffdfe 100644
--- a/connmgr/error.go
+++ b/connmgr/error.go
@@ -4,60 +4,81 @@
 
 package connmgr
 
-type Err string
+// ErrorKind identifies a kind of error.  It has full support for errors.Is and
+// errors.As, so the caller can directly check against an error kind when
+// determining the reason for an error.
+type ErrorKind string
 
-func (e Err) Error() string { return string(e) }
-
-type Error struct {
-	Description string
-	Err         error
-}
-
-func (e Error) Error() string { return e.Description }
-func (e Error) Unwrap() error { return e.Err }
-
-var (
+const (
 	// ErrDialNil is used to indicate that Dial cannot be nil in
 	// the configuration.
-	ErrDialNil = Err("ErrDialNil")
+	ErrDialNil = ErrorKind("ErrDialNil")
 
 	// ErrBothDialsFilled is used to indicate that Dial and DialAddr
 	// cannot both be specified in the configuration.
-	ErrBothDialsFilled = Err("ErrBothDialsFilled")
+	ErrBothDialsFilled = ErrorKind("ErrBothDialsFilled")
 
 	// ErrTorInvalidAddressResponse indicates an invalid address was
 	// returned by the Tor DNS resolver.
-	ErrTorInvalidAddressResponse = Err("ErrTorInvalidAddressResponse")
+	ErrTorInvalidAddressResponse = ErrorKind("ErrTorInvalidAddressResponse")
 
 	// ErrTorInvalidProxyResponse indicates the Tor proxy returned a
 	// response in an unexpected format.
-	ErrTorInvalidProxyResponse = Err("ErrTorInvalidProxyResponse")
+	ErrTorInvalidProxyResponse = ErrorKind("ErrTorInvalidProxyResponse")
 
 	// ErrTorUnrecognizedAuthMethod indicates the authentication method
 	// provided is not recognized.
-	ErrTorUnrecognizedAuthMethod = Err("ErrTorUnrecognizedAuthMethod")
+	ErrTorUnrecognizedAuthMethod = ErrorKind("ErrTorUnrecognizedAuthMethod")
 
 	// ErrTorGeneralError indicates a general tor error.
-	ErrTorGeneralError = Err("ErrTorGeneralError")
+	ErrTorGeneralError = ErrorKind("ErrTorGeneralError")
 
 	// ErrTorNotAllowed indicates tor connections are not allowed.
-	ErrTorNotAllowed = Err("ErrTorNotAllowed")
+	ErrTorNotAllowed = ErrorKind("ErrTorNotAllowed")
 
 	// ErrTorNetUnreachable indicates the tor network is unreachable.
-	ErrTorNetUnreachable = Err("ErrTorNetUnreachable")
+	ErrTorNetUnreachable = ErrorKind("ErrTorNetUnreachable")
 
 	// ErrTorHostUnreachable indicates the tor host is unreachable.
-	ErrTorHostUnreachable = Err("ErrTorHostUnreachable")
+	ErrTorHostUnreachable = ErrorKind("ErrTorHostUnreachable")
 
 	// ErrTorConnectionRefused indicates the tor connection was refused.
-	ErrTorConnectionRefused = Err("ErrTorConnectionRefused")
+	ErrTorConnectionRefused = ErrorKind("ErrTorConnectionRefused")
 
 	// ErrTorTTLExpired indicates the tor request Time-To-Live (TTL) expired.
-	ErrTorTTLExpired = Err("ErrTorTTLExpired")
+	ErrTorTTLExpired = ErrorKind("ErrTorTTLExpired")
 
 	// ErrTorCmdNotSupported indicates the tor command is not supported.
-	ErrTorCmdNotSupported = Err("ErrTorCmdNotSupported")
+	ErrTorCmdNotSupported = ErrorKind("ErrTorCmdNotSupported")
 
 	// ErrTorAddrNotSupported indicates the tor address type is not supported.
-	ErrTorAddrNotSupported = Err("ErrTorAddrNotSupported")
+	ErrTorAddrNotSupported = ErrorKind("ErrTorAddrNotSupported")
 )
+
+// Error satisfies the error interface and prints human-readable errors.
+func (e ErrorKind) Error() string {
+	return string(e)
+}
+
+// Error identifies an error related to the connection manager error.  It has
+// full support for errors.Is and errors.As, so the caller can ascertain the
+// specific reason for the error by checking the underlying error.
+type Error struct {
+	Description string
+	Err         error
+}
+
+// Error satisfies the error interface and prints human-readable errors.
+func (e Error) Error() string {
+	return e.Description
+}
+
+// Unwrap returns the underlying wrapped error.
+func (e Error) Unwrap() error {
+	return e.Err
+}
+
+// MakeError creates an Error given a set of arguments.
+func MakeError(kind ErrorKind, desc string) Error {
+	return Error{Err: kind, Description: desc}
+}
diff --git a/connmgr/tor.go b/connmgr/tor.go
index 7536c95c..b58bb35a 100644
--- a/connmgr/tor.go
+++ b/connmgr/tor.go
@@ -30,14 +30,14 @@ const (
 
 var (
 	torStatusErrors = map[byte]error{
-		torGeneralError:      Error{"tor general error", ErrTorGeneralError},
-		torNotAllowed:        Error{"tor not allowed", ErrTorNotAllowed},
-		torNetUnreachable:    Error{"tor network is unreachable", ErrTorNetUnreachable},
-		torHostUnreachable:   Error{"tor host is unreachable", ErrTorHostUnreachable},
-		torConnectionRefused: Error{"tor connection refused", ErrTorConnectionRefused},
-		torTTLExpired:        Error{"tor TTL expired", ErrTorTTLExpired},
-		torCmdNotSupported:   Error{"tor command not supported", ErrTorCmdNotSupported},
-		torAddrNotSupported:  Error{"tor address type not supported", ErrTorAddrNotSupported},
+		torGeneralError:      MakeError(ErrTorGeneralError, "tor general error"),
+		torNotAllowed:        MakeError(ErrTorNotAllowed, "tor not allowed"),
+		torNetUnreachable:    MakeError(ErrTorNetUnreachable, "tor network is unreachable"),
+		torHostUnreachable:   MakeError(ErrTorHostUnreachable, "tor host is unreachable"),
+		torConnectionRefused: MakeError(ErrTorConnectionRefused, "tor connection refused"),
+		torTTLExpired:        MakeError(ErrTorTTLExpired, "tor TTL expired"),
+		torCmdNotSupported:   MakeError(ErrTorCmdNotSupported, "tor command not supported"),
+		torAddrNotSupported:  MakeError(ErrTorAddrNotSupported, "tor address type not supported"),
 	}
 )
 
@@ -62,12 +62,12 @@ func TorLookupIP(ctx context.Context, host, proxy string) ([]net.IP, error) {
 		return nil, err
 	}
 	if buf[0] != 0x05 {
-		return nil, Error{"invalid SOCKS proxy version",
-			ErrTorInvalidProxyResponse}
+		return nil, MakeError(ErrTorInvalidProxyResponse,
+			"invalid SOCKS proxy version")
 	}
 	if buf[1] != 0x00 {
-		return nil, Error{"invalid proxy authentication method",
-			ErrTorUnrecognizedAuthMethod}
+		return nil, MakeError(ErrTorUnrecognizedAuthMethod,
+			"invalid proxy authentication method")
 	}
 
 	buf = make([]byte, 7+len(host))
@@ -90,19 +90,20 @@ func TorLookupIP(ctx context.Context, host, proxy string) ([]net.IP, error) {
 		return nil, err
 	}
 	if buf[0] != 5 {
-		return nil, Error{"invalid SOCKS proxy version",
-			ErrTorInvalidProxyResponse}
+		return nil, MakeError(ErrTorInvalidProxyResponse,
+			"invalid SOCKS proxy version")
 	}
 	if buf[1] != 0 {
 		err, exists := torStatusErrors[buf[1]]
 		if !exists {
-			err = Error{"invalid SOCKS proxy version",
-				ErrTorInvalidProxyResponse}
+			err = MakeError(ErrTorInvalidProxyResponse,
+				"invalid SOCKS proxy version")
 		}
 		return nil, err
 	}
 	if buf[3] != torATypeIPv4 && buf[3] != torATypeIPv6 {
-		return nil, Error{"invalid IP address", ErrTorInvalidAddressResponse}
+		return nil, MakeError(ErrTorInvalidAddressResponse,
+			"invalid IP address")
 	}
 
 	var reply [32 + 2]byte
@@ -115,18 +116,21 @@ func TorLookupIP(ctx context.Context, host, proxy string) ([]net.IP, error) {
 	switch buf[3] {
 	case torATypeIPv4:
 		if replyLen != 4+2 {
-			return nil, Error{"invalid IPV4 address", ErrTorInvalidAddressResponse}
+			return nil, MakeError(ErrTorInvalidAddressResponse,
+				"invalid IPV4 address")
 		}
 		r := binary.BigEndian.Uint32(reply[0:4])
 		addr = net.IPv4(byte(r>>24), byte(r>>16),
 			byte(r>>8), byte(r))
 	case torATypeIPv6:
 		if replyLen <= 4+2 {
-			return nil, Error{"invalid IPV6 address", ErrTorInvalidAddressResponse}
+			return nil, MakeError(ErrTorInvalidAddressResponse,
+				"invalid IPV6 address")
 		}
 		addr = net.IP(reply[0 : replyLen-2])
 	default:
-		return nil, Error{"unknown address type", ErrTorInvalidAddressResponse}
+		return nil, MakeError(ErrTorInvalidAddressResponse,
+			"unknown address type")
 	}
 
 	return []net.IP{addr}, nil


Test patch:
diff --git a/connmgr/error_test.go b/connmgr/error_test.go
new file mode 100644
index 00000000..f01cc189
--- /dev/null
+++ b/connmgr/error_test.go
@@ -0,0 +1,146 @@
+// Copyright (c) 2020 The Decred developers
+// Use of this source code is governed by an ISC
+// license that can be found in the LICENSE file.
+
+package connmgr
+
+import (
+	"errors"
+	"io"
+	"testing"
+)
+
+// TestErrorKindStringer tests the stringized output for the ErrorKind type.
+func TestErrorKindStringer(t *testing.T) {
+	tests := []struct {
+		in   ErrorKind
+		want string
+	}{
+		{ErrDialNil, "ErrDialNil"},
+		{ErrBothDialsFilled, "ErrBothDialsFilled"},
+		{ErrTorInvalidAddressResponse, "ErrTorInvalidAddressResponse"},
+		{ErrTorInvalidProxyResponse, "ErrTorInvalidProxyResponse"},
+		{ErrTorUnrecognizedAuthMethod, "ErrTorUnrecognizedAuthMethod"},
+		{ErrTorGeneralError, "ErrTorGeneralError"},
+		{ErrTorNotAllowed, "ErrTorNotAllowed"},
+		{ErrTorNetUnreachable, "ErrTorNetUnreachable"},
+		{ErrTorHostUnreachable, "ErrTorHostUnreachable"},
+		{ErrTorConnectionRefused, "ErrTorConnectionRefused"},
+		{ErrTorTTLExpired, "ErrTorTTLExpired"},
+		{ErrTorCmdNotSupported, "ErrTorCmdNotSupported"},
+		{ErrTorAddrNotSupported, "ErrTorAddrNotSupported"},
+	}
+
+	for i, test := range tests {
+		result := test.in.Error()
+		if result != test.want {
+			t.Errorf("#%d: got: %s want: %s", i, result, test.want)
+			continue
+		}
+	}
+}
+
+// TestError tests the error output for the Error type.
+func TestError(t *testing.T) {
+	tests := []struct {
+		in   Error
+		want string
+	}{{
+		Error{Description: "some error"},
+		"some error",
+	}, {
+		Error{Description: "human-readable error"},
+		"human-readable error",
+	}}
+
+	for i, test := range tests {
+		result := test.in.Error()
+		if result != test.want {
+			t.Errorf("#%d: got: %s want: %s", i, result, test.want)
+			continue
+		}
+	}
+}
+
+// TestErrorKindIsAs ensures both ErrorKind and Error can be identified as being
+// a specific error kind via errors.Is and unwrapped via errors.As.
+func TestErrorKindIsAs(t *testing.T) {
+	tests := []struct {
+		name      string
+		err       error
+		target    error
+		wantMatch bool
+		wantAs    ErrorKind
+	}{{
+		name:      "ErrDialNil == ErrDialNil",
+		err:       ErrDialNil,
+		target:    ErrDialNil,
+		wantMatch: true,
+		wantAs:    ErrDialNil,
+	}, {
+		name:      "Error.ErrDialNil == ErrDialNil",
+		err:       MakeError(ErrDialNil, ""),
+		target:    ErrDialNil,
+		wantMatch: true,
+		wantAs:    ErrDialNil,
+	}, {
+		name:      "Error.ErrDialNil == Error.ErrDialNil",
+		err:       MakeError(ErrDialNil, ""),
+		target:    MakeError(ErrDialNil, ""),
+		wantMatch: true,
+		wantAs:    ErrDialNil,
+	}, {
+		name:      "ErrBothDialsFilled != ErrDialNil",
+		err:       ErrBothDialsFilled,
+		target:    ErrDialNil,
+		wantMatch: false,
+		wantAs:    ErrBothDialsFilled,
+	}, {
+		name:      "Error.ErrBothDialsFilled != ErrDialNil",
+		err:       MakeError(ErrBothDialsFilled, ""),
+		target:    ErrDialNil,
+		wantMatch: false,
+		wantAs:    ErrBothDialsFilled,
+	}, {
+		name:      "ErrBothDialsFilled != Error.ErrDialNil",
+		err:       ErrBothDialsFilled,
+		target:    MakeError(ErrDialNil, ""),
+		wantMatch: false,
+		wantAs:    ErrBothDialsFilled,
+	}, {
+		name:      "Error.ErrBothDialsFilled != Error.ErrDialNil",
+		err:       MakeError(ErrBothDialsFilled, ""),
+		target:    MakeError(ErrDialNil, ""),
+		wantMatch: false,
+		wantAs:    ErrBothDialsFilled,
+	}, {
+		name:      "Error.ErrBothDialsFilled != io.EOF",
+		err:       MakeError(ErrBothDialsFilled, ""),
+		target:    io.EOF,
+		wantMatch: false,
+		wantAs:    ErrBothDialsFilled,
+	}}
+
+	for _, test := range tests {
+		// Ensure the error matches or not depending on the expected result.
+		result := errors.Is(test.err, test.target)
+		if result != test.wantMatch {
+			t.Errorf("%s: incorrect error identification -- got %v, want %v",
+				test.name, result, test.wantMatch)
+			continue
+		}
+
+		// Ensure the underlying error kind can be unwrapped is and is the
+		// expected kind.
+		var kind ErrorKind
+		if !errors.As(test.err, &kind) {
+			t.Errorf("%s: unable to unwrap to error kind", test.name)
+			continue
+		}
+		if kind != test.wantAs {
+			t.Errorf("%s: unexpected unwrapped error kind -- got %v, want %v",
+				test.name, kind, test.wantAs)
+			continue
+		}
+	}
+}