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

Add Mint client adapter #242

Closed
polvalente opened this issue Jul 21, 2022 · 11 comments · Fixed by #272
Closed

Add Mint client adapter #242

polvalente opened this issue Jul 21, 2022 · 11 comments · Fixed by #272
Assignees
Milestone

Comments

@polvalente
Copy link
Contributor

After #199, we now have a contract defined for the HTTP client, so we can now implement an adapter using the Mint library and use that as a default instead of the Gun adapter

@hkrutzer
Copy link
Contributor

I extracted the great work done in Spear into a standalone gRPC client while this repo was inactive here. It might be of help.

@polvalente
Copy link
Contributor Author

@hkrutzer thanks for pointing that out! Do you want to submit it as a pull request, or should I extract the relevant code?

@hkrutzer
Copy link
Contributor

I probably won't submit a PR anytime soon; streaming with that client can be done by sending messages, which I need for my own use case, and that doesn't look compatible with https://github.com/elixir-grpc/grpc/blob/d15bdb36e238d4352c87da4fb4bf056dc1960453/lib/grpc/client/adapter.ex

@polvalente
Copy link
Contributor Author

@hkrutzer no worries! We can leave this up for grabs, then :)

@tony612
Copy link
Collaborator

tony612 commented Jul 29, 2022

I want to give some of my previous ideas about HTTP/2 library. One reason I didn't add Mint or other HTTP/2 client in early days is I want to let client and service share the same underlying HTTP/2 code like cowlib in Cowboy and Gun. In HTTP/2 protocol, the difference between the client and server roles is small and the protocol of them is almost the same. Code will be simpler and more stable if we can do this. Unfortunately, Mint has only client now, Cowboy and Gun are not active as Mint, and there's no other better choice in Elixir community. My ideal HTTP/2 library should like:

  • Active and be maintained well
  • Only HTTP/2. HTTP/1 will add unnecessary complexity given gRPC doesn't need HTTP/1
  • Client and Server share the same HTTP/2 protocol-related code
  • Correct protocol implementation like passing https://github.com/summerwind/h2spec
  • Good performance, flexibility, and concurrency

But I didn't mean we can't add Mint or others. We can do this now given Mint is active and good. I just hope we can move to a better target slowly in the future.

@polvalente
Copy link
Contributor Author

These are great points for us to keep in mind!

@beligante
Copy link
Contributor

@polvalente

While looking close to this (took me some time to get full context about the lib mechanics lol)

The way Mint vs Gun works are a bit different.

[Tony I'm simplifying what you did in the explanation for brevity]
The Gun approach that Tony took while implementing the adapter was basically to use a the :gun.await function to synchronously wait for a message to be received and process contents of the request with a recursive function until the end of the request

For Mint things are a bit different, when sending a request, the caller process starts to receive tcp messages and should be responsible for deal with them.


I would like some opinions here

To implement the adapter, I'm thinking in spawn a short lived gen_server to deal with the requests.

  • Why a GenServer ? Because I need to process the incoming messages, accumulate and return. So, I need to hold some state
  • Spawn a GenServer per connection - That would make it easier to receive multiple requests for bi-directional requests
    • not 100% sure, but I believe it's possible to spawn a very short lived gen_server per request, but that could be very complex

Thoughts on that approach? Based on the code that @hkrutzer shared here he followed the same/similar approach.

@polvalente
Copy link
Contributor Author

Thanks for the follow-up investigation!

We can start by having a GenServer per connection. This might be the case where message processing serialization is not an issue, because if we're talking about the same connection, it means that we can't send/receive two different request at the same time anyway.

@sleipnir
Copy link
Collaborator

...it means that we can't send/receive two different request at the same time anyway.

Is this also true for bidirectional streams?

@beligante
Copy link
Contributor

...it means that we can't send/receive two different request at the same time anyway.

Is this also true for bidirectional streams?

You kinda can, that's where the state management comes in. When we receive a request, I was thinking in store the information about the information about the caller process in a sort of map with the request_ref

When I receive the messages from tcp, that message comes with the request_ref so, I know which process I should interact with.

BTW, I'm talking about process here, but it might be another type of communication. Like use Stream.unfold/2 (the elixir Stream here lol - lot's Stream in the same context)

@polvalente
Copy link
Contributor Author

A process will always process the messages it receives sequentially. What @sleipnir is worried about is that this could mean that a bidirectional system, which would be full-duplex in the best case, would be ensured to become half-duplex.

I'm not entirely familiar with HTTP/2, but from a connection socket point of view, this could indeed be the case.

However, I wouldn't worry about this too much at first, at least for a proof of concept implementation with Mint. In the worst case scenario we would have half the throughput for a given connection, but we can think of ways to optimize this afterwards (for instance, a handler process that splits tx and rx for a given connection into two different handlers)

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

Successfully merging a pull request may close this issue.

5 participants