server: Convert all non-status errors to codes.Unknown

[jeanbza]

This PR converts all non-status errors to UKNOWN, rather than trying to maintain a complex mapping of Go/network-specific codes to canonical gRPC codes.

Was waffling on whether the os.IsExist(err) and os.IsNotExist(err) errors should be Unknown, but went with internal. Let me know if y'all think any of these could be amended

[dfawley]

This whole function is a bad idea.

We're attempting to interpret what the user gave us, but they are supposed to return a status error to us, and nothing else. We should delete this function entirely, and where it's called, replace it with codes.Unknown.

This is a minor behavior change. I believe it should be OK. Can you also update the doc strings on StreamHandler and UnaryHandler to document that the error returned should be produced from the status package?

[jeanbza]

I'm on it!

[dfawley]

This is something users implement (their service handler). So:

"are compatible with" -> "should be produced by"

Also, please add something like: "Otherwise, the status code returned by the RPC will be codes.Unknown."

(same for StreamHandler)

EDIT: added "by the RPC"

[jeanbza]

👍 on 'produced by'.

However, regarding the second part - I'm confused by the "otherwise". Aren't all the errors (including the codes.Unknown ones I touched) "produced" by the status package via status.New? As in, in the first sentence you talk about errors, but the second you talk about codes.

[dfawley]

This may clarify (add to godoc string too?):

The error returned by this handler determines the status code and message of the RPC. If the error was produced by the status package, the code and message encoded in the error will be used directly. Otherwise, the status code will be UNKNOWN and the status message will be the string returned by error's Error method.

[jeanbza]

Ahh ok - I think I see what you're going for. Would something like this make sense?

All errors returned by UnaryHandler will either be produced by the status package, or wrapped in a status.Status with codes.Unknown as the status code and err.Error() as the status message.

^ trying to condense

Otherwise, I'm happy to write it as you have it!

[dfawley]

All errors returned by UnaryHandler will either be produced by the status package,
or wrapped in a status.Status with codes.Unknown as the status code and err.Error()
as the status message.

Maybe I wasn't clear on what this interface is for. A UnaryHandler is something the user implements (it's their server's RPC handler), and we call. This doc string should tell them what they need to do. If they return another error type, that's fine, but we'll turn their error into an UNKNOWN status code for the RPC when we send the trailers back to the client. This does not describe types of things we will return from the function. (Your version seems to be describing something we are in control of.) How about this slight modification:

All errors returned by a UnaryHandler implementation should be produced by the status package, or else gRPC will use codes.Unknown as the status code and err.Error() as the status message of the RPC.

Maybe an additional sentence like "a nil error indicates success", even though it's obvious, since the above may lead someone to think they always have to return some error, even on successes, or else they'll get an UNKNOWN error code. Or reword the first sentence to "If a UnaryHandler returns an error, it should be produced by ....". (?)

[jeanbza]

A UnaryHandler is something the user implements (it's their server's RPC handler), and we call

AH shoot that's the part I was entirely missing. Thanks very much for the explanation! Incoming update.