allow client to reconnect if other side closes connection

[cybernetlab]

It would be nice to allow framed client to reconnect if other side closed connection because of client inactivity for example. In my personal case thrift library used as a client for hbase and this server closes inactive connections in about 10 minutes. Also I'm using poolboy to pool connections and without reconnection ability I'll need to do some unnecessary logic to monitor status of tcp connections and push out clients with closed connection from the pool.

[cybernetlab]

Ah, sorry - I've forgotten to resend data after reconnection - some additional fixes needed

[cybernetlab]

Now all works fine in my environment. What should I do with test coverage? ) Where are no any tests related to Elixir.Connection behaviour in client_test.exs.. Should I implement some additional test cases?

[cybernetlab]

However would using :inet.setopts(sock, active: :once) in the client process allow us to handle idle disconnect earlier and not have to retry a request? We should usually get a close notification message, and can reconnect proactively? Then instead of :gen_tcp.recv we could do receive do {:tcp, ^socket, data} -> etc.

Agree. The better solution is to handle disconnects at the moment of disconnect. I think that we can change active option to false right before :gen_tcp.send and switch it back to :once after :gen_tcp.recv. Is it reasonable approach?

Also its not always the case that all connection closes can be retried, right now this could double the request rate to a badly behaving service :(.

Why it doubles request rate? If connection is closed at the moment of sending then where are no data sent at all and it will sent once after reconnect

[cybernetlab]

@fishcakez I'm ready for your re-review )

[fishcakez]

Agree. The better solution is to handle disconnects at the moment of disconnect. I think that we can change active option to false right before :gen_tcp.send and switch it back to :once after :gen_tcp.recv. Is it reasonable approach?

I like what you did with active: true, left comment about always handling all socket messages. Outside a request any message is an issue and we could close.

Why it doubles request rate? If connection is closed at the moment of sending then where are no data sent at all and it will sent once after reconnect.

Unfortunately with TCP we can not tell why the socket closed. There are three main scenarios:

1. The socket closed before we sent the request, and we'd like to retry as we never go to send the request.

2. The socket closed at some point after we sent the request, and the server never received the full request so didn't try to handle it.

3. The socket closed after the server received the request. This is the case where we could increase the load on the server because the server could receive the request multiple times but the socket is closed before it can send a response. Most likely there is a bug in the server that causes the socket to get closed. A problematic example would be with the thrift server in this repo, and using Task.async inside the handler function. If the task processes crashes then the server processes will get an exit signal and exit - leading to the socket being closed without sending a response. As the client does not get a response it would resend the request, doubling the qps. This double qps occurence is quite concern to me.

I wonder if handling the disconnect immediately (active: true with message handling) is enough for the issue you're seeing as we would catch scenario 1) most of the time but never retry on 2) or 3). Perhaps we can split this PR in two, and follow up on reconnect retry in a second one if its still a problem.

[cybernetlab]

I wonder if handling the disconnect immediately (active: true with message handling) is enough for the issue you're seeing as we would catch scenario 1) most of the time but never retry on 2) or 3). Perhaps we can split this PR in two, and follow up on reconnect retry in a second one if its still a problem.

I'm writing this PR not only for my case but for any possible similar cases. But I agree that in case of 2) and 3) we should not resend data

[cybernetlab]

@fishcakez I've kept active: true and remove resending logic. Ready to review

[cybernetlab]

Hi, guys! I need a SASL authentication for hbase. I can prepare another PR with auth backend support, but I need this PR to be included in my code too (In my mix.exs I've github dependency that points to my repo with this PR). How can I achive this? Add my changes in this PR? As I understand, the better would be to create a new PR after merging this one... Thanks!

[cybernetlab]

@fishcakez ready to review )

[cybernetlab]

@jparise is it enough to add:

The :reconnect option if setted to true forces client to reopen tcp connection whenever it closed.

as a last paragraph to the end of @doc for start_link/3?

[jparise]

@jparise is it enough to add:

The :reconnect option if setted to true forces client to reopen tcp connection whenever it closed.

s/setted/set

as a last paragraph to the end of @doc for start_link/3 ?

Yes, that would be fine.