Potential confusion about `apistatus` error types' semantic equivalence #481

New Issue

Closed

opened 2023-06-28 12:09:20 +00:00 by ale64bit · 5 comments

ale64bit commented

2023-06-28 12:09:20 +00:00

Collaborator

Consider the following (failing) test:

import (
	"testing"

	"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/container"
	apistatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status"
	"github.com/stretchr/testify/require"
)

func f1() error { return apistatus.ContainerNotFound{} }
func f2() error { return new(apistatus.ContainerNotFound) }

func TestSemanticErrorEquivalence(t *testing.T) {
	require.True(t, container.IsErrNotFound(f1()))
	require.True(t, container.IsErrNotFound(f2())) // fails
}

The failure is due to the implementation of container.IsErrNotFound:

func IsErrNotFound(err error) bool {
	return errors.As(err, new(apistatus.ContainerNotFound))
}

In general, this can give rise to some very subtle hard-to-track bugs when using pointers or structs, since we often act on error types. For example, it could cause objects to not be buried (inhumed) silently by the policer: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/policer/check.go#L81.

We should do something about this to save ourselves from future pain.

Consider the following (failing) test: ```golang import ( "testing" "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/core/container" apistatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status" "github.com/stretchr/testify/require" ) func f1() error { return apistatus.ContainerNotFound{} } func f2() error { return new(apistatus.ContainerNotFound) } func TestSemanticErrorEquivalence(t *testing.T) { require.True(t, container.IsErrNotFound(f1())) require.True(t, container.IsErrNotFound(f2())) // fails } ``` The failure is due to the implementation of [container.IsErrNotFound](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/core/container/storage.go#L43): ```golang func IsErrNotFound(err error) bool { return errors.As(err, new(apistatus.ContainerNotFound)) } ``` In general, this can give rise to some very subtle hard-to-track bugs when using pointers or structs, since we often act on error types. For example, it could cause objects to not be buried (inhumed) silently by the policer: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/policer/check.go#L81. We should do something about this to save ourselves from future pain.

ale64bit added the

bug

discussion

triage

labels 2023-06-28 12:09:20 +00:00

fyrchik added this to the v0.38.0 milestone 2023-07-10 13:14:22 +00:00

aarifullin commented

2023-07-31 14:59:57 +00:00

Collaborator

I suppose we've already got a bug

Here is ObjectAccessDenided status check within Range remote procedure call. The reason has not been discovered yet but the problem is that errors.As never works with apistatus error.
This leads to incorrect behavior and unexpected error

I suppose we've already got a bug Here is [ObjectAccessDenided](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/services/object/get/types.go#L155) status check within `Range` remote procedure call. The reason has not been discovered yet but the problem is that `errors.As` never works with apistatus error. This leads to incorrect behavior and unexpected error

ale64bit commented

2023-08-01 09:48:58 +00:00

Poster

Collaborator

can we make all apistatus error receivers pointers and use errors.Is instead? this would avoid the issue altogether at compile time:

type myError struct{}

func (*myError) Error() string { return "my error msg" }

func f1() error { return myError{} } // doesn't compile
func f2() error { return new(myError) }

func TestSemanticErrorEquivalence(t *testing.T) {
	require.True(t, errors.Is(f1(), myError{}))    // doesn't compile
	require.True(t, errors.Is(f2(), new(myError))) // passes
}

EDIT: although that doesn't work when errors are not actually equal. So perhaps the solution is just implementing Is for each error. Kinda annoying.

can we make all `apistatus` error receivers pointers and use `errors.Is` instead? this would avoid the issue altogether at compile time: ```go type myError struct{} func (*myError) Error() string { return "my error msg" } func f1() error { return myError{} } // doesn't compile func f2() error { return new(myError) } func TestSemanticErrorEquivalence(t *testing.T) { require.True(t, errors.Is(f1(), myError{})) // doesn't compile require.True(t, errors.Is(f2(), new(myError))) // passes } ``` EDIT: although that doesn't work when errors are not actually equal. So perhaps the solution is just implementing `Is` for each error. Kinda annoying.

ale64bit was assigned by fyrchik

2023-08-01 11:10:29 +00:00

ale64bit referenced this issue from TrueCloudLab/frostfs-sdk-go

2023-08-02 07:02:12 +00:00

[#xx] Make all error status receivers pointers #135

ale64bit referenced this issue

2023-08-04 11:16:11 +00:00

[#481] Update frostfs-sdk-go and error pointer receivers #567

ale64bit commented

2023-08-07 11:25:22 +00:00

Poster

Collaborator

I think it's easier to export wrapsErrType and use that everywhere.

The alternatives I see are:

Keep it unexported and implement IsErrX for every error type X.
Keep it unexported and implement the appropriate Is for every error type. Note that in this case, Is must handle the case where the error contains error-specific data and so it's not pure equality.

I think it's easier to export [wrapsErrType](https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/branch/master/client/errors.go#L9-L26) and use that everywhere. The alternatives I see are: 1. Keep it unexported and implement `IsErrX` for every error type X. 2. Keep it unexported and implement the appropriate [Is](https://pkg.go.dev/errors#Is) for every error type. Note that in this case, `Is` must handle the case where the error contains error-specific data and so it's not pure equality.

fyrchik referenced this issue from a commit

2023-08-09 10:26:57 +00:00

[#481] Update frostfs-sdk-go and error pointer receivers

fyrchik commented

2023-08-15 13:21:17 +00:00

Owner

Think wrapsErrType and (2) are not mutually exclusive, both are ok. The only thing I don't like about wrapsErrType is generic errors checking function being exported from the SDK package -- we already have errors.Is, it may be more appropriate here.

Another option is to have a single type for status errors and status.Code extractor, like for gRPC. It wont fork with multi-wrapped errors, but it doesn't seem a problem.

Think `wrapsErrType` and (2) are not mutually exclusive, both are ok. The only thing I don't like about `wrapsErrType` is generic errors checking function being exported from the SDK package -- we already have `errors.Is`, it may be more appropriate here. Another option is to have a single type for status errors and `status.Code` extractor, like for gRPC. It wont fork with multi-wrapped errors, but it doesn't seem a problem.

ale64bit commented

2023-08-16 07:11:16 +00:00

Poster

Collaborator

ok. The original bug is not more, so I'm closing. We can address the design question elsewhere.

ale64bit closed this issue

2023-08-16 07:11:16 +00:00