Gracefuly check connection status with server PINGs

[LiraNuna]

Currently, every 5 minutes of no data received, the connection will terminate, assuming the server is dead or another issue has occurred. Some servers, such as irc.mzima.net have a higher default PING timeout than 5 minutes, causing false-positives of dead-sockets.

This PR changes this behaviour to instead, PING the server to check if it's still there - when we don't hear back, we THEN terminate the connection.

Fixes #51

[theunkn0wn1]

Looks like you targeted the master branch with these changes, I would prefer you to target develop.

I have changed the pull request's target branch, but could you please rebase your commit against upstream/develop?

master is reserved for releases, whereas develop is used as the destination for pull requests for the next release.
As for the content of the changes, they look mostly good to me, short one or two minor things.

[shizmob]

The MR looks otherwise good to me!

Thanks a lot for doing this work, and participating in the discussion upfront about it.
The traditional behaviour has been a long-standing issue caused by my early interpretation
of RFC1459, but I think this new behaviour makes more sense.

[LiraNuna]

master is reserved for releases, whereas develop is used as the destination for pull requests for the next release.

I apologize for that. I believe there's a way to select a default PR branch in github for that purpose.

[shizmob]

I apologize for that. I believe there's a way to select a default PR branch in github for that purpose.

Good call, I've changed this in the project settings now. This wasn't done before as I believe @theunkn0wn1 has no access to those settings, and moving the main development HEAD to develop was mostly his work.

[theunkn0wn1]

LGTM 👍

[LiraNuna]

Please note I have changed PING_TIMEOUT to READ_TIMEOUT, which MAY be a breaking change to some users. I would probably add a note in the CHANGELOG before releasing or add a compatibility layer of some sort.

[theunkn0wn1]

Yes I had noticed that, Upon consideration we should keep the old name, or alias it with a property. Otherwise it will take a major point release to merge this.

[shizmob]

I did notice that -- I was going to propose to add a property that proxies to READ_TIMEOUT for compatibility, but figured that didn't need to impact the MR itself.

Unrelatedly, I think the current reconnect code is a bit flaky as well; once that has been fixed, it seems the connection problems would finally be gone for good.

[theunkn0wn1]

I was going to touch the reconnect code soon anyways, as i intend to work in #117 over the weekend. My current idea will require changing how pydle expects a disconnect.

[LiraNuna]

I did notice that -- I was going to propose to add a property that proxies to READ_TIMEOUT for compatibility, but figured that didn't need to impact the MR itself.

Just so we're on the same page, is there anything you'd like me to do? should I add the proxy property or defer to a later PR? I would love to see this merged and ideally released because right now I am using this library with a modified sites-packages for my issue to be resolved.

Personally I don't think this is a huge breaking change because if someone DID modify PING_TIMEOUT, it's most likely to increase the amount in order to avoid the incorrect behavior, now that it's gone it's really not needed anymore so it won't really break anything behavior wise, as the timeout handling is now in the library instead of being a client's responsibility.

Let me know how you'd like to proceed.

[theunkn0wn1]

Since this resolves an immediate issue here is what I think is best:
We merge this PR now as-is, with the understanding that a proxy property will be added in a later PR.

I am drafting this fix up shortly, so we can do a point release soon. If anyone is both dependent on modifying the renamed variable and is using the develop branch, they should be aware of the risk to using a development branch rather than a release branch.