Implement the errors.Unwrap interface as appropriate

[LukeShu]

Since Go 1.13, implementations of error that wrap another error should implement an Unwrap() error method, which will make the error be usable with the errors.Is, errors.As, and errors.Unwrap inspection functions.

There is an existing ticket about supporting error unwrapping (#2934) but most of what is being talked about in that ticket is about wrapping for gRPC statuses being sent over the network. This PR doesn't deal with that. It just deals with implementing Wrap on local error types.

It is tempting to also search for fmt.Errorf("whatever: %v", err) and change the %v to %w, but (1) that would have required either depending on Go 1.13 (compared to the current 1.11) or replacing fmt.Errorf with golang.org/x/xerrors.Errorf, and (2) seemed tedious and I didn't really want to put in the effort. The existing PR covers the errors that I happen to care about.

As an example of what this allows: If I dial an AF_UNIX socket that doesn't exist, (conn, err := grpc.DialContext(ctx, "unix:/path/to/file.sock", ...)) I can now check for that condition with errors.Is(err, os.ErrNotExist).

RELEASE NOTES:

• Custom error types now implement the interface for errors.Unwrap as appropriate.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Luke Shumaker (c1fb398, 5ef87f3)

[irl-segfault]

Nice. Looking forward to this being the default error treatment so that we can always leverage .Is and .As in error handling.

Team lead disagrees with idea.

[dfawley]

There is an existing ticket about supporting error unwrapping (#2934) but most of what is being talked about in that ticket is about wrapping for gRPC statuses being sent over the network

Correction: #2934 is about wrapping Status errors inside other errors.

I'm not sure this is something we want. The errors coming back from gRPC (Dial* and RPCs) should be status.Status errors, and they should be self-sufficient (i.e. not wrap other errors). In your example, you should be getting status.Convert(err).Code() == codes.Unavailable. Is that not the case / does that not work?

[LukeShu]

@dfawley wrote:

The errors coming back from gRPC (Dial* and RPCs) should be status.Status errors, and they should be self-sufficient (i.e. not wrap other errors). In your example, you should be getting status.Convert(err).Code() == codes.Unavailable. Is that not the case / does that not work?

That is not the case / that does not work.

Given the code

_, err := grpc.Dial("unix:/does/not/exist.sock",
	grpc.WithInsecure(),
	grpc.WithBlock(),
	grpc.FailOnNonTempDialError(true),
)

err will be

err := transport.ConnectionError{ // google.golang.org/grpc/internal/transport
	Desc: "transport: error while dialing: dial unix /does/not/exist.sock: connect: no such file or directory",
	temp: false,
	err: &net.OpError{
		Op:     "dial",
		Net:    "unix",
		Source: nil,
		Addr: &net.UnixAddr{
			Name: "/does/not/exist.sock",
			Net:  "unix",
		},
		Err: syscall.ENOENT,
	},
}

Because transport.ConnectionError does not implement GRPCStatus, status.Convert→status.FromError will hit the fallback return New(codes.Unknown, err.Error()), false case. And so status.Convert(err).Code() will be codes.Unknown.

[LukeShu]

@dfawley wrote:

I'm not sure this is something we want. The errors coming back from gRPC (Dial* and RPCs) should be status.Status errors, and they should be self-sufficient (i.e. not wrap other errors).

I'm not sure I agree.

---

The google.golang.org/grpc/status documentation reads:

Package status implements errors returned by gRPC. These errors are serialized and transmitted on the wire between server and client, and allow for additional data to be transmitted via the Details field in the status proto.

It does not speak of synthesizing statuses for errors not returned over gRPC over the wire. To use statuses that way would be surprising and unexpected to current users.

---

In HTTP client libraries we don't represent network errors as HTTP 500 responses; we differentiet between protocol-errors and errors-sent-over-the-protocol. Between in-band and out-of-band errors. I believe that gRPC libraries should follow the same principle.

[LukeShu]

Also note that in the case of ConnectionError being able to extract that underlying error is already supported functionality; just via the non-standard Origin method. All this PR is doing for that type is adding an Unwrap "alias" for it (actually they behave differently for nil), to conform to the standard interface for errors.Unwrap.

[dfawley]

Note that Dial shouldn't actually be blocking or error (except for things like target parse errors, illegal DialOption values, etc). That's not how client creation works in other languages. In other languages, you create the client and then make RPCs. If the client didn't connect to the server for whatever reason, then you get UNAVAILABLE RPC errors. (This is the basis behind #1786.)

IMO, considering that, we should really be returning only status errors from Dial for the things that are connection related. However, I may be convinced that #3616 (wrapping other errors inside status errors that don't go over the wire) should happen. gRPC-Java apparently does something similar to this and it seems to work fine for them.

Because transport.ConnectionError does not implement GRPCStatus, status.Convert→status.FromError will hit the fallback return New(codes.Unknown, err.Error()), false case. And so status.Convert(err).Code() will be codes.Unknown.

So maybe this is what we can do for now? Implement GRPCStatus in these errors so they can be converted to status errors, but also can be inspected and unwrapped otherwise?

[github-actions]

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.