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

Middleware support #41

Closed
wants to merge 17 commits into from
Closed

Middleware support #41

wants to merge 17 commits into from

Conversation

@jhchabran
Copy link

@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
Copy link
Author

@jhchabran 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
Copy link
Collaborator

@tony612 tony612 Dec 12, 2017

I prefer using if. :)

Copy link
Author

@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
Copy link
Collaborator

@tony612 tony612 Dec 12, 2017

Why not using if?

Copy link
Author

@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 :)

Copy link
Collaborator

@tony612 tony612 Dec 13, 2017

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
Copy link
Collaborator

@tony612 tony612 Dec 12, 2017

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

Copy link
Author

@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 😄

Copy link
Collaborator

@tony612 tony612 Dec 13, 2017

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
Copy link
Collaborator

@tony612 tony612 Dec 12, 2017

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

Copy link
Author

@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?

Copy link
Collaborator

@tony612 tony612 Dec 13, 2017

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])
Copy link
Collaborator

@tony612 tony612 Dec 12, 2017

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

Copy link
Author

@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?

Copy link
Collaborator

@tony612 tony612 Dec 13, 2017

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 😅

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

@tony612
Copy link
Collaborator

@tony612 tony612 commented Dec 12, 2017

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

@tooooolong
Copy link

@tooooolong 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
Copy link
Collaborator

@tony612 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
Copy link
Author

@jhchabran 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
Copy link
Collaborator

@tony612 tony612 commented May 12, 2018

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

@tony612 tony612 closed this Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants