Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request context not being used #470

Closed
paulbdavis opened this issue Oct 5, 2017 · 13 comments
Closed

Request context not being used #470

paulbdavis opened this issue Oct 5, 2017 · 13 comments

Comments

@paulbdavis
Copy link

paulbdavis commented Oct 5, 2017

I am trying to add values to the context being passed into the service methods using the request_context option when generating. The generated code seems to be doing the right thing, and the code for the runtime appears that it should do the right thing too, but I am not getting the modified context in the service handlers.

- $GOPATH/src/test
  - pb
    test.proto
  main.go

test.proto

syntax = "proto3";
package pb;
import "google/api/annotations.proto";

message TestRequest {
  string data = 1;
}

message TestResponse {
  string data = 1;
}

service TestSvc {

  rpc GetTest(TestRequest) returns (TestResponse) {
    option (google.api.http) = {
      get: "/test",
    };
  }
  
}

main.go

package main

import (
	"github.com/grpc-ecosystem/grpc-gateway/runtime"
	"golang.org/x/net/context"
	"google.golang.org/grpc"
	"log"
	"net"
	"net/http"
	"test/grpc-gateway-context-issue/pb"
)

type ctxKey struct{}

func main() {

	ctx := context.Background()
	ctx, cancel := context.WithCancel(ctx)
	defer cancel()

	// start grpc service in background
	go startService()

	mux := runtime.NewServeMux()
	opts := []grpc.DialOption{grpc.WithInsecure()}
	err := pb.RegisterTestSvcHandlerFromEndpoint(ctx, mux, "localhost:10000", opts)
	if err != nil {
		log.Fatal(err)
	}

	handler := http.NewServeMux()

	handler.Handle("/", contextWrap(mux))

	if err := http.ListenAndServe(":8081", handler); err != nil {
		log.Fatal(err)
	}

}

func contextWrap(h http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
		req = req.WithContext(context.WithValue(req.Context(), ctxKey{}, "test"))
		log.Println("in wrapper, context value is", req.Context().Value(ctxKey{}))
		h.ServeHTTP(w, req)
	})
}

func startService() {
	lis, err := net.Listen("tcp", ":10000")
	if err != nil {
		log.Fatal(err)
	}
	s := grpc.NewServer()

	pb.RegisterTestSvcServer(s, &testSvc{})
	if err := s.Serve(lis); err != nil {
		log.Fatalf("While serving gRPC request: %v", err)
	}

}

type testSvc struct{}

func (svc *testSvc) GetTest(ctx context.Context, req *pb.TestRequest) (*pb.TestResponse, error) {

	log.Println("in service, context value is", ctx.Value(ctxKey{}))

	return &pb.TestResponse{
		Data: req.Data,
	}, nil
}

The proto file was compiled using protoc --go_out=plugins=grpc:pb --grpc-gateway_out=logtostderr=true,request_context=true:pb -I$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis -Ipb pb/test.proto

When I run this, and curl localhost:8081/test I would expect the server logs to print out

in wrapper, context value is test
in service, context value is test

But instead I get

in wrapper, context value is test
in service, context value is <nil>
@achew22
Copy link
Collaborator

achew22 commented Dec 9, 2017

Could you try recompiling the protoc with current head? This is now the default behavior so it should work. I'm going to close this preemptively but feel free to reopen if you're having issues still.

@achew22 achew22 closed this as completed Dec 9, 2017
@rinatio
Copy link

rinatio commented Feb 9, 2018

@achew22 Is it included in v1.3.1 release? Cannot make it working. I'm on the latest commit 6658b3a.

@phea
Copy link

phea commented Mar 12, 2018

I am experiencing the same problem with my project along with this basic example by @paulbdavis

2018/03/12 11:27:39 Starting service
2018/03/12 11:27:45 in wrapper, context value is test
2018/03/12 11:27:45 in service, context value is <nil>

[pd@XPS pb]$ protoc --version
libprotoc 3.5.1
[pd@XPS pb]$ go version
go version go1.10 linux/amd64 

@vilterp
Copy link

vilterp commented May 24, 2018

From looking at the generated code, it seems as though the context is lost because the generated HTTP handler doesn't directly call the service method — it calls it through the gRPC client, same as it would if it were making a network RPC.

Not sure how to pass the context across this boundary, other than to change the library to take the server objects directly when initializing a gateway (instead of a net.ClientConn) and then call the service methods directly, instead of through gRPC. That seems like a pretty drastic change though.

@vilterp
Copy link

vilterp commented May 24, 2018

Looks like you're supposed to solve this problem using gwruntime.WithMetadata(…), which passes some string-string key-value pairs through the gRPC boundary.

@achew22
Copy link
Collaborator

achew22 commented May 25, 2018

Huh, that is not what I would expect. If you have this flag set I would have expected it to reuse the context from the inbound http context in the grpc outbound context. Could you file a new issue documenting the issues you're having?

craig bot pushed a commit to cockroachdb/cockroach that referenced this issue May 31, 2018
26062: server, ui: logout r=vilterp a=vilterp

**implement logout endpoint on backend**

Because of grpc-ecosystem/grpc-gateway#470, had to do an end run around gRPC. We'll need to fix grpc-gateway to pass the context through eventually, but I don't want logout to be blocked on it.

**implement logout button on frontend**

Makes an RPC to invalidate the session. If successful, reloads the page.

Fixes #25784 
Produces awesomeness when combined with #26053 

Co-authored-by: Pete Vilter <[email protected]>
@irl-segfault
Copy link

uh, did this ever get fixed?

@johanbrandhorst
Copy link
Collaborator

Pretty sure the current gateway respects the request context. Do you have an issue that implies it does not?

@mteodor
Copy link

mteodor commented Jul 29, 2022

can you point me to the solution, I have the same problem

@koo04
Copy link

koo04 commented Nov 29, 2023

Today, I ran into this as well. I am able to replicate it with the exact same pattern as the OP.

@johanbrandhorst
Copy link
Collaborator

Sorry to hear this is still a problem for you. Could you provide a simple example repo that reproduces the issue?

@koo04
Copy link

koo04 commented Dec 2, 2023

Here is my rendition of the OP's setup. It is doing all the exact same things in a more real-world pattern. Check out how the context does not contain the test string when getting down to the endpoint.

https://github.com/koo04/gateway-test

@johanbrandhorst
Copy link
Collaborator

Thanks for the example repo, it made it much easier for me to understand the issue. The problem here is an incorrect assumption about the behavior of contexts in Go in general. Put simply: Go context values are not portable across the network. The repository establishes a gRPC connection between the gateway proxy and the gRPC server. The only way to transport metadata over the gRPC connection is using grpc metadata. The following changes, using the grpc metadata, allows the string value to be transported over the gRPC connection:

diff --git a/api/server.go b/api/server.go
index 50db0e5..121ea7a 100644
--- a/api/server.go
+++ b/api/server.go
@@ -10,6 +10,7 @@ import (
 	testv1 "github.com/koo04/gateway-test/internal/gen/proto/go/api/v1"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/credentials/insecure"
+	"google.golang.org/grpc/metadata"
 	"google.golang.org/protobuf/encoding/protojson"
 )
 
@@ -39,7 +40,18 @@ func Start() error {
 				},
 			}),
 		)
-		opts := []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())}
+		opts := []grpc.DialOption{
+			grpc.WithTransportCredentials(insecure.NewCredentials()),
+			grpc.WithUnaryInterceptor(
+				func(ctx context.Context, method string, req, reply any, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
+					v := ctx.Value(ContextTestString{})
+					if v != nil {
+						ctx = metadata.AppendToOutgoingContext(ctx, "test-string", v.(string))
+					}
+					return invoker(ctx, method, req, reply, cc, opts...)
+				},
+			),
+		}
 
 		// register contact types
 		if err := testv1.RegisterTestAPIServiceHandlerFromEndpoint(ctx, mux, ":9000", opts); err != nil {
@@ -69,7 +81,17 @@ func Start() error {
 		return err
 	}
 
-	s := grpc.NewServer()
+	s := grpc.NewServer(
+		grpc.UnaryInterceptor(
+			func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp any, err error) {
+				md, ok := metadata.FromIncomingContext(ctx)
+				if ok {
+					ctx = context.WithValue(ctx, ContextTestString{}, md.Get("test-string")[0])
+				}
+				return handler(ctx, req)
+			},
+		),
+	)
 	testv1.RegisterTestAPIServiceServer(s, srv)
 
 	slog.Info("GRPC server listening on port :9000")

Note that only values that can be converted into string values and back can be transported over the grpc connection.

Another alternative is to skip the gRPC server altogether and use the RegisterFooServiceHandlerServer instead of gRPC:

diff --git a/api/server.go b/api/server.go
index 50db0e5..517ae92 100644
--- a/api/server.go
+++ b/api/server.go
@@ -3,13 +3,10 @@ package api
 import (
 	"context"
 	"log/slog"
-	"net"
 	"net/http"

 	"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
 	testv1 "github.com/koo04/gateway-test/internal/gen/proto/go/api/v1"
-	"google.golang.org/grpc"
-	"google.golang.org/grpc/credentials/insecure"
 	"google.golang.org/protobuf/encoding/protojson"
 )

@@ -19,61 +16,40 @@ var srv *server = &server{}

 func Start() error {
 	slog.Info("Starting Service")
-
-	// start the REST proxy endpoints
-	go func() {
-		ctx := context.Background()
-		ctx, cancel := context.WithCancel(ctx)
-		defer cancel()
-
-		mux := runtime.NewServeMux(
-			runtime.WithMarshalerOption(runtime.MIMEWildcard, &runtime.HTTPBodyMarshaler{
-				Marshaler: &runtime.JSONPb{
-					MarshalOptions: protojson.MarshalOptions{
-						UseProtoNames:   true,
-						EmitUnpopulated: true,
-					},
-					UnmarshalOptions: protojson.UnmarshalOptions{
-						DiscardUnknown: true,
-					},
+	ctx := context.Background()
+
+	// register contact types
+	mux := runtime.NewServeMux(
+		runtime.WithMarshalerOption(runtime.MIMEWildcard, &runtime.HTTPBodyMarshaler{
+			Marshaler: &runtime.JSONPb{
+				MarshalOptions: protojson.MarshalOptions{
+					UseProtoNames:   true,
+					EmitUnpopulated: true,
 				},
-			}),
-		)
-		opts := []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())}
-
-		// register contact types
-		if err := testv1.RegisterTestAPIServiceHandlerFromEndpoint(ctx, mux, ":9000", opts); err != nil {
-			slog.Error("failed to register handlers", "error", err)
-			return
-		}
-
-		httpmux := http.NewServeMux()
+				UnmarshalOptions: protojson.UnmarshalOptions{
+					DiscardUnknown: true,
+				},
+			},
+		}),
+	)
+	testv1.RegisterTestAPIServiceHandlerServer(ctx, mux, srv)

-		httpmux.Handle("/", mux)
+	httpmux := http.NewServeMux()

-		s := &http.Server{
-			Addr:    ":8000",
-			Handler: testHandler(httpmux),
-		}
+	httpmux.Handle("/", mux)

-		slog.Info("REST server listening on port :8000")
-		if err := s.ListenAndServe(); err != nil {
-			slog.Error("failed to serve proxy", "error", err)
-			return
-		}
-	}()
+	s := &http.Server{
+		Addr:    ":8000",
+		Handler: testHandler(httpmux),
+	}

-	lis, err := net.Listen("tcp", ":9000")
-	if err != nil {
-		slog.Error("failed to listen", "error", err)
+	slog.Info("REST server listening on port :8000")
+	if err := s.ListenAndServe(); err != nil {
+		slog.Error("failed to serve proxy", "error", err)
 		return err
 	}

-	s := grpc.NewServer()
-	testv1.RegisterTestAPIServiceServer(s, srv)
-
-	slog.Info("GRPC server listening on port :9000")
-	return s.Serve(lis)
+	return nil
 }

 type ContextTestString struct{}

Note that this doesn't execute any of the usual gRPC logic such as interceptors. It also doesn't support streaming.

I'll close this issue as the reproducible example as shown is not due to an issue in the gateway. If anyone has a different example I'm happy to open it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants