Handle Connection: close response from server

[ananthakumaran]

A server may return Connection: close while gracefully shutting down. Mint will mark the connection state as closed and return the response from the server (NOTE: it will not return any error). Since the client doesn't know the connection is closed, it will try to use the connection for the next request and will fail.

Before making any HTTP request, check the internal state of the connection and attempt to reconnect once if necessary.

partially related to #210

[ruslandoga]

👋 @ananthakumaran

Thank you for the contribution! I had my doubts about re-connecting outside of DBConnection lifecycle, but this looks very clean and I think is the way to go!

[ananthakumaran]

@ruslandoga btw, to be clear, this will only work if the server uses Connection: close. If the connection gets interrupted in any other way, there will be at least one failed request per connection. My guess is, without going active mode etc, that problem can't be solved.

[ruslandoga]

Or maybe https://www.erlang.org/doc/apps/kernel/inet.html#monitor/1 and whatever is :ssl's alternative could work? I wonder where the messages would be received and how it would even work with the DBConnection's checkout mechanism. I guess, if we monitor the socket in connect, the down messages would be received in the "connection" process, but I don't think we would be able to actually handle them in Ch. Same problem probably applies to active sockets, but AFAIK Postgrex is using active sockets sometimes (for buffering?) so it might be used as an example.

[ananthakumaran]

I haven't looked into the general case issue. I added graceful shutdown to our clickhouse proxy and realized the client is still failing once. This fix will gracefully handle the connection: close. For the general case, I think DBConnection ping already acts like a socket monitor and reconnects in case of timeout, etc. We could definitely improve the detection time, but that would require much more effort, as you noted, it seems like DBConnection controls the process and the callback can't have a separate message queue, etc. I initially attempted to use disconnect, but that always results in sending an error to the caller outside of ping.

[ruslandoga]

I initially attempted to use disconnect, but that always results in sending an error to the caller outside of ping.

Ah, I see. I just wanted to recommend that :) It would've been nice to reconnect right after the "last" request.

[ruslandoga]

Thank you again for the contribution!

I wonder if you'd like me to make another release (btw, do you think it warrants a version bump to v0.3.0?) or if we can sit on it for a bit until we come up with a more general fix for closed connections?

[ruslandoga]

It would've been nice to reconnect right after the "last" request.

Actually, can we do it with the same logic as in this PR right after request? If response headers contain connection: closed -> "swizzle" connection to a new one without telling DBConnection? I guess that approach would be kind of equivalent, as instead of the next query, it's the current one that would incur the reconnection cost, so might not be worth the effort.

[ananthakumaran]

I wonder if you'd like me to make another release

give me some time, let me test this out on our staging

[ruslandoga]

Btw, I opened an issue on DBConnection asking for guidance: elixir-ecto/db_connection#315

[ananthakumaran]

I wonder if you'd like me to make another release (btw, do you think it warrants a version bump to v0.3.0?) or if we can sit on it for a bit until we come up with a more general fix for closed connections?

I have tested it a bit, and the connection: close problem looks like solved. I still see a few Mint.TransportError, but it might be something else.

if we can sit on it for a bit until we come up with a more general fix for closed connections?

Release can be delayed, no issue from my side.

[hkrutzer]

I am seeing a lot of Ch.Connection (#PID<0.6408.0>) disconnected: ** (Mint.HTTPError) the connection is closed errors from a server for some reason (it's not restarting), should I try this branch too or will it be released soon?

[ruslandoga]

👋 @hkrutzer

Yes, please try it! Regarding the next release, the approach there might be a bit different, using the new handle_info/2 callback.