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

allow storing alternate transport.ServerStream implementations in context #1904

Merged
merged 2 commits into from
Mar 21, 2018

Conversation

jhump
Copy link
Member

@jhump jhump commented Mar 7, 2018

Fixes #1802.

This allows handlers to be run from tests without requiring the entirety of the gRPC server machinery: a test can store a mock transport.ServerStream into the context and then call the handler.

It also enables alternate transport implementations.

…text -- for tests and for implementing alternate transports
@jhump
Copy link
Member Author

jhump commented Mar 7, 2018

@dfawley, might you have some time today or later this week to take a look at this (and at #1902)?

@jhump
Copy link
Member Author

jhump commented Mar 9, 2018

Ping?

@dfawley
Copy link
Member

dfawley commented Mar 9, 2018

A couple observations/quesitons:

  • The transport package is supposed to be grpc-internal-only; this would presumably expose an API for users?
  • If grpc-go natively had an in-process transport, would this still be useful?

@jhump
Copy link
Member Author

jhump commented Mar 9, 2018

The transport package is supposed to be grpc-internal-only; this would presumably expose an API for users?

In it's current form, it does -- the mechanism for storing a transport stream into context. But I think I can move stuff around so that is not the case (so that new exported API is in the main grpc package, not in transport).

If grpc-go natively had an in-process transport, would this still be useful?

I suppose it depends. If, in the process of creating an alternate transport, the gRPC library gained better abstractions that de-couple the interfaces from the existing singular implementations, then maybe! But if there were no additional abstractions provided for custom transports, then that would only help so much.

We also want this for an HTTP 1.1 transport implementation. And, generally, abstracting these details so that custom transports can be used is IMO very empowering to users.

Another argument for something like this is to resolve #1494, to make the interceptor model generally more coherent. As is, only streaming interceptors can actually intercept response metadata (both client and server sides). But this change makes it possible for a unary interceptor to do so -- by installing its own "transport stream" implementation in context that can intercept metadata and forward to the real stream. (Similarly, #1902 enables intercepting metadata for unary RPCs on the client side.)

@jhump
Copy link
Member Author

jhump commented Mar 11, 2018

I pushed a change so that new API is in the main grpc package (not in transport). This results in a change to the exported API of the transport package: no more StreamFromContext method. (If that package were not meant to be used by anything other than gRPC internals, I expect that is okay.)

@dfawley: Does this seem better?

@jhump
Copy link
Member Author

jhump commented Mar 15, 2018

ping

…ream (and corresponding interface); move to grpc package (out of transport package)
@jhump jhump force-pushed the jh/server-metadata-alt-transports branch from 41aaea6 to eff3003 Compare March 16, 2018 22:25
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Thanks, @jhump. I don't particularly like the way this is done, but I think it's the best we can do for now, and it is notable missing functionality for our interceptors.

When we address #1805, we'll be sure to make this use case "just work".

@dfawley dfawley merged commit 57640c0 into grpc:master Mar 21, 2018
@jhump
Copy link
Member Author

jhump commented Mar 21, 2018

Thank you @dfawley! I understand that it is not ideal. I do appreciate you merging it anyway.

@trusktr
Copy link

trusktr commented Aug 10, 2018

For anyone stumbling here, this commit perfectly describes how to fix undefined: transport.StreamFromContext (or why there's no bin file after go install): jfyne/docker-grpcwebproxy@da71230

For example, I had this problem with https://github.com/improbable-eng/grpc-web, and the following worked:

go get -u github.com/improbable-eng/grpc-web/go/grpcwebproxy
cd $GOPATH/src/github.com/improbable-eng/grpc-web
brew install dep # install golang/dep somehow
dep ensure
go get -u github.com/improbable-eng/grpc-web/go/grpcwebproxy

After that $GOPATH/bin/grpcwebproxy was available.

@lock lock bot locked as resolved and limited conversation to collaborators Feb 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants