shizmob / pydle

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

Various reconnection fixes #102

Closed theunkn0wn1 closed 5 years ago

theunkn0wn1 commented 5 years ago

An issue exists within Pydle's reconnection code, which was most probably introduced in #79 .

During reconnection after a graceless disconnection a bad call to call_soon was made. The offending line,

self.eventloop.call_soon(delay, self.connect(reconnect=True))

The first error here is the signature for call_soon is (callback, delay); the arguments in our call are out of order. Secondly, call_soon expects a synchronous callback which won't work since self.connect is now asynchronous.

The solution was to properly re-implement the desired end behavior using modern asyncio features. We pause the disconnect coroutine via asyncio.sleep(delay), then await connection().

A small lingering issue seems to be Pydle takes a long time reconnecting, it seems an issue remains where the first reconnection attempt times out however the second attempt succeeds. closes #101 closes #103 closes #99

theunkn0wn1 commented 5 years ago

I have added a fix for #103 since the two bugs are related.

there still appears to be an issue with registration on the first reconnect attempt, further investigation is required.

Unknown command: [irc.unknown.com] 396 ['Pydlebot-unknown', 'BE9A4370.314F469.E00AF3E3.IP', 'is now your displayed host']
Encountered error on socket.
TimeoutError: Ping timeout: no data received from server in 50 seconds.
Unexpected disconnect. Attempting to reconnect.
Task exception was never retrieved
future: <Task finished coro=<BasicClient._perform_ping_timeout() done, defined at J:\worspaces\pycharm\fuelrats\forks\theunkn0wn1\pydle\pydle\client.py:159> exception=CancelledError()>
concurrent.futures._base.CancelledError
Encountered error on socket.
pydle.features.rfc1459.protocol.ServerError: Closing Link: [172.19.0.1] (Registration Timeout)
Unexpected disconnect. Attempting to reconnect within 5 seconds.
Unknown command: [irc.unknown.com] 396 ['Pydlebot-unknown', 'BE9A4370.314F469.E00AF3E3.IP', 'is now your displayed host']
theunkn0wn1 commented 5 years ago

Fixed the Cancellation related errors, canceling a running task raises a CancellationError, which is a little

fixed Client.disconnect still being synchronous while the underlying ._disconnect was async. This was leading to some misbehavior with future scheduling so i went ahead and made it async so the event handling flow is more linear as it should be.

I have made Client.handle_forever a async Task and store its handle. This allows us to cancel this event during disconnection. This is necessary as it does not properly exit on its own (possibly due to being executed out-of-order, joys of async programming)

Without this, exceptions may be raised when a new connection to the server is made during reconnection.

theunkn0wn1 commented 5 years ago

Identified and fixed two additional reconnection related bugs. First one was rather simple, child on_connect handlers in the codebase didn't call their superclass, resulting in the reconnection attempt timer not being reset upon successful connection in most instances.

Second one took a bit more digging. An issue exists where, upon first attempt to reconnect, registration is skipped. This causes the IRC server to eventually kick the bot off the network. Here is a raw irc conversation between pydle and an IRC server that illustrates the bug image

Here is a snippet of what a normal, successful connection stream looks like image Red is pydle, blue is server.

The root cause of this lies within pydle.client._disconnect https://github.com/Shizmob/pydle/blob/develop/pydle/client.py#L124-L136

The problem stems from the on_disconnect callback being called prior to _reset_attributes. Specifically, during connection this line causes registration to be incorrectly skipped and directly causes the subsequent registration timeout.

This issue was resolved by calling _reset_attributes prior to the on_disconnect handler.