From 5bf43f0bc8e3659add77f8ff57299671fe2f78e0 Mon Sep 17 00:00:00 2001 From: Akshay Shah Date: Mon, 9 Aug 2021 22:31:30 -0700 Subject: [PATCH] Make Handler an http.Handler We can reduce the number of new ideas this package introduces if we make our Handler an actual http.Handler. This has a number of nice effects - we can return the concrete type instead of an anonymous http.HandlerFunc in a few places. Across the board, this commit also changes functions to return the concrete http.ServeMux type instead of the http.Handler interface. --- bad_route.go | 9 ++---- cmd/protoc-gen-go-rerpc/rerpc.go | 7 ++--- handler.go | 35 ++++++++++++--------- health.go | 27 +++++++--------- internal/crosstest/v1test/cross_rerpc.pb.go | 24 +++++++------- internal/ping/v1test/ping_rerpc.pb.go | 29 +++++++++-------- reflection.go | 21 +++++++------ 7 files changed, 76 insertions(+), 76 deletions(-) diff --git a/bad_route.go b/bad_route.go index 02a1762d..472b2762 100644 --- a/bad_route.go +++ b/bad_route.go @@ -2,7 +2,6 @@ package rerpc import ( "context" - "net/http" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/emptypb" @@ -12,9 +11,10 @@ import ( // standard library's http.StatusNotFound. To be fully compatible with the // Twirp specification, mount this handler at the root of your API (so that it // handles any requests for invalid protobuf methods). -func NewBadRouteHandler(opts ...HandlerOption) http.Handler { - h := NewHandler( +func NewBadRouteHandler(opts ...HandlerOption) *Handler { + return NewHandler( "", "", "", // protobuf method, service, package names + func() proto.Message { return &emptypb.Empty{} }, // unused req msg func(ctx context.Context, _ proto.Message) (proto.Message, error) { path := "???" if md, ok := HandlerMeta(ctx); ok { @@ -24,7 +24,4 @@ func NewBadRouteHandler(opts ...HandlerOption) http.Handler { }, opts..., ) - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - h.Serve(w, r, &emptypb.Empty{}) - }) } diff --git a/cmd/protoc-gen-go-rerpc/rerpc.go b/cmd/protoc-gen-go-rerpc/rerpc.go index 5d3d358c..1c424f3b 100644 --- a/cmd/protoc-gen-go-rerpc/rerpc.go +++ b/cmd/protoc-gen-go-rerpc/rerpc.go @@ -220,7 +220,7 @@ func serverConstructor(g *protogen.GeneratedFile, service *protogen.Service, nam deprecated(g) } g.P("func New", service.GoName, "HandlerReRPC(svc ", name, ", opts ...", rerpcPackage.Ident("HandlerOption"), - ") (string, ", httpPackage.Ident("Handler"), ") {") + ") (string, *", httpPackage.Ident("ServeMux"), ") {") g.P("mux := ", httpPackage.Ident("NewServeMux"), "()") g.P() for _, method := range unaryMethods(service) { @@ -230,6 +230,7 @@ func serverConstructor(g *protogen.GeneratedFile, service *protogen.Service, nam g.P(`"`, method.Desc.FullName(), `", // fully-qualified protobuf method`) g.P(`"`, service.Desc.FullName(), `", // fully-qualified protobuf service`) g.P(`"`, service.Desc.ParentFile().Package(), `", // fully-qualified protobuf package`) + g.P("func() ", protoPackage.Ident("Message"), " { return &", method.Input.GoIdent, "{} }, // request msg constructor") g.P(rerpcPackage.Ident("Func"), "(func(ctx ", contextPackage.Ident("Context"), ", req ", protoPackage.Ident("Message"), ") (", protoPackage.Ident("Message"), ", error) {") @@ -245,9 +246,7 @@ func serverConstructor(g *protogen.GeneratedFile, service *protogen.Service, nam g.P("}),") g.P("opts...,") g.P(")") - g.P(`mux.HandleFunc("/`, path, `", func(w `, httpPackage.Ident("ResponseWriter"), ", r *", httpPackage.Ident("Request"), ") {") - g.P(hname, ".Serve(w, r, &", method.Input.GoIdent, "{})") - g.P("})") + g.P(`mux.Handle("/`, path, `", `, hname, ")") g.P() } comment(g, "Respond to unknown protobuf methods with gRPC and Twirp's 404 equivalents.") diff --git a/handler.go b/handler.go index af43ee97..cadf823a 100644 --- a/handler.go +++ b/handler.go @@ -75,6 +75,7 @@ type Handler struct { methodFQN string serviceFQN string packageFQN string + newRequest func() proto.Message implementation Func // rawGRPC is used only for our hand-rolled reflection handler, which needs // bidi streaming @@ -90,13 +91,24 @@ type Handler struct { } // NewHandler constructs a Handler. The supplied method, service, and package -// must be fully-qualified protobuf identifiers. +// must be fully-qualified protobuf identifiers, and the newRequest constructor +// must be safe to call concurrently. // // For example, a handler might have method "acme.foo.v1.FooService.Bar", -// service "acme.foo.v1.FooService", and package "acme.foo.v1". Remember that -// NewHandler is usually called from generated code - most users won't need to -// deal with protobuf identifiers directly. -func NewHandler(methodFQN, serviceFQN, packageFQN string, impl Func, opts ...HandlerOption) *Handler { +// service "acme.foo.v1.FooService", and package "acme.foo.v1". In that case, +// the newRequest constructor would be: +// func() proto.Message { +// return &foopb.BarRequest{} +// } +// +// Remember that NewHandler is usually called from generated code - most users +// won't need to deal with protobuf identifiers directly. +func NewHandler( + methodFQN, serviceFQN, packageFQN string, + newRequest func() proto.Message, + impl Func, + opts ...HandlerOption, +) *Handler { var cfg handlerCfg for _, opt := range opts { opt.applyToHandler(&cfg) @@ -108,19 +120,14 @@ func NewHandler(methodFQN, serviceFQN, packageFQN string, impl Func, opts ...Han methodFQN: methodFQN, serviceFQN: serviceFQN, packageFQN: packageFQN, + newRequest: newRequest, implementation: impl, config: cfg, } } -// Serve executes the handler, much like the standard library's http.Handler. -// Unlike http.Handler, it requires a pointer to the generated request struct. -// See the internal/ping/v1test package for an example of how this code is used -// in reRPC's generated code. -// -// As long as the caller allocates a new request struct for each call, this -// method is safe to call concurrently. -func (h *Handler) Serve(w http.ResponseWriter, r *http.Request, req proto.Message) { +// ServeHTTP implements http.Handler. +func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // To ensure that we can re-use connections, always consume and close the // request body. defer r.Body.Close() @@ -250,7 +257,7 @@ func (h *Handler) Serve(w http.ResponseWriter, r *http.Request, req proto.Messag } else { implementation = h.implementationGRPC(w, r, spec) } - res, err := h.wrap(implementation)(ctx, req) + res, err := h.wrap(implementation)(ctx, h.newRequest()) h.writeResult(r.Context(), w, spec, res, err) } diff --git a/health.go b/health.go index f1e06020..5ba262a7 100644 --- a/health.go +++ b/health.go @@ -2,7 +2,6 @@ package rerpc import ( "context" - "fmt" "net/http" "google.golang.org/protobuf/proto" @@ -63,17 +62,19 @@ func NewChecker(reg *Registrar) func(context.Context, string) (HealthStatus, err func NewHealthHandler( check func(context.Context, string) (HealthStatus, error), opts ...HandlerOption, -) (string, http.Handler) { +) (string, *http.ServeMux) { const packageFQN = "grpc.health.v1" const serviceFQN = packageFQN + ".Health" const checkFQN = serviceFQN + ".Check" const watchFQN = serviceFQN + ".Watch" + const servicePath = "/" + serviceFQN + "/" + const checkPath = servicePath + "Check" + const watchPath = servicePath + "Watch" mux := http.NewServeMux() checkHandler := NewHandler( - checkFQN, - serviceFQN, - packageFQN, + checkFQN, serviceFQN, packageFQN, + func() proto.Message { return &healthpb.HealthCheckRequest{} }, func(ctx context.Context, req proto.Message) (proto.Message, error) { typed, ok := req.(*healthpb.HealthCheckRequest) if !ok { @@ -94,22 +95,18 @@ func NewHealthHandler( }, opts..., ) - mux.HandleFunc(fmt.Sprintf("/%s/Check", serviceFQN), func(w http.ResponseWriter, r *http.Request) { - checkHandler.Serve(w, r, &healthpb.HealthCheckRequest{}) - }) + mux.Handle(checkPath, checkHandler) watch := NewHandler( - watchFQN, - serviceFQN, - packageFQN, + watchFQN, serviceFQN, packageFQN, + func() proto.Message { return &healthpb.HealthCheckRequest{} }, func(ctx context.Context, req proto.Message) (proto.Message, error) { return nil, errorf(CodeUnimplemented, "reRPC doesn't support watching health state") }, opts..., ) - mux.HandleFunc(fmt.Sprintf("/%s/Watch", serviceFQN), func(w http.ResponseWriter, r *http.Request) { - watch.Serve(w, r, &healthpb.HealthCheckRequest{}) - }) + mux.Handle(watchPath, watch) + mux.Handle("/", NewBadRouteHandler(opts...)) - return fmt.Sprintf("/%s/", serviceFQN), mux + return servicePath, mux } diff --git a/internal/crosstest/v1test/cross_rerpc.pb.go b/internal/crosstest/v1test/cross_rerpc.pb.go index 2127a595..bf5db645 100644 --- a/internal/crosstest/v1test/cross_rerpc.pb.go +++ b/internal/crosstest/v1test/cross_rerpc.pb.go @@ -100,13 +100,14 @@ type CrossServiceReRPC interface { // NewCrossServiceHandlerReRPC wraps the service implementation in an HTTP // handler. It returns the handler and the path on which to mount it. -func NewCrossServiceHandlerReRPC(svc CrossServiceReRPC, opts ...rerpc.HandlerOption) (string, http.Handler) { +func NewCrossServiceHandlerReRPC(svc CrossServiceReRPC, opts ...rerpc.HandlerOption) (string, *http.ServeMux) { mux := http.NewServeMux() ping := rerpc.NewHandler( - "internal.crosstest.v1test.CrossService.Ping", // fully-qualified protobuf method - "internal.crosstest.v1test.CrossService", // fully-qualified protobuf service - "internal.crosstest.v1test", // fully-qualified protobuf package + "internal.crosstest.v1test.CrossService.Ping", // fully-qualified protobuf method + "internal.crosstest.v1test.CrossService", // fully-qualified protobuf service + "internal.crosstest.v1test", // fully-qualified protobuf package + func() proto.Message { return &PingRequest{} }, // request msg constructor rerpc.Func(func(ctx context.Context, req proto.Message) (proto.Message, error) { typed, ok := req.(*PingRequest) if !ok { @@ -120,14 +121,13 @@ func NewCrossServiceHandlerReRPC(svc CrossServiceReRPC, opts ...rerpc.HandlerOpt }), opts..., ) - mux.HandleFunc("/internal.crosstest.v1test.CrossService/Ping", func(w http.ResponseWriter, r *http.Request) { - ping.Serve(w, r, &PingRequest{}) - }) + mux.Handle("/internal.crosstest.v1test.CrossService/Ping", ping) fail := rerpc.NewHandler( - "internal.crosstest.v1test.CrossService.Fail", // fully-qualified protobuf method - "internal.crosstest.v1test.CrossService", // fully-qualified protobuf service - "internal.crosstest.v1test", // fully-qualified protobuf package + "internal.crosstest.v1test.CrossService.Fail", // fully-qualified protobuf method + "internal.crosstest.v1test.CrossService", // fully-qualified protobuf service + "internal.crosstest.v1test", // fully-qualified protobuf package + func() proto.Message { return &FailRequest{} }, // request msg constructor rerpc.Func(func(ctx context.Context, req proto.Message) (proto.Message, error) { typed, ok := req.(*FailRequest) if !ok { @@ -141,9 +141,7 @@ func NewCrossServiceHandlerReRPC(svc CrossServiceReRPC, opts ...rerpc.HandlerOpt }), opts..., ) - mux.HandleFunc("/internal.crosstest.v1test.CrossService/Fail", func(w http.ResponseWriter, r *http.Request) { - fail.Serve(w, r, &FailRequest{}) - }) + mux.Handle("/internal.crosstest.v1test.CrossService/Fail", fail) // Respond to unknown protobuf methods with gRPC and Twirp's 404 equivalents. mux.Handle("/", rerpc.NewBadRouteHandler(opts...)) diff --git a/internal/ping/v1test/ping_rerpc.pb.go b/internal/ping/v1test/ping_rerpc.pb.go index 6c5e00ba..2a4820a7 100644 --- a/internal/ping/v1test/ping_rerpc.pb.go +++ b/internal/ping/v1test/ping_rerpc.pb.go @@ -8,10 +8,11 @@ package pingpb import ( context "context" - rerpc "github.com/rerpc/rerpc" - proto "google.golang.org/protobuf/proto" http "net/http" strings "strings" + + rerpc "github.com/rerpc/rerpc" + proto "google.golang.org/protobuf/proto" ) // This is a compile-time assertion to ensure that this generated file and the @@ -100,13 +101,14 @@ type PingServiceReRPC interface { // NewPingServiceHandlerReRPC wraps the service implementation in an HTTP // handler. It returns the handler and the path on which to mount it. -func NewPingServiceHandlerReRPC(svc PingServiceReRPC, opts ...rerpc.HandlerOption) (string, http.Handler) { +func NewPingServiceHandlerReRPC(svc PingServiceReRPC, opts ...rerpc.HandlerOption) (string, *http.ServeMux) { mux := http.NewServeMux() ping := rerpc.NewHandler( - "internal.ping.v1test.PingService.Ping", // fully-qualified protobuf method - "internal.ping.v1test.PingService", // fully-qualified protobuf service - "internal.ping.v1test", // fully-qualified protobuf package + "internal.ping.v1test.PingService.Ping", // fully-qualified protobuf method + "internal.ping.v1test.PingService", // fully-qualified protobuf service + "internal.ping.v1test", // fully-qualified protobuf package + func() proto.Message { return &PingRequest{} }, // request msg constructor rerpc.Func(func(ctx context.Context, req proto.Message) (proto.Message, error) { typed, ok := req.(*PingRequest) if !ok { @@ -120,14 +122,13 @@ func NewPingServiceHandlerReRPC(svc PingServiceReRPC, opts ...rerpc.HandlerOptio }), opts..., ) - mux.HandleFunc("/internal.ping.v1test.PingService/Ping", func(w http.ResponseWriter, r *http.Request) { - ping.Serve(w, r, &PingRequest{}) - }) + mux.Handle("/internal.ping.v1test.PingService/Ping", ping) fail := rerpc.NewHandler( - "internal.ping.v1test.PingService.Fail", // fully-qualified protobuf method - "internal.ping.v1test.PingService", // fully-qualified protobuf service - "internal.ping.v1test", // fully-qualified protobuf package + "internal.ping.v1test.PingService.Fail", // fully-qualified protobuf method + "internal.ping.v1test.PingService", // fully-qualified protobuf service + "internal.ping.v1test", // fully-qualified protobuf package + func() proto.Message { return &FailRequest{} }, // request msg constructor rerpc.Func(func(ctx context.Context, req proto.Message) (proto.Message, error) { typed, ok := req.(*FailRequest) if !ok { @@ -141,9 +142,7 @@ func NewPingServiceHandlerReRPC(svc PingServiceReRPC, opts ...rerpc.HandlerOptio }), opts..., ) - mux.HandleFunc("/internal.ping.v1test.PingService/Fail", func(w http.ResponseWriter, r *http.Request) { - fail.Serve(w, r, &FailRequest{}) - }) + mux.Handle("/internal.ping.v1test.PingService/Fail", fail) // Respond to unknown protobuf methods with gRPC and Twirp's 404 equivalents. mux.Handle("/", rerpc.NewBadRouteHandler(opts...)) diff --git a/reflection.go b/reflection.go index a230fe17..27e5998f 100644 --- a/reflection.go +++ b/reflection.go @@ -83,24 +83,27 @@ func (r *Registrar) applyToHandler(cfg *handlerCfg) { // https://github.com/grpc/grpc-go/blob/master/Documentation/server-reflection-tutorial.md // https://github.com/grpc/grpc/blob/master/doc/server-reflection.md // https://github.com/fullstorydev/grpcurl -func NewReflectionHandler(reg *Registrar) (string, http.Handler) { +func NewReflectionHandler(reg *Registrar) (string, *http.ServeMux) { const packageFQN = "grpc.reflection.v1alpha" const serviceFQN = packageFQN + ".ServerReflection" - const methodFQN = serviceFQN + ".ServerReflectionInfo" + const method = "ServerReflectionInfo" + const methodFQN = serviceFQN + "." + method + const servicePath = "/" + serviceFQN + "/" + const methodPath = servicePath + method reg.register(serviceFQN) h := NewHandler( - methodFQN, - serviceFQN, - packageFQN, + methodFQN, serviceFQN, packageFQN, + func() proto.Message { return nil }, nil, // no unary implementation ServeTwirp(false), // no reflection in Twirp ) raw := &rawReflectionHandler{reg} h.rawGRPC = raw.rawGRPC - httpHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - h.Serve(w, r, nil) - }) - return "/grpc.reflection.v1alpha.ServerReflection/ServerReflectionInfo", httpHandler + + mux := http.NewServeMux() + mux.Handle(methodPath, h) + mux.Handle("/", NewBadRouteHandler()) + return servicePath, mux } type rawReflectionHandler struct {