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

Middleware support #41

Open
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jhchabran

jhchabran commented Dec 11, 2017

Hello,

Following-up to #12, you'll find here support for middlewares + examples. I'm not really yet satisfied about how those are tested and the middlewares should have their own example. I'll rebase the commits along the way.

@tony612 Could you make a quick pass to check if overall the changes are in the right direction and that's it's the kind of design you expected ?

jhchabran added some commits Nov 4, 2017

@jhchabran

This comment has been minimized.

jhchabran commented Dec 11, 2017

There's an issue with OTP 19.1, I'll check it :)

def call(service_mod, %{marshal: marshal, unmarshal: unmarshal} = stream, {_, {req_mod, req_stream}, {res_mod, res_stream}} = _rpc, func_name) do
# Unless a middleware inserted a marshal function, use the one provided by service_mod
stream =
unless marshal do

This comment has been minimized.

@tony612

tony612 Dec 12, 2017

Owner

I prefer using if. :)

This comment has been minimized.

@jhchabran

jhchabran Dec 13, 2017

😄 haha, I'll update this :)

def call(service_mod, %{marshal: marshal, unmarshal: unmarshal} = stream, {_, {req_mod, req_stream}, {res_mod, res_stream}} = _rpc, func_name) do
# Unless a middleware inserted a marshal function, use the one provided by service_mod
stream =
unless marshal do

This comment has been minimized.

@tony612

tony612 Dec 12, 2017

Owner

Why not using if?

This comment has been minimized.

@jhchabran

jhchabran Dec 13, 2017

No strong opinion on this, I just happen to get used to unless from my ruby days. I'll put some if there :)

This comment has been minimized.

@tony612

tony612 Dec 13, 2017

Owner

Sorry, I added a redundant comment before 😂

@@ -1,5 +1,43 @@
defmodule FooReplacer do
def call(service_mod, stream, {_, {req_mod, req_stream}, {res_mod, res_stream}} = rpc, func_name, next) do

This comment has been minimized.

@tony612

tony612 Dec 12, 2017

Owner

I just think there's many arguments for a middleware. Is there anyway to make it simple?

This comment has been minimized.

@jhchabran

jhchabran Dec 13, 2017

Yeah I do agree. I just wanted to keep things simple for a start but it's a bit clunky as it is and requires to dive in the internals to understand what's going on.

We could do like Go does, just take move the middleware further down the line, after the request / response had been deserialized.

But that's a decision I'd rather talk with you first 😄

This comment has been minimized.

@tony612

tony612 Dec 13, 2017

Owner

In theory, we can remove stream(if we don't want to support overriding marshal/unmarshal) and func_name. server in stream is one-to-one with server, it's enough for users to log something with the service. adapter and payload is specified to the adapter, most users probably not want to access to those. And func_name can be calculated using name in rpc tuple.

unmarshal_func = fn(req) -> service_mod.unmarshal(req_mod, req) end
stream = %{stream | marshal: marshal_func, unmarshal: unmarshal_func}
def call(service_mod, %{marshal: marshal, unmarshal: unmarshal} = stream, {_, {req_mod, req_stream}, {res_mod, res_stream}} = _rpc, func_name) do
# Unless a middleware inserted a marshal function, use the one provided by service_mod

This comment has been minimized.

@tony612

tony612 Dec 12, 2017

Owner

I just wonder why you want to override marshal/unmarshal functions in a middleware. I think the (maybe?) default is enough in most cases.

This comment has been minimized.

@jhchabran

jhchabran Dec 13, 2017

Since all middleware functions operates at the same level of their GRPC.* counterparts, those parameters being available means that if I swap with a new marshaller that's expected for it to be taken in account, check the FooReplacer example.

In practice though, I don't think that's a common use case, but it felt a bit counterintuitive to not be taken in account at that point.

Another problem though is that's not supported on client-side (because Stub.call/6 doesn't take a stream as a parameter). I'm not sure it's worth the cost to make such changes there.

Go implementation addresses this case by providing to middlewares directly the unmarshalled request and response and thus do not allow such a capability.

I think it makes the whole thing nicer and clearer but I'd have to make more changes to server and stub code.

WDYT?

This comment has been minimized.

@tony612

tony612 Dec 13, 2017

Owner

How about justing ignoring this(leave it as before). Then when we can add the feature back if we need it or others hope it can be supported.

{:ok, channel} = GRPC.Stub.connect("localhost:50051")
{:ok, reply} = channel |> Helloworld.Greeter.Stub.say_hello(Helloworld.HelloRequest.new(name: "grpc-elixir"))
{:ok, reply} = channel |> Helloworld.Greeter.Stub.say_hello(Helloworld.HelloRequest.new(name: "grpc-elixir"), middlewares: [Foo])

This comment has been minimized.

@tony612

tony612 Dec 12, 2017

Owner

The usage of client middleware is a little inconvenient. I wouldn't like to pass middleware to every function call😂

This comment has been minimized.

@jhchabran

jhchabran Dec 13, 2017

Yeah it's not very convenient indeed 😅.

This can be improved but would require to add a middleware: [A,B,C] option to the Stub callback module , which happens to be in generated files (i.e. helloworld.pb.ex) so I didn't go straight for it.

Do you think that's an acceptable tradeoff?

This comment has been minimized.

@tony612

tony612 Dec 13, 2017

Owner

I don't think it's a good idea. I thought about adding middleware in creating connection(apart from how to implement) before. But it seems good enough too because it's applied to a channel instead of a service. I don't have a better idea now 😅

This comment has been minimized.

@tooooolong

tooooolong Feb 24, 2018

Middlewares should designs like the Phoenix pipe.
Requests go through all the middlewares in a pipeline.

@tony612

This comment has been minimized.

Owner

tony612 commented Dec 12, 2017

Don't worry about that CI failure. I just retried it. Sometimes, it failes :(

@tooooolong

This comment has been minimized.

tooooolong commented Feb 24, 2018

Hi,I'd like to join this work. I want to migrate the middleware in the golang implenment of gRPC directly.

Do you guys think this is a good idea?

@tony612

This comment has been minimized.

Owner

tony612 commented Mar 18, 2018

@tooooolong I think we'd better add middleware feature when the next branch is merged to master. I'm not very familiar with middleware in golang grpc, but Elixir and Go are very different, so I doubt that we have to figure out how to do in Elixir.

@jhchabran

This comment has been minimized.

jhchabran commented May 6, 2018

@tony612 shouldn't we close this now you're working on interceptors?

Also, you were talking about a next branch which I'd love to follow but couldn't find, any infos regarding that?

Thanks for the hard work, cheers :)

@tony612

This comment has been minimized.

Owner

tony612 commented May 12, 2018

@jhchabran next has been merged to master. I'm still working on the client interceptors.

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