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

Add a register, so that the gRPC service can be invoked in-process to provide a HTTP server. #947

Merged
merged 14 commits into from
Aug 28, 2019

Conversation

hb-chen
Copy link
Contributor

@hb-chen hb-chen commented Jun 14, 2019

sometimes we don't need the HTTP gateway to be a RPC.
just convert the gRPC service to an HTTP service.
this reduces one remote call.

example code

func main() {
	s := grpc.NewServer(
	)
	srv := service{}
	pb.RegisterExampleServiceServer(s, &srv)

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

	mux := runtime.NewServeMux()
	err := pb.RegisterExampleServiceHandlerServer(ctx, mux, &srv)
	if err != nil {
		log.Panic(err)
	}

	if err := http.ListenAndServe(serveAddr, mux); err != nil {
		log.Fatalf("http failed to serve: %v", err)
	}
}

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@johanbrandhorst
Copy link
Collaborator

Hi @hb-chen! Thanks for your PR. This is an interesting new feature! You'll need to regenerate all the example files with this template change: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/CONTRIBUTING.md#i-want-to-regenerate-the-files-after-making-changes.

@hb-chen
Copy link
Contributor Author

hb-chen commented Jun 15, 2019

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@hb-chen
Copy link
Contributor Author

hb-chen commented Jun 17, 2019

When I try to support stream, I see the PR(grpc/grpc-go#906 (comment)) of grpc-go, will support for In-Process transport.
So waiting the feature and close this PR.

And now I use buffconn. grpc/grpc-go#1263 (comment)

func main() {
	s := grpc.NewServer(
	)
	srv := service{}
	pb.RegisterExampleServiceServer(s, &srv)

	bcLis := bufconn.Listen(256 * 1024)
	go s.Serve(bcLis)

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

	mux := runtime.NewServeMux()
	err := pb.RegisterExampleServiceHandlerFromEndpoint(
		ctx,
		mux,
		"",
		[]grpc.DialOption{
			grpc.WithContextDialer(func(ctx context.Context, s string) (net.Conn, error) {
				log.Printf("dial with s:%s", s)
				return bcLis.Dial()
			}),
		},
	)
	if err != nil {
		log.Panic(err)
	}

	if err := http.ListenAndServe(serveAddr, mux); err != nil {
		log.Fatalf("http failed to serve: %v", err)
	}
}

@hb-chen hb-chen closed this Jun 17, 2019
@johanbrandhorst
Copy link
Collaborator

@hb-chen The in-process transport is still some time away, and #952 shows there's still interest in this feature. Would you be interested to merge this anyway?

@hb-chen
Copy link
Contributor Author

hb-chen commented Jun 17, 2019

@hb-chen The in-process transport is still some time away, and #952 shows there's still interest in this feature. Would you be interested to merge this anyway?

Of course, I will continue this😁

@johanbrandhorst
Copy link
Collaborator

Seems like the latest version of the generated files have some compilation errors - I assume this is due to a small problem in the template. Could you take another look please?

@nicklofaso
Copy link

+1 for this feature. Much appreciated

@rittneje
Copy link

+1
We would be fine even if this new function just didn’t get generated for interfaces using streams. 🙂

@johanbrandhorst
Copy link
Collaborator

@hb-chen I'd be happy to merge initial support for just unary requests. What do you think?

@hb-chen
Copy link
Contributor Author

hb-chen commented Jun 28, 2019

@hb-chen I'd be happy to merge initial support for just unary requests. What do you think?

I agree with you.
Streaming used less in gateway, and we can wait for gRPC support.

@rittneje
Copy link

@hb-chen Any progress on this?

@vipingoel
Copy link

+1
Much awaited change.

@hb-chen
Copy link
Contributor Author

hb-chen commented Jul 30, 2019

@hb-chen Any progress on this?

Just bazel check failed.
Or you can fork my branch, and build.

@achew22
Copy link
Collaborator

achew22 commented Jul 30, 2019

@hb-chen, thanks so much for doing this! Looks like I need to push an update to the rules_go stuff to make Bazel work. One moment please. Would you be willing to wait for that PR (I'll mention this one in it) and then rebase off master?

@achew22
Copy link
Collaborator

achew22 commented Jul 30, 2019

@hb-chen, if you could rebase off of master I think this will pass CI. Thanks so much!

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@hb-chen hb-chen reopened this Jul 30, 2019
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

protoc-gen-grpc-gateway/gengateway/template.go Outdated Show resolved Hide resolved

client := New{{$svc.GetName}}Client(conn)
{{end}}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this newline before the {{end}} to avoid a newline at the top of non streaming services.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On line 579 have streaming condition {{if eq $streaming 1}}.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we decided not to support streaming endpoints fort now, can we remove this?

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the streaming specific code until it's implemented by gRPC.


client := New{{$svc.GetName}}Client(conn)
{{end}}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we decided not to support streaming endpoints fort now, can we remove this?

// Register{{$svc.GetName}}{{$.RegisterFuncSuffix}}Server registers the http handlers for service {{$svc.GetName}} to "mux".
// UnaryRPC :call {{$svc.GetName}}Server directly.
// StreamingRPC :currently unsupported pending https://github.com/grpc/grpc-go/issues/906.
// If the gateway proto have stream must add DialOption grpc.WithContextDialer. e.g.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the comments below this line.

@johanbrandhorst
Copy link
Collaborator

The new template changes look great, just a quick thought; do you think we could add a test case to the integration tests that calls the unary methods? It shouldn't be too hard, hopefully. Just a sanity test that the new functionality works.

@johanbrandhorst
Copy link
Collaborator

@hb-chen I don't want to lose sight of this, would you be able to write the tests or would you like to merge this as is?

@hb-chen
Copy link
Contributor Author

hb-chen commented Aug 19, 2019

@hb-chen I don't want to lose sight of this, would you be able to write the tests or would you like to merge this as is?

tks, I will write tests.

@johanbrandhorst
Copy link
Collaborator

OK I see we have some tests, I think we can merge this!

@johanbrandhorst johanbrandhorst merged commit e70d128 into grpc-ecosystem:master Aug 28, 2019
@hb-chen
Copy link
Contributor Author

hb-chen commented Aug 30, 2019

func Register{{$svc.GetName}}{{$.RegisterFuncSuffix}}Server(ctx context.Context, mux *runtime.ServeMux, server {{$svc.GetName}}Server, opts []grpc.DialOption) error

Oops, the func have an error, opts []grpc.DialOption is unused.
It used for test/bufconn mode, but I forgot to remove it.
remove it and create new pr?

@johanbrandhorst
Copy link
Collaborator

Hm, it would be a backward compatibility breaking change at this point... but it's a bit nasty to require something that isn't used. Make a PR and let's merge it quickly 😅.

@achew22
Copy link
Collaborator

achew22 commented Aug 30, 2019

+1 if we do it quickly I think I'm okay breaking

@rittneje
Copy link

@johanbrandhorst It looks like @hb-chen's second PR got merged, so is this issue now officially closed?

@johanbrandhorst
Copy link
Collaborator

Did you mean another issue? This is a pull request, and it is merged already.

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

Successfully merging this pull request may close these issues.

7 participants