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

feat: Add Mint adapter #272

Merged
merged 90 commits into from
Jan 10, 2023
Merged

Conversation

beligante
Copy link
Contributor

@beligante beligante commented Sep 10, 2022

Closes: #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):

  • Unary -> Unary
  • Unary -> Stream
  • Stream -> Unary
  • Steam -> Stream

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

  • Canceling requests
  • Compressing headers
  • Large requests
  • Timeout scenarios

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:

  • Open and store the Mint connection struct.
  • Deals with tcp_messages . And that includes
    • Handling window_size frames
    • pass the headers/trailers/data to the response process
  • Handling the requests

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:

  • I close/end all pending requests as suggested here
  • I'm also sending an message to the parent process to inform that the connection droped. with the following format:
{:elixir_grpc, :connection_down, self_pid}

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.

Copy link
Contributor

@polvalente polvalente left a comment

Choose a reason for hiding this comment

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

The overall work looks great!

examples/route_guide/lib/client.ex Outdated Show resolved Hide resolved
lib/grpc/client/adapter.ex Outdated Show resolved Hide resolved
lib/grpc/client/adapters/gun.ex Outdated Show resolved Hide resolved
lib/grpc/client/adapters/mint.ex Outdated Show resolved Hide resolved
lib/grpc/client/adapters/mint.ex Outdated Show resolved Hide resolved
lib/grpc/client/adapters/mint/stream_response_process.ex Outdated Show resolved Hide resolved
lib/grpc/client/adapters/mint/stream_response_process.ex Outdated Show resolved Hide resolved
@beligante
Copy link
Contributor Author

beligante commented Dec 29, 2022

@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

@polvalente
Copy link
Contributor

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

Copy link
Contributor

@polvalente polvalente left a comment

Choose a reason for hiding this comment

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

I think we're getting to the final rounds here! I just left some comments improving docs and a last question regarding the stream.

After that's answered I'll do a final pass through the whole PR. I don't think we missed anything, but since this PR went through many iterations, it's best to be safe.

Copy link
Contributor

@polvalente polvalente left a comment

Choose a reason for hiding this comment

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

Awesome work! I left a final batch of comments.
Most are stylistic, but there are some questions as well (which don't seem to block this PR further!)

Once we resolve this last batch we can merge and open an issue to tackle all of the TODOs we left.

lib/grpc/client/adapters/mint.ex Outdated Show resolved Hide resolved
lib/grpc/client/adapters/mint.ex Outdated Show resolved Hide resolved
lib/grpc/message.ex Show resolved Hide resolved
lib/grpc/stub.ex Outdated Show resolved Hide resolved
lib/grpc/stub.ex Outdated Show resolved Hide resolved
lib/grpc/stub.ex Outdated Show resolved Hide resolved
lib/grpc/stub.ex Show resolved Hide resolved
lib/grpc/client/adapters/mint.ex Outdated Show resolved Hide resolved
lib/grpc/client/adapters/mint.ex Outdated Show resolved Hide resolved
lib/grpc/client/adapters/mint.ex Outdated Show resolved Hide resolved
test/grpc/client/adapters/mint_test.exs Outdated Show resolved Hide resolved
Copy link
Contributor

@polvalente polvalente left a comment

Choose a reason for hiding this comment

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

I think we finally got this to a place where it can be merged! I've commited some changes to your latest error messages and we just need to wait for CI :)

Awesome work!

@polvalente
Copy link
Contributor

@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

@polvalente polvalente merged commit f5f5fab into elixir-grpc:master Jan 10, 2023
@beligante
Copy link
Contributor Author

Opened an Issue for one of the TODO's: #291

@tony612
Copy link
Collaborator

tony612 commented Jan 16, 2023

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

Nice to see this got merged!

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

Successfully merging this pull request may close these issues.

Add Mint client adapter
6 participants