reconnect when EOF

[critical27]

Almost same as #411. Two difference:

1. use AsyncSocket::hangup to tell if we encounter EOF, in chaos graphd will failed to send any request to storage after storage reboots in worst case. As @guojun85 said in Fix problem: storage restarted but reconnect is not working. #411, readable will return true while the connection is still working (during my pressure test, it will return true occasionally, once I stop the pressure, no more true is returned), that is probably because it use POLLIN, and default read buffer if quite small. And AsyncSocket::hangup use POLLRDHUP | POLLHUP, which is what we want.
2. remove duplicate check in impl(), it is uesless now because we didn't use ReconnectRequestChannel, and ReconnectingRequestChannel::impl() will be invoked multiple times, and check more than once. After this PR, we manage retry by ourself.

@guojun85, PTAL, thx

[critical27]

Hold on a second, there maybe a way to make changes minimum.

[critical27]

I believe it is the right solution now, ready to review.

[liuyu85cn]

LGTM

[panda-sheep]

well done!

[critical27]

Wait dutor to have a look

[bright-starry-sky]

Wait dutor to have a look

👌

[dutor]

Skimmed through code of the AsyncSocket transport, I think calling transport->good() is enough, hangup() is unnecessary. Because if the remote side closes the connection(either restarted or crashed), there will be a EPOLLIN event which triggers a read action which returns a EOF, then the socket states will be set to indicate not good()

You could have a try to verify it.

[dutor]

Skimmed through code of the AsyncSocket transport, I think calling transport->good() is enough, hangup() is unnecessary. Because if the remote side closes the connection(either restarted or crashed), there will be a EPOLLIN event which triggers a read action which returns a EOF, then the socket states will be set to indicate not good()

You could have a try to verify it.

emmm. It seems that channel->good() calls transport->good() for us. It think we could just check if the channel is good, then create one if not.

[critical27]

Skimmed through code of the AsyncSocket transport, I think calling transport->good() is enough, hangup() is unnecessary. Because if the remote side closes the connection(either restarted or crashed), there will be a EPOLLIN event which triggers a read action which returns a EOF, then the socket states will be set to indicate not good()
You could have a try to verify it.

emmm. It seems that channel->good() calls transport->good() for us. It think we could just check if the channel is good, then create one if not.

transport->good() is AsyncSocket::good() actually, and it did not check POLLIN, however AsyncSocket::readable() will check POLLIN.

During my test, readable will be effected by pressure, and will return true even the socket is valid. You could try #411 and you will find raft client transport will be released with a little pressure, very easy to reproduce.

[dutor]

Skimmed through code of the AsyncSocket transport, I think calling transport->good() is enough, hangup() is unnecessary. Because if the remote side closes the connection(either restarted or crashed), there will be a EPOLLIN event which triggers a read action which returns a EOF, then the socket states will be set to indicate not good()

You could have a try to verify it.

After digging, we found out that, after each RPC call finishes, READ event of the connection will be removed from the event set, so that even the connection is lost the client side won't be able to update the state. So we have to be ask to kernel about the connection state, which is what hangup does.

[dutor]

During my test, readable will be effected by pressure, and will return true even the socket is valid. You could try #411 and you will find raft client transport will be released with a little pressure, very easy to reproduce.

This is because a disconnect from the remove side will incur a READ event on the client side, so the connection IS readable in terms of event handling.

[critical27]

Skimmed through code of the AsyncSocket transport, I think calling transport->good() is enough, hangup() is unnecessary. Because if the remote side closes the connection(either restarted or crashed), there will be a EPOLLIN event which triggers a read action which returns a EOF, then the socket states will be set to indicate not good()
You could have a try to verify it.

After digging, we found out that, after each RPC call finishes, READ event of the connection will be removed from the event set, so that even the connection is lost the client side won't be able to update the state. So we have to be ask to kernel about the connection state, which is what hangup does.

👍 , let me have a look.

[critical27]

Ready to go, close #411

[panda-sheep]

Should use tempHost instead of host？

[critical27]

It doesn't matter, actually validate won't change the value of the parameter, so tempHost will be equal to host.

[critical27]

I know what you mean, good catch

[dutor]

[panda-sheep]

Good job

[dutor]