elixir-grpc / grpc

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

Why does `GRPC.Stub.connect` return a Channel struct rather than a PID? #191

Open isaacsanders opened 3 years ago

isaacsanders commented 3 years ago

As a user, I like having a PID rather than a channel, as I can do a number of things with a process:

  1. I can start a PID in a supervision tree
  2. I can Monitor a PID for disconnections
  3. I can link other processes to PIDs
  4. I can name a PID

I don't know that I find a lot of uses where the struct is a better approach to this public API.

kirega commented 3 years ago

I am not very good at this but I believe I can provide some insight. Part of the struct payload from the channel is


               accepted_compressors: [],
               adapter: GRPC.Adapter.Gun,
               adapter_payload: %{conn_pid: channel.adapter_payload.conn_pid},
               codec: GRPC.Codec.Proto,
               compressor: nil,
               cred: nil,
               headers: [],
               host: host,
               interceptors: [{GRPC.Logger.Client, :info}],
               port: port,
               scheme: "http"
             }```
`adapter_payload: %{conn_pid: channel.adapter_payload.conn_pid},`  can provide you the PID you need.
I believe part of the reason it is a struct is that it gives you some flexibility to change the details of your connection on the fly. 

On your second point around monitoring the connection, I have managed to monitor disconnection and reconnections on a channel and specifically on this library. My approach was to open a channel that runs on a GenServer process and then utilize the `handle_info` callbacks that will receive information such as `{:gun_up, ...}` and `{:gun_down,  ...}` from the gun process
Which you can utilize to reconnect. There are probably better approaches but this worked. 

I hope this helps even if its a little.
sleipnir commented 3 years ago

On your second point around monitoring the connection, I have managed to monitor disconnection and reconnections on a channel and specifically on this library. My approach was to open a channel that runs on a GenServer process and then utilize the handle_info callbacks that will receive information such as {:gun_up, ...} and {:gun_down, ...} from the gun process Which you can utilize to reconnect. There are probably better approaches but this worked.

Hello @kirega, good suggestions. Can you show your approach to reconnection with a code example? It was not entirely clear to me how this works. Thanks!

kirega commented 3 years ago
defmodule GRPClient do
  use GenServer
  require Logger

  def start_link(_) do
    GenServer.start_link(__MODULE__, :ok, name: __MODULE__)
  end

  def channel() do
    GenServer.call(__MODULE__, :channel)
  end

   @impl true
    def init(_) do
      Logger.debug(
        "gRPC client connecting to gateway at #{Application.get_env(:app, :gateway_url)}"
      )

      case GRPC.Stub.connect(Application.get_env(:app, :gateway_url),
             interceptors: [GRPC.Logger.Client]
           ) do
        {:error, error} ->
          Logger.critical("Could not connect. Retrying... #{error}")
          init(%{})

        channel ->
          Logger.debug(
            "Connected to the gateway at #{Application.get_env(:app, :gateway_url)}"
          )
          {:ok, channel}
      end
    end

    @impl true
    def handle_info({:gun_down, _, _, _, :closed}, _state) do
      Logger.debug("Server disconnected, will attempt to reconnect")
      init(%{})
    end

    @impl true
    def handle_info({:gun_up, _, _, _, _}, _state) do
      Logger.debug("Server connected")
      init(%{})
    end
  end

  @impl true
  def handle_call(:channel, _from, channel) do
    {:reply, channel, channel}
  end
end

In this GenServer for example, I only care about the channel so I don't use state. @sleipnir

sleipnir commented 3 years ago

Thanks @kirega. A question in the gun_up message is it okay to try to reconnect?

kirega commented 3 years ago

Thanks @kirega. A question in the gun_up message is it okay to try to reconnect?

No it is not, probably not useful.

isaacsanders commented 3 years ago

@kirega Making the channel a struct, rather than a PID, makes it hard to change thing on the fly.

It is easier for you to have changed things, but the channel could manage the gun client connection in a supervisor on its own, and the channel could be a supervisor itself, allowing you to have a more resilient interface.

I don't think there is a good reason to make it a struct vs a PID, and, while I'm glad it is working for you, you haven't convinced me.

polvalente commented 2 years ago

@isaacsanders do you want to write a draft PR to better lay out your idea on how a channel can be represented as a process instead?

One downside is that processes serialize messages passed to them, but perhaps this is something that is acceptable if there already is a serialization point anyway for a given channel. I'm not 100% familiar with the code base yet, so I can't give my thoughts on this specifically.

isaacsanders commented 1 year ago

One can still serialize the request to proto bytes before sending the request to the PID, right? You don't need to pass the struct to the pid.

polvalente commented 1 year ago

@isaacsanders by "serialize messages"/"serialization" point I meant that messages will be processed in sequence instead of concurrently if you centralize them onto a single process

isaacsanders commented 1 year ago

@polvalente is there an adapter implementation that doesn’t serialize the messages to a process? I might be misremembering.

isaacsanders commented 1 year ago

And you can have multiple processes and easily use something like the PartitionSupervisor with a process.