Backend is not initialized during plugin setup

[dgw]

Problem analysis

When the AbstractBot that forms the foundation of a Sopel instance is initialized, it sets its backend to None:

https://github.com/sopel-irc/sopel/blob/53c5543d15a73aaba904ee5093212a575b3b6cf9/sopel/irc/__init__.py#L75-L76

This attribute isn't populated until the bot's run() method gets called:

https://github.com/sopel-irc/sopel/blob/53c5543d15a73aaba904ee5093212a575b3b6cf9/sopel/irc/__init__.py#L254-L263

Calling run() initiates the connection to the IRC network, so all plugins (especially coretasks) must be loaded by this point. But this means that before run() is called, there is no backend, and therefore plugins that check the state of the IRC connection "the nice way" (by checking bot.backend.is_connected()) will raise an AttributeError if that check runs during the setup phase, while the backend is still None.

One such plugin is remind, which starts a scheduled task that dutifully checks (as of #2351) whether the IRC connection is ready yet:

https://github.com/sopel-irc/sopel/blob/53c5543d15a73aaba904ee5093212a575b3b6cf9/sopel/modules/remind.py#L163-L165

Per the documentation for @plugin.interval, that scheduled task may be called before the bot is connected to IRC, and sometimes it is. This previously (before #2351) led to a RuntimeError because the plugin tried to say() messages before the backend was initialized, and those messages were subsequently lost. Now that the plugin is more well behaved, it causes the above-mentioned AttributeError instead under the same circumstances—but at least now it no longer silently eats the offending reminders.

Proposed solution

I would like to separate initialization of the AbstractBot.backend attribute from running the backend, so the backend will be available to query even if a scheduled task happens to run before the network connection is initiated.

The reason I'm putting this up for discussion first is that it would be a redesign of the interface. The host and port used by the backend are currently passed in through a long chain of method calls:

• cli.run.command_start(opts) (when sopel runs with the [default] start action)
  • cli.run.run(settings, pid_file_path)
    • irc.AbstractBot.run(host, port)
    • irc.AbstractBot.get_irc_backend(host, port, source_address)

You can see that along the way, the host and port are turned into explicit arguments for some reason, despite the fact that irc.AbstractBot.run() has access to self.settings and can fetch those options for itself without needing them to be passed in. Likewise, irc.AbstractBot.__init__() receives only one argument, the settings, and can therefore also determine for itself the host, port, and source_address arguments needed by irc.AbstractBot.get_irc_backend(), which in turn also has access to self.settings, so it doesn't actually need any arguments at all.

Why don't you Just Do It™?

My first thought was that things are built this way to make unit tests easier, but it turns out that neither irc.AbstractBot.run() nor irc.AbstractBot.get_irc_backend() are covered by the test suite at this moment. It would therefore be pretty easy to change things. Despite that, because the current architecture is @Exirel's brainchild, I'd love to get some insight on why it is the way it is before I start remodeling the place.

Plus, there might be a better solution that I haven't thought of, and the only way to have a chance at finding it is to see if any of the other contributors have other ideas. 😁

[Exirel]

Yeah that interface isn't the best there is. However, there is a simpler solution to the current problem (two, actually):

• stop using bot.backend.is_connected() where the plugin meant bot.connection_registered
• initialize self.backend: AbstractIRCBackend = NotConnectedBackend() in irc.AbstractBot class instead of None

These are not mutually exclusive, quite the contrary.

The NotConnectedBackend class would set some default value (such as is_connected() always returning False), and raise some NotConnectedError when trying to call bot.say().

[dgw]

there is a simpler solution to the current problem (two, actually):

• stop using bot.backend.is_connected() where the plugin meant bot.connection_registered.

A quick search of my IRC logs shows we (me and Exi) talked about it on 28 September (around when I was working on #2351), and the problem with using bot.connection_registered remains the fact that it is never set back to False if the connection is lost, while bot.backend.is_connected() does reflect the correct state in the case of timeouts, ECONNRESET, etc.

However, is_connected() and connection_registered technically mean different things at the moment: "connected" means the socket is open, but "registered" isn't true until the bot receives RPL_WELCOME. For the use case in remind, we need to know if the connection is "registered" (because only then is it valid to send PRIVMSGs to IRC, even though the backend may allow doing so as soon as the "connected" state is reached).

To fix the remind bug that brought this up again, and other issues like it, the simpler solution would work assuming that bot.connection_registered is fixed to not contain True after the bot gets disconnected.

Because I kinda want to fix connection_registered anyway regardless of where this thing with the self._backend = None assignment, I'll start working on that while we continue thinking about this.

[Exirel]

I'm going to implement a NotConnectedBackend or NotInitializedBackend or something like that (name to be defined, I'm really not sure here), so there is always an object to use an interrogate, so there is no NoneType has no attribute <fn/attr> when using the bot in an unexpected way.

(i.e. I'm working on it right now)

[dgw]

Discovered that I got the jump on @Exirel by starting on the dummy backend, work on which now lives in #2394.