elixir-grpc / grpc

An Elixir implementation of gRPC
https://hex.pm/packages/grpc
Apache License 2.0
1.38k stars 212 forks source link

feat: Add Mint adapter #272

Closed beligante closed 1 year ago

beligante commented 1 year ago

Closes: https://github.com/elixir-grpc/grpc/issues/242 (sorry for the huge PR - tried to make everything well tested + make interop tests work with mint)

Covered in this PR

Cover all possible use cases for a gRPC (client-server):

There is also several E2E test cases here, which includes:

Suggestion on how to review

We have two main modules that handles the interactions with the server.

Connection Process

This process is spawned every time we attempt to connect to a gRPC server. It does strictly three things:

Because of HTTP2 flow control mechanics I've designed the request handling of all requests to behave as a streamed request, this way it's easier to check for the window size and chunk the request if necessary. Because of that, you might see that I'm treating even unary requests, as streams - but that behavior is transparent to the user of the lib.

I'm also handling connection drops with the following strategy:

Response Process

This process is spawned every time a request is made. It's responsible to receive cast calls from the ConnectionProcess and produce the messages to an Elixir.Stream. It dies when the request is finished - so it should be a short lived process.

wingyplus commented 1 year ago

Found dialyzer fail on my machine (Elixir 1.14, OTP 25):

lib/grpc/server/supervisor.ex:39:callback_spec_type_mismatch
The @spec return type does not match the expected return type
for init/1 callback in Supervisor behaviour.

Actual:
@spec init(...) :: 
  {:ok,
   {{:one_for_all, non_neg_integer(), pos_integer()}
    | {:one_for_one, non_neg_integer(), pos_integer()}
    | {:rest_for_one, non_neg_integer(), pos_integer()}
    | {:simple_one_for_one, non_neg_integer(), pos_integer()}
    | %{
        :auto_shutdown => :all_significant | :any_significant | :never,
        :intensity => non_neg_integer(),
        :period => pos_integer(),
        :strategy => :one_for_all | :one_for_one | :rest_for_one | :simple_one_for_one
      },
    [
      {_, {atom(), atom(), :undefined | [any()]}, :permanent | :temporary | :transient,
       :brutal_kill | timeout(), :supervisor | :worker, :dynamic | [atom()]}
      | %{
          :id => _,
          :start => {atom(), atom(), :undefined | [any()]},
          :modules => :dynamic | [atom()],
          :restart => :permanent | :temporary | :transient,
          :shutdown => :brutal_kill | timeout(),
          :significant => boolean(),
          :type => :supervisor | :worker
        }
    ]}}

Expected:
@spec init(...) :: 
  :ignore
  | {:ok,
     {%{
        :intensity => non_neg_integer(),
        :period => pos_integer(),
        :strategy => :one_for_all | :one_for_one | :rest_for_one
      },
      [
        {_, {atom(), atom(), :undefined | [any()]}, :permanent | :temporary | :transient,
         :brutal_kill | timeout(), :supervisor | :worker, :dynamic | [atom()]}
        | %{
            :id => _,
            :start => {atom(), atom(), :undefined | [any()]},
            :modules => :dynamic | [atom()],
            :restart => :permanent | :temporary | :transient,
            :shutdown => :brutal_kill | timeout(),
            :significant => boolean(),
            :type => :supervisor | :worker
          }
      ]}}

________________________________________________________________________________
done (warnings were emitted)
Halting VM with exit status 2

So I open the PR to try fixing the issue here https://github.com/elixir-grpc/grpc/pull/282

NOTE: found it on master branch as well.

polvalente commented 1 year ago

Is this ready for review?

beligante commented 1 year ago

Is this ready for review?

M'bad! Forgot to ping you! Yes! It's ready

One thing that I might need your help here is: How to make mint optional here?

I tried to add the check using the Code.ensure_compiled?, but it stops to run the tests

polvalente commented 1 year ago

Is this ready for review?

M'bad! Forgot to ping you! Yes! It's ready

One thing that I might need your help here is: How to make mint optional here?

I tried to add the check using the Code.ensure_compiled?, but it stops to run the tests

You can try using Code.ensure_loaded, which is a blocking call. Also, be aware that tests will always see an optional dependency as available, so it might be hard to check that it is indeed optional.

edit: I'm ok with having Mint as a mandatory dependency since it's what we're moving towards

wingyplus commented 1 year ago

@beligante Hi, I've some fix from code review suggestions here https://github.com/beligante/grpc/pull/1. Please kindly review. 🙇

beligante commented 1 year ago

Hey Folks! Tks for the review. I'm on vacations for one week. I promise to carefully check your comments once I'm back.

Hope ya understand - I need a week of computers haha

beligante commented 1 year ago

@polvalente I think this one is good to go. There is one this one comment that I've solved, but I like your input about what I saw: https://github.com/elixir-grpc/grpc/pull/272#discussion_r1059002072

Also, dialyzer is breaking for 24.x because of cache. Since my last successful build til today the latest OTP 24 version was updated from OTP-24.3.4.6 to OTP-24.3.4.7 thoughts on pin that version to use an exact match?

Edit: Found out that you can also delete the cache manually: https://github.com/elixir-grpc/grpc/actions/caches lol

polvalente commented 1 year ago

@polvalente I think this one is good to go. There is one this one comment that I've solved, but I like your input about what I saw: #272 (comment)

Also, dialyzer is breaking for 24.x because of cache. Since my last successful build til today the latest OTP 24 version was updated from OTP-24.3.4.6 to OTP-24.3.4.7 thoughts on pin that version to use an exact match?

Edit: Found out that you can also delete the cache manually: https://github.com/elixir-grpc/grpc/actions/caches lol

TIL about manually deleting cache entries!

polvalente commented 1 year ago

@tony612 I'm merging this so we can have the current state of the adapter as a clean slate. We can discuss next iterations on a future issue/PR if needed.

There are already some TODO comments that will be addressed

beligante commented 1 year ago

Opened an Issue for one of the TODO's: https://github.com/elixir-grpc/grpc/issues/291

tony612 commented 1 year ago

@polvalente Got it. Please ping me if there's anything needed from me.

Nice to see this got merged!