search

elixir-ecto / db_connection

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

DBConnection.Watcher should handle unknown messages #219

Closed Cantido closed 4 years ago

Cantido commented 4 years ago

This is a good practice because anything can send our Watcher process a message and crash it. From my own experimentation, the process isn't starting back up, either.

Now, I should say that I do have a process that sends messages to random processes, as part of a chaos engineering effort on my team, so it's up to y'all if this is a practical concern or not. From what I understand, though, implementing a catch-all handle_info clause is a best practice.

josevalim commented 4 years ago

Hi @Cantido, thanks for the PR!

From what I understand, though, implementing a catch-all handle_info clause is a best practice.

Not necessarily. For example, I often prefer to not implement a catch-all because if it does receive a message that it doesn't expect, then it may be a bug somewhere in the software (i.e. I forgot to receive a message that I should OR someone is sending a message to the wrong place).

For the reason above, I prefer to not merge this one for now. Thanks!

Cantido commented 4 years ago

Understood! Thanks for the quick response!

On Jun 17, 2020, at 1:25 PM, José Valim notifications@github.com wrote:

Hi @Cantido https://github.com/Cantido, thanks for the PR!

From what I understand, though, implementing a catch-all handle_info clause is a best practice.

Not necessarily. For example, I often prefer to not implement a catch-all because if it does receive a message that it doesn't expect, then it may be a bug somewhere in the software (i.e. I forgot to receive a message that I should OR someone is sending a message to the wrong place).

For the reason above, I prefer to not merge this one for now. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elixir-ecto/db_connection/pull/219#issuecomment-645574827, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALPSQYLZKWWFGCC7XOBXPLRXEKDJANCNFSM4OA4NM5Q.

Cantido commented 4 years ago

I have a related question, actually. When this process crashes, I'm not seeing it restart. In fact, it seems to take down the entire DB connection and it doesn't reconnect. Queries fail with ** (DBConnection.ConnectionError) tcp recv: closed. Is this intended?

josevalim commented 4 years ago

I need to investigate to be sure. :)

Cantido commented 4 years ago

If you could do that, that would be fantastic! Either way, I got some good results from this experiment of mine and my app will be much more tolerant of DB errors.

I super appreciate the work you do. Thank you!