search

elixir-ecto / db_connection

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

More helpful ConnectionError message on query timeout #119

Closed novaugust closed 6 years ago

novaugust commented 6 years ago

Description of issue

When a connection errors due to a query timeout a helpful message is logged to errors like

Postgrex.Protocol (#PID<0.603.0>) disconnected: ** (DBConnection.ConnectionError) client #PID<0.14026.0> timed out because it checked out the connection for longer than 15000ms

Whatever process was using that connection will also exit if it wasn't trapping, which the error_logger will pick up and print but with a far less helpful message

#PID<0.14026.0> running ... terminated 
(exit) an exception was raised:     ** (DBConnection.ConnectionError) ssl recv: closed

When reporting exceptions to a service, only this second, less useful message gets reported: "ssl recv: closed". Devs then have to open up the logs and go searching for a Postgrex error logged sometime before the exception that contains a matching pid.

Request

Would it be possible to have these timeout exits maintain the useful "timed out because..." message as their exception message? It'd be a big quality of life improvement as a developer, both to cut down on the exception dereferencing but also to differentiate timeout errors from actual ssl errors (handshake, network, ...)

fishcakez commented 6 years ago

This isn't possible in the current implementation. We can likely improve it after #108 if we move to using ETS where we currently use pdict.

josevalim commented 6 years ago

To test this, we need to sleep on a execute instruction and make sure we provide a good message.

josevalim commented 6 years ago

We should also probably include a bit of the client information in the failure.

josevalim commented 6 years ago

@fishcakez it seems the major concern from @novaugust is not the execute timeout message, which we already handle it nicely (although we could include client information in there) but rather that the process that triggers the timeout will fail with either tcp/ssl recv error. To be fair, I don't think we can fix this though.

fishcakez commented 6 years ago

@josevalim I think we can fix it after #152

josevalim commented 6 years ago

Closed in favor of #157.

novaugust commented 6 years ago

in the words of jose:

❤️ 💚 💙 💛 💜