overture8 / mangoex

Elixir wrapper for the MangoPay API
3 stars 3 forks source link

Cannot send concurrent requests to MangoPay's API #16

Open kimlai opened 7 years ago

kimlai commented 7 years ago

Hi!

If I understand the code correctly, it seems that you're using a GenServer to be able to store the auth_token and client_id across requests.

Since GenServer calls are blocking, this means that it is not possible to send concurrent requests to MangoPay's API, each call having to wait for the previous one to have been processed before being executed. This behaviour can easily be observed from an iex session:

iex(1)> 1..100 |>
...(1)> Enum.map(fn _ ->
...(1)>   Task.start(fn -> Mangoex.Client.list_users() |> IO.inspect() end)
...(1)> end)

This will output responses sequentially, and start sending GenServer timeout errors after a while.

[error] Task #PID<0.1073.0> started from #PID<0.421.0> terminating
** (stop) exited in: GenServer.call(Mangoex.Client, {:list_users}, 5000)
    ** (EXIT) time out

It seems to me that instead of passing all calls through a GenServer, you could only use one for authentication. It could look something like this:

# Mangoex.Client
  def auth(client_id, client_pass) do
    Mangoex.Auth.authenticate(client_id, client_pass) do
  end

  def list_users do
    {token, client_id} = Auth.get_credentials()
    Mangoex.Api.list_users(client_id, token)
  end

This will make it possible to send concurrent requests to MangoPay, while using the blocking nature of GenServer.call to block all requests while an authentication request is pending. There might be other concerns to deal with in the authentication GenServer (like calling send_after to re-authenticate when the current token expires), but I think that the key point is to keep the GenServer around authentication (ie around the actual state of the application) instead of the whole client.

It will also simplify the code of Mangoex.Client quite a bit.

What do you think?

overture8 commented 6 years ago

Hi @kimlai - so sorry for the late reply. Yes, what you're suggesting makes total sense. It will likely be a while before I can look into this. In the meantime, feel free to put in a PR.

Thanks for your input on the above issue - much appreciated 👍