elixir-ecto / db_connection

Database connection behaviour
http://hexdocs.pm/db_connection/DBConnection.html
309 stars 112 forks source link

Attempted to call GenServer #PID<...> but no handle_call/3 clause was provided #174

Closed dmorneau closed 5 years ago

dmorneau commented 6 years ago

Environment

Current behavior

After upgrading my application to db_connection-2.0.1 and ecto_sql-3.0.0, this code in a Phoenix controller started failing:

    parent_pid = self()
    {pid, monitor} = spawn_monitor(fn ->
      Ecto.Adapters.SQL.Sandbox.allow(MyApp.Repo, parent_pid, self())
      Repo.aggregate(MyApp.User, :count, :id)
    end)

The error says:

[error] Child DBConnection.ConnectionPool of Supervisor MyApp.Repo terminated
** (exit) an exception was raised:
    ** (RuntimeError) attempted to call GenServer #PID<0.2282.0> but no handle_call/3 clause was provided
        (db_connection) lib/gen_server.ex:693: DBConnection.ConnectionPool.handle_call/3
        (stdlib) gen_server.erl:661: :gen_server.try_handle_call/4
        (stdlib) gen_server.erl:690: :gen_server.handle_msg/6
        (stdlib) proc_lib.erl:249: :proc_lib.init_p_do_apply/3

and

Last message (from #PID<0.2289.0>): {:allow, #PID<0.2287.0>, #PID<0.2289.0>}

I noticed the same error happening in all tests if I run this command in the db_connection repo:

mix test.all integration_test/ownership/manager_test.exs

Expected behavior

I would expect the code to work as it did previously, i.e. the Sandbox.allow call would be a no-op when not using the sandbox (not running tests) rather than cause a crash.

If that's not allowed anymore that's fine too, but I figured I would ask first, the error message is rather confusing.

fishcakez commented 6 years ago

Ah this would only work when using the previous poolboy pool, other pools would error. It was unintentional and this is bad practice on poolboy side as the reply was causing incorrect client behavior. We can definitely provide a better error but it would be raising as something has gone very wrong when call this with the wrong pool.

The way to solve this in Ecto is to pass caller: parent_pid to the repo calls in the spawned process. This will handle allow without requiring an explicit call.

josevalim commented 5 years ago

I will improve this error on the ecto side of things. But yes, this is not supported behaviour. We will instead raise a better error message!