search

shizmob / pydle

An IRCv3-compliant Python 3 IRC library.
BSD 3-Clause "New" or "Revised" License
154 stars 48 forks source link

Gracefuly check connection status with server PINGs #124

Closed LiraNuna closed 5 years ago

LiraNuna commented 5 years ago

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 https://github.com/Shizmob/pydle/issues/51

shizmob commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

LiraNuna commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.