esl / release_poller

3 stars 2 forks source link

Usage of poolboy in the bugs bunny AMQP library #26

Open gausby opened 5 years ago

gausby commented 5 years ago

My poolboy skills might be a bit rusty, but this strikes me a bit odd:

  # … in bugs_bunny.ex

  def with_channel(pool_id, fun) do
    conn_worker = :poolboy.checkout(pool_id)
    :ok = :poolboy.checkin(pool_id, conn_worker)
    do_with_conn(conn_worker, fun)
  end

As I read this it will check out a worker, instantly check it back in, and then perform its work. If I remember correctly one should check out a worker, perform some blocking operation, and only then check it back in. We might also want to assert on the result of do_with_conn/2.

Or am I missing something ?

sescobb27 commented 5 years ago

@gausby you are right, but this is not the case, bugs bunny has a pool of pools, a pool of connections managed by poolboy, and each connection manage a pool of channels which are the important part for us, when working with RabbitMQ we usually work with channels instead of connections, so we get a connection out of the pool and then create/get a channel from that connection, in this case, we already have a pool of channels, do_with_conn is going to check out a channel from the connection and then exec the given function passing with that channel as an argument, in this case, we do keep the channel until the function returns or throws an error, then we return the channel back to the pool, and that's the case you are describing, but for connections, is safe to put them back as fast as possible so another process can get a connection and maybe a channel

NOTE: channels pool is implemented using a list (would be nice to improve this and use ETS or somrthing), i did not use poolboy for this, because i wasn't able to create a pool of pools and also didn't know how to deal with all its caveats.

  defp do_with_conn(conn_worker, fun) do
    case checkout_channel(conn_worker) do
      {:ok, channel} = ok_chan ->
        try do
          fun.(ok_chan)
        after
          :ok = checkin_channel(conn_worker, channel)
        end

      {:error, _} = error ->
        fun.(error)
    end
  end

let me know if this is a good explanation, or something else is missing

sescobb27 commented 5 years ago

one may want to checkout at connection, checkout a channel put back the connection to the pool and call the function with the channel, but i think is safe to it the other way because messages are going to be processed "sync" so there is not going to be a race condition when checking out/in a channel (which is the only work of the connection worker)

gausby commented 5 years ago

I have never seen this pattern. Why have a pool at all then ?

sescobb27 commented 5 years ago

AFAIK channels are multiplexed connections to rabbitmq, connecting to rabbitmq is really expensive (chatty), so we try to have a long live pool of connections to it (or 1) and use channels to communicate to RabbitMQ (i think the max number of channels per connection is something like 65,535). When using channels, there are 2 approaches, create them on demand (re-use connection but get rid of channels after usage) or keep them in a pool inside the connection worker (my approach), that was the reason.