ErrorNode should not always implement Causer interface

[jjcollinge]

I am using the package github.com/Azure/azure-storage-blob-go/2016-05-31/azblob to add support for Azure Storage to the Pulumi project (pulumi/pulumi#2026). Currently, when I receive a StorageError the Pulumi codebase uses errors.Wrap(err, "") to add annotations to the original error. The error is then unwrapped using errors.Cause(err) to determine if the original error was a "not found" error (BlobNotFound in our case). However, as the current implementation of ErrorNode can always be type asserted against the Causer interface, even if the cause error is nil, it breaks the expected behavior for errors.Cause(err) defined here. I've inlined the code so that I can annotate.

func Cause(err error) error {
	type causer interface {
		Cause() error
	}

	for err != nil {
		cause, ok := err.(causer) //  1. This will return OK, even if the cause is nil
		if !ok {
			break // 4. We actually want to break here with the original error
		}
		err = cause.Cause() // 2. This will then return nil
	}
	return err // 3. Meaning this will return nil
}

This means we get nil back rather than the nested StorageError as expected and in accordance to how the standard library errors work.

I have created a temporary work around in pulumi by defining a new error struct that implements the StorageError interface but does not implement Causer and this has fixed my code. This is available here: https://github.com/pulumi/pulumi/blob/03d8939eee39599dcee8e3328fcf6c1da6ce1f47/pkg/backend/filestate/storage/azure/error.go

My proposal would be to define a new error type pcErrorNoCause and a new type ErrorNodeNoCause that doesn't implement the Causer interface. Then in NewError() error check whether the cause parameter is nil and if so return a pcErrorNoCause error instead of the existing pcError. Something similar to below:

// NewError creates a simple string error (like Error.New). But, this
// error also captures the caller's Program Counter and the preceding error.
func NewError(cause error, msg string) error {
        if cause != nil {
                return &pcError{
		    ErrorNode: ErrorNode{}.Initialize(cause, 3),
		    msg:       msg,
	        }
        }
	return &pcErrorNoCause{
		ErrorNode: ErrorNodeNoCause{}.Initialize(3),
		msg:       msg,
	}
}

Offending code:

azure-pipeline-go/pipeline/error.go

Line 106 in b8e3409

func NewError(cause error, msg string) error {

Does this sound reasonable? If so, I am happy to PR the change.

[JeffreyRichter]

Yes, this sounds reasonable and we would accept the PR. Thank you. * Jeffrey Richter (watch Architecting Distributed Cloud Apps<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fazure.microsoft.com%2Ftraining%2Fdistributed-cloud-apps%2F&data=02%7C01%7Cjeffreyr%40microsoft.com%7C948cf3366acf45f6ef8d08d62e3b3b00%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636747229263047172&sdata=sg%2FtzHWpscWQDr7foOSsaNrKDP5vNSHE3b72HJG8swU%3D&reserved=0>) From: Joni Collinge <[email protected]> Sent: Wednesday, October 24, 2018 7:38 AM To: Azure/azure-pipeline-go <[email protected]> Cc: Subscribed <[email protected]> Subject: [Azure/azure-pipeline-go] ErrorNode should not always implement Causer interface (#13) I am using the package github.com/Azure/azure-storage-blob-go/2016-05-31/azblob to add support for Azure Storage to the Pulumi project (pulumi/pulumi#2026<pulumi/pulumi#2026>). Currently, when I receive a StorageError the Pulumi codebase uses errors.Wrap(err, "") to add annotations to the original error. The error is then unwrapped using errors.Cause(err) to determine if the original error was a "not found" error (BlobNotFound in our case). However, as the current implementation of ErrorNode can always be type asserted against the Causer interface even if the cause error is nil it breaks the expected behavior for errors.Cause(err) defined here<https://github.com/pkg/errors/blob/059132a15dd08d6704c67711dae0cf35ab991756/errors.go#L269>. I've inlined the code so that I can annotate. func Cause(err error) error { type causer interface { Cause() error } for err != nil { cause, ok := err.(causer) // 1. This will return OK, even if the cause is nil if !ok { break // 4. We actually want to break here with the original error } err = cause.Cause() // 2. This will then return nil } return err // 3. Meaning this will return nil } This means we get nil back rather than the nested StorageError as expected and in accordance to how the standard library errors work. I have created a temporary work around in pulumi by defining a new error struct that implements the StorageError interface but does not implement Causer and this has fixed my code. My proposal would be to define a new error type pcErrorNoCause and a new type ErrorNodeNoCause that doesn't implement the Causer interface. Then in NewError() error check whether the cause parameter is nil and if so return a pcErrorNoCause error instead of the existing pcError. Something similar to below: // NewError creates a simple string error (like Error.New). But, this // error also captures the caller's Program Counter and the preceding error. func NewError(cause error, msg string) error { if cause != nil { return &pcError{ ErrorNode: ErrorNode{}.Initialize(cause, 3), msg: msg, } } return &pcErrorNoCause{ ErrorNode: ErrorNodeNoCause{}.Initialize(3), msg: msg, } } Offending code: https://github.com/Azure/azure-pipeline-go/blob/b8e3409182fd52e74f7d7bdfbff5833591b3b655/pipeline/error.go#L106 Does this sound reasonable? If so, I am happy to PR the change. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#13>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACK0v4_bNmGwRqcvDhQyLaRat5e3B3N6ks5uoHtCgaJpZM4X4E3_>.