Disconnect actual db connection on terminate

[greg-rychlewski]

Closes https://github.com/elixir-ecto/postgrex/issues/661

When I tested it out on the example given in the ticket it seems to work nicely. This would only catch the parent causing the connection to exit. I don't think we need to worry about anything else since the connection process shouldn't be touched outside of db_connection, if I'm remembering correctly. But please let me know if I'm missing something.

[greg-rychlewski]

Ah I only ran the unit tests locally. I'll see what's causing those other tests to complain.

[greg-rychlewski]

The test issues were because of the new calls to disconnect when the connection dies. They weren't being recorded by the test connection/pool modules.

I also noticed that the connection might have already been disconnected before getting to the terminate callback. For example if disconnect_all is called and the connection doesn't have a backoff.

This seems like it is working to me but if I'm being honest I feel less confident than normal due to not knowing db connection as well as the other repos. To me it seems like this should work. Expected disconnects seem like they are already being caught and this should catch unexpected disconnects like if a client exits or if the connection pool is shut down. Part of me is concerned I am not seeing some edge cases though.

[josevalim]

This actually looks great to me, I have only added a comment about docs and not trapping by default, we can trap on tests. This means we will need to trap on Postgrex anyway (and MyXQL), but it feels keeping it opt-in is more consistent with OTP.

[greg-rychlewski]

I could help with the Postgrex/MyXQL changes. I'm having a bit trouble seeing the solution though sorry. For "regular" connections I believe they are just providing functions to the caller and not starting any process itself. How would trapping exits work in those libraries?

[josevalim]

I may be wrong, but I think DBConnection.Connection calls this function: https://github.com/elixir-ecto/postgrex/blob/master/lib/postgrex/protocol.ex#L70 - so you could trap exits there.

[greg-rychlewski]

Ah I think I get it now. After my last commit is it how you are expecting?

And do I understand correctly we would do it this way so that the adapter has to opt into the behaviour. i.e. it shouldn't be allowed just because it's using db connection.

edit: if so i should document the option in the drivers instead of here. removed it

[josevalim]

Sorry, maybe I misunderstood, but I don't think we need an option. I think we should do it in one of two ways:

1. Always disconnect on terminate (which will happen as long as you trap exits)

2. Expose terminate as an additional callback on DBConnection behaviour which we simply delegate, the driver can decide to trap exits or not and to disconnect or not

Thoughts?

[josevalim]

And, to be clear, the Process.flag(:trap_exit, true) call will always happen inside Postgrex.Protocol.connect, never here.

[greg-rychlewski]

I was in favour of 1 but I'm not sure I can get it to work if trapping exits is in Postgrex.Protocol, etc.. I could make terminate trigger without trapping exits. For example, if the process calling execute raises and dies. So I think this means we can never have it completely controlled by the adapter.

If I'm not missing anything above then I think (2) is the best option? Does it seem right to you as well?

[greg-rychlewski]

Not sure how clear I was being about 1. What I mean is that sometimes terminate is called even if the process is not trapping exits. So by default even if the adapter is not trapping exits we would disconnect sometimes. And not sure if that is too inconsistent.

[josevalim]

Yes, terminate is called from stop tuples as well, and that’s normal OTP behavior. But it will only be invoked from exit signals if trapping exits.

[josevalim]

So to clarify, what I mean is that the “always” will only happen if trapping exits. Without trapping exits, is not always, but on expected shutdowns. :)

[greg-rychlewski]

Now I got it :). Apologies for being slow. Let me know what you think. If ok I'll send the PRs for Postgrex/MyXQL