Let's stop using sleep()

[dgw]

In recent bugfix testing, I discovered that Sopel uses time.sleep() in some inconvenient places.

For example, pressing Ctrl-C while the bot is waiting to reconnect previously raised an exception, but fixing that bug reveals that Sopel now appears to hang for some number of seconds until the sleep() call finishes. Calling sleep() blocks that entire thread of execution, after all, so no received signals can be processed until the sleep time has elapsed.

There are probably other places like that hiding in the code, too, which is why I didn't write this issue specifically about that one case.

I'm hoping we can develop a more responsive way of doing things, maybe using threading.Events. Free admission: I haven't thought about this a lot yet, because I just discovered the issue, and my ideas on how to solve it are probably still way off the mark. 😁

Knowing @Exirel's love of refactoring, I'm going to tentatively assign this to him. No milestone because it's not that important, and I don't mind delaying this for a while until there are a lot fewer more-pressing things to do.

[Exirel]

👍

[HumorBaby]

For example, pressing CTRL+C while the bot is waiting to reconnect previously raised an exception, but fixing that bug reveals that Sopel now appears to hang for some number of seconds until the sleep() call finishes.

I cannot get this behavior to replicate. Not sure how you are forcing the disconnected/waiting to reconnect state, but I can confirm at least this behavior:
/kline the bot to force disconnect from server --> bot is now in disconnected/waiting to reconnect state --> CTRL+C quits immediately, despite the time.sleep(delay).

This indicates to me that it is not in fact the time.sleep that is causing this, but instead is possibly some artifact of how the CTRL+C was tested. Can you share how you got the bot in the disconnected/waiting for reconnection state?

A cursory trace indicated that an unexpected socket state was the more likely culprit.

[dgw]

Those terminal logs are gone, but I believe the test involved configuring Sopel to connect to a server that would time out or refuse the connection.

[Exirel]

I was randomly looking at things and stumbled upon the remind module.

With #1479 and #1557 it should be safer to use the JobScheduler instead of that... thing.

[dgw]

Yeah, that's more than a little ugly. Let's try to get as many rewrites like that done as we can.

Do you need #1479 merged before starting on a refactor of remind? Honestly, #1479 is so big I can't easily remember everything it touches, but you wrote it. :P

[Exirel]

Do you need #1479 merged before starting on a refactor of remind?

No if #1479 gets merged at some point for 7.x. Yes if it isn't. #1479 makes the setup/shutdown cycle better for the job scheduler, combined with a consistent behavior with reload (calling shutdown and removing properly jobs).

On the other hand, the current job scheduler is still better than this rogue thread used by remind.py.

[Exirel]

Quick note here: in the last few months, we decreased the need for time.sleep, using them when it's necessary. I don't think this is a big issue anymore. In particular:

• new setup/shutdown routine
• better handling of scheduled jobs
• improved flood protection
• no more unmonitored wild threads

So, even if there are still some occurrence of time.sleep, they are reduced to a strict minimum.

[dgw]

We're down to 4 calls of time.sleep() in the whole codebase, as far as I can tell. Good enough.