Schedule a reconnection check when starting the bot

jaraco / irc

Full-featured Python IRC library for Python.

MIT License

390 stars 84 forks source link

Schedule a reconnection check when starting the bot #156

Closed palvarez89 closed 5 years ago

palvarez89 commented 5 years ago

If the first attept of connecting to the IRC server fails, the scheduled reconnection check will ensure we start executing the configured reconnection strategy.

Before this change no other connection attempts were executed, given that the exception in _connect was ignored and _on_disconnect was never triggered.

jaraco commented 5 years ago

I don't feel comfortable with this change for a few reasons:

it causes the code to call recon.run() in more than one place, making it harder to analyze and verify the behavior
the recon behavior is triggered irrespective of whether the connect operation failed. This means that if a connection is successfully established, a ReconnectStrategy will begin running, signaling to the relevant strategy that a disconnect just occurred (which in most cases won't be the case).
the documentation for the abstract ReconnectStrategy indicates that run is invoked by the bot "on disconnect", but this call happens after a failed (or successful) connect operation.
the name "reconnect" sort-of implies that an initial connection was established.
this code has no tests or even a comment, so could be optimized away.

I think the point you made about the error in _connect being suppressed is itself a more serious issue. I wonder why that behavior is the way it is.

jaraco commented 5 years ago

That try/except block has been around a very long time, but removing it doesn't cause any tests to fail... so it's difficult to say exactly why it's around.

Would you consider revisiting this request in light of my concerns above?

palvarez89 commented 5 years ago

Thanks for taking the time to review the patch and explaining those useful points. I agree with all of them.

I think the point you made about the error in _connect being suppressed is itself a more serious issue. I wonder why that behavior is the way it is.

That try/except block has been around a very long time, but removing it doesn't cause any tests to fail... so it's difficult to say exactly why it's around.

I guess that what I could do is to issue a reconnection request if an exception running _connect happens.

the name "reconnect" sort-of implies that an initial connection was established.

If we end up implementing this, could we say that "reconnect" name is also valid for when an initial connection attempt was done, but failed?

jaraco commented 5 years ago

If we end up implementing this, could we say that "reconnect" name is also valid for when an initial connection attempt was done, but failed?

Yes, that seems reasonable.