refactor mux.go

[vaporz]

Code duplication is not good.

[yugui]

Thank you for your contribution. That's a very good point.
It's definitely the time to simplify the error handling flow.

This PR itself looks good to me. But let me

• summarize the current status of the handlers,
• confirm the intention of this PR, and
• ask some questions to simplify the mechanism even more in a later PR, @vaporz and @kazegusuri.

I'd really appreciate it if you give me some feedback.

The current status

• var HTTPError func(context.Context, *ServeMux, Marshaler, http.ResponseWriter, *http.Request, error)

  • Responsible for reporting runtime errors coming from the remote gRPC service, gRPC framework or request/response transformation in the gateway.
  • Need to return an HTTP status code corresponding to the given gRPC error.
  • It should respect custom marshalers because the error can happen at runtime.
  • It depend on a context.Context

• func DefaultHTTPError
  Default implementation of HTTPError

• var OtherErrorHandler func(http.ResponseWriter, r *http.Request, msg string, code int)

  • Responsible for reporting errors caused by wrong implementation of client stub.
  • Cannot assume that the given status code has a corresponding gRPC error code.
  • Usually the error won't happen at runtime as far as the client implementation is consistent to the swagger spec of the service.
  • It does not depend on context.Context for historical reason and compatibility.
    • It might be the time to rethink, taking several factors into account:
      • Now there's a context everywhere with request.Context.
      • Use status package for error and introduce WithProtoErrorHandler option #378 (comment)

        This also unifies errors returned by gRPC server and errors happened inside grpc-gateway, which helps error handling in client.

• func DefaultOtherErrorHandler
  Default implementation of OtherErrorHandler. Just calls http.Error.

  • It does not depend on the marshaler because simpler is better to reduce the number of assumptions we rely on in such an exceptional case.

• var ServeMux.protoErrorHandler func(context.Context, *ServeMux, Marshaler, http.ResponseWriter, *http.Request, error)

  • An customizable ServeMux option which overrides HTTPError and OtherErrorHandler.
  • It globally updates HTTPError (and OtherErrorHandler). So multiple instances of ServeMux with different protoErrorHandler cannot safely coexist.
  • Recognizes the new error reporting mechanism with status.Status in gRPC.

• func DefaultHTTPProtoErrorHandler

  • An implementation of protoErrorHandler. Almost a modified copy of DefaultHTTPError.

This PR

And this PR is trying to dedupe the implementation by adding two wrapper functions.

• func (s *ServeMux) executeErrorHandler(ctx context.Context, w http.ResponseWriter, r *http.Request, protoErrorCode codes.Code, protoErrorMsg string, otherStatusCode int, otherErrorMsg string)

  • a wrapper of the overriding mechanism of OtherErrorHandler by ServeMux.protoErrorHandler.
  • uses proto version of error representations if possible, falls back to the plain HTTP version of error representations if otherwise.

• func executeProtoErrorHandler(ctx context.Context, s *ServeMux, w http.ResponseWriter, r *http.Request, code codes.Code, msg string)

  • a wrapper of HTTPError which determines an appropriate marshaler for it.

Questions

• @kazegusuri, protoErrorHandler returns 501 Not Implemented instead of 405 Method Not Allowed when the given request method is not allowed (defined) on the resource. Is it really good.
• @kazegusuri, Was it the reason why you kept DefaultHTTPProtoErrorHandler separated from DefaulHTTPError that DefaultHTTPProtoErrorHandler returns a different format of response body from DefaultHTTPError?
• @vaporz, The responsibility of executeErrorHandler would be unclear if we only take a look at its signature. Is there any way to simplify it even more? It is totally fine to check it in as is, though.

[vaporz]

hi, @yugui , thanks for your comment.
It took me some time to figure out the relationship between HTTPError, OtherErrorHandler and serveMux.protoErrorHandler.
Now I'm working on a new commit for this PR, a better(hopefully) solution.

[kazegusuri]

@kazegusuri, protoErrorHandler returns 501 Not Implemented instead of 405 Method Not Allowed when the given request method is not allowed (defined) on the resource. Is it really good.

Because gRPC spec says Method not found at server returns UNIMPLEMENTED and the code means 501 Not Implemented in grpc-gateway. However gRPC spec defines HTTP to gRPC Status Code Mapping and it says UNIMPLEMENTED matches 404 Not Found.

Was it the reason why you kept DefaultHTTPProtoErrorHandler separated from DefaulHTTPError

For the first part, actually there is no strong reason, but I wanted to use an option in order to change the behavior of the error handler instead of modify the variable directly.

DefaultHTTPProtoErrorHandler returns a different format of response body from DefaultHTTPError?

The ProtoErrorHandler returns error as status proto, so client can parse the response with the proto.

[vaporz]

Committed, changes are:
1, When NewServeMux, if serveMux.protoErrorHandler is nil, then OtherErrorHandler is adapted for ProtoErrorHandlerFunc, and assigned to serveMux.protoErrorHandler.
2, Return 404/405, instead of 501.

However, I believe this commit is just a start of "error handling mechanism" overhaul.
Here're my thoughts in this topic.

I don't think a HTTP server like grpc-gateway should new a grpc.Status as error and pass them internally. Grpc-gateway should new a gatewayError which contains a grpc.Status(returned by grpc server), and use it internally.
As comments on package status says:

Package status implements errors returned by gRPC. These errors are serialized and transmitted on the wire between server and client,

"between server and client"

I suggest grpc-gateway should transfer grpc.Status error returned by grpc stubs immediately to a gatewayError with HTTP code, message and grpc.Status, then use gatewayError instead of a grpc.Status error.
Whenever an error is returned in grpc-gateway, return a gatewayError.
For example, in *.pb.gw.go, pass a gatewayError to runtime.HTTPError(currently is grpc.Status).
gatewayError may look like this:

type gatewayError struct {
HTTPCode int,
msg string,
status grpc.Status,
}