Guard against panics from typed-nil internal error types (#1452)

This caused some surprising panics in a user's service, and it's not hard to do better.

Author: Groxx

internal/error.go

@@ -190,6 +190,10 @@ const (
 	errReasonGeneric  = "cadenceInternal:Generic"
 	errReasonCanceled = "cadenceInternal:Canceled"
 	errReasonTimeout  = "cadenceInternal:Timeout"
+
+	badNilErrMsgFmt = "cadence received an invalid nil `%T`." +
+		" this likely means you have a typed nil which is being" +
+		" converted into an `error` value: https://go.dev/doc/faq#nil_error"
 )
 
 // ErrNoData is returned when trying to extract strong typed data while there is no data available.

internal/internal_utils.go

@@ -63,9 +63,9 @@ const (
 
 	// defaultRPCTimeout is the default tchannel rpc call timeout
 	defaultRPCTimeout = 10 * time.Second
-	//minRPCTimeout is minimum rpc call timeout allowed
+	// minRPCTimeout is minimum rpc call timeout allowed
 	minRPCTimeout = 1 * time.Second
-	//maxRPCTimeout is maximum rpc call timeout allowed
+	// maxRPCTimeout is maximum rpc call timeout allowed
 	maxRPCTimeout = 5 * time.Second
 	// maxQueryRPCTimeout is the maximum rpc call timeout allowed for query
 	maxQueryRPCTimeout = 20 * time.Second
@@ -243,6 +243,9 @@ func getErrorDetails(err error, dataConverter DataConverter) (string, []byte) {
 	case *CustomError:
 		var data []byte
 		var err0 error
+		if err == nil {
+			return errReasonGeneric, []byte(fmt.Sprintf(badNilErrMsgFmt, err))
+		}
 		switch details := err.details.(type) {
 		case ErrorDetailsValues:
 			data, err0 = encodeArgs(dataConverter, details)
@@ -258,6 +261,10 @@ func getErrorDetails(err error, dataConverter DataConverter) (string, []byte) {
 	case *CanceledError:
 		var data []byte
 		var err0 error
+		if err == nil {
+			// treat this as a failure, not a cancel, as it likely is not a real cancel
+			return errReasonGeneric, []byte(fmt.Sprintf(badNilErrMsgFmt, err))
+		}
 		switch details := err.details.(type) {
 		case ErrorDetailsValues:
 			data, err0 = encodeArgs(dataConverter, details)
@@ -271,6 +278,10 @@ func getErrorDetails(err error, dataConverter DataConverter) (string, []byte) {
 		}
 		return errReasonCanceled, data
 	case *PanicError:
+		if err == nil {
+			// treat this as a failure, not a panic, as it likely is not a real panic
+			return errReasonGeneric, []byte(fmt.Sprintf(badNilErrMsgFmt, err))
+		}
 		data, err0 := encodeArgs(dataConverter, []interface{}{err.Error(), err.StackTrace()})
 		if err0 != nil {
 			panic(err0)
@@ -279,6 +290,10 @@ func getErrorDetails(err error, dataConverter DataConverter) (string, []byte) {
 	case *TimeoutError:
 		var data []byte
 		var err0 error
+		if err == nil {
+			// treat this as a failure, not a timeout, as it likely is not a real timeout
+			return errReasonGeneric, []byte(fmt.Sprintf(badNilErrMsgFmt, err))
+		}
 		switch details := err.details.(type) {
 		case ErrorDetailsValues:
 			data, err0 = encodeArgs(dataConverter, details)

internal/internal_utils_test.go

@@ -25,6 +25,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	s "go.uber.org/cadence/.gen/go/shared"
@@ -135,6 +136,23 @@ func TestGetErrorDetails_TimeoutError(t *testing.T) {
 	require.Equal(t, val2, data)
 }
 
+func Test_getErrorDetails_nil(t *testing.T) {
+	// typed nils caused panics, prevent them from coming back
+	check := func(t *testing.T, err error) {
+		reason, data := getErrorDetails(err, getDefaultDataConverter())
+		dataStr := string(data)
+		assert.Equalf(t, errReasonGeneric, reason, "wrong reason for %T", err)
+		assert.Contains(t, dataStr, "typed nil", "wrong data for %T", err)
+	}
+	check(t, (*CanceledError)(nil))
+	check(t, (*CustomError)(nil))
+	check(t, (*PanicError)(nil))
+	check(t, (*TimeoutError)(nil))
+	// other types are probably also good to handle, but e.g. handling
+	// external error types will require panic recovery.
+	// this might be a good idea, but for now this just covers "we can do better with our types".
+}
+
 func TestConstructError_TimeoutError(t *testing.T) {
 	t.Parallel()
 	dc := getDefaultDataConverter()