Make server stream work with mint client and add keepalive. #352
Conversation
| @@ -94,6 +106,9 @@ defmodule GRPC.Client.Adapters.Mint.ConnectionProcess do | |||
| def handle_call(:disconnect, _from, state) do | |||
| # TODO add a code to if disconnect is brutal we just stop if is friendly we wait for pending requests | |||
| {:ok, conn} = Mint.HTTP.close(state.conn) | |||
|
|
|||
| finish_all_pending_requests(state) | |||
| keep_alive = Keyword.get(opts, :keep_alive, false) | ||
|
|
||
| [transport_opts: Keyword.merge(@default_transport_opts, transport_opts), keep_alive: keep_alive] |
There was a problem hiding this comment.
For clarity, it's better if you add a question mark to this option, since, usually keep_alive is a provided as a number by convention.
| keep_alive = Keyword.get(opts, :keep_alive, false) | |
| [transport_opts: Keyword.merge(@default_transport_opts, transport_opts), keep_alive: keep_alive] | |
| keep_alive? = Keyword.get(opts, :keep_alive?, false) | |
| [transport_opts: Keyword.merge(@default_transport_opts, transport_opts), keep_alive?: keep_alive?] |
There was a problem hiding this comment.
I wonder if it's valuable to add a module doc describing this adapter-specific option. WDYT?
There was a problem hiding this comment.
https://github.com/elixir-mint/mint/blob/main/lib/mint/http.ex#L276-L364
can't see any keep_alive option here
There was a problem hiding this comment.
transport_opts is forwarded to gen_tcp or ssl. The Mint docs list the most common ones, but there are more of them.
https://www.erlang.org/doc/man/gen_tcp.html#type-option
Now that you mentioned, keepalive on gen_tcp doesn't have a underscore
| check_connection_status(state) | ||
| {:noreply, state} |
There was a problem hiding this comment.
You want the check_connection_status to handle the GS reply for you
| check_connection_status(state) | |
| {:noreply, state} | |
| check_connection_status(state) |
| defp process_response({:error, request_ref, error}, state) do | ||
| pid = State.stream_response_pid(state, request_ref) | ||
| :ok = StreamResponseProcess.consume(pid, :error, error) | ||
| :ok = StreamResponseProcess.done(pid) |
There was a problem hiding this comment.
Why you're sending done/1 here? By default, even if a error happens, the connection will produce a :done message when the stream is closed by the server
|
@nulian did you get a chance to act on the reviews? |
This is what I been using for a while in our application.
Changed connection open? check to :read so server streaming is seen as a valid state for the connection.
And added a keepalive to make sure the connection is still open of the server stream because if a service like nginx breaks the connection you won't get any notification so you will never close the connection even if it's already broken.
Also added some error handling cases so it won't cause any server errors.
#312