-
-
Notifications
You must be signed in to change notification settings - Fork 409
irc: properly manage exception of the run-forever loop #2431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dgw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm nitpicking… Maybe the final commit/PR title line should say "exceptions in the run-forever loop"?
246d766 to
c1e0cc7
Compare
Co-authored-by: dgw <[email protected]>
ba1a4ac to
ad21253
Compare
dgw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to wait quite a while for my test bots to time out, but they both disconnected due to ping timeout at the same minute, and both reconnected automatically about 30 seconds later with no manual intervention. Win for this patch!
Both affected bots were started a few seconds apart, and pinged out a few seconds apart, pointing to an actual, brief, network issue as the root cause. In both bots' logs (example below), there is clearly time between the final PING and "Reached timeout" for another PING attempt to avert closing the socket after only one PONG is missed. It makes me think Sopel's ping timeout detection might warrant some further tweaks, perhaps in 8.1.
[2023-05-19 09:29:34,886] sopel.irc.backends DEBUG - Sending PING after 54.0s of inactivity.
[2023-05-19 09:30:29,037] sopel.irc.backends DEBUG - Sending PING after 54.0s of inactivity.
[2023-05-19 09:31:35,074] sopel.irc.backends WARNING - Reached timeout (120.0s); closing connection.
[2023-05-19 09:31:35,075] sopel.irc.backends DEBUG - Read task was cancelled.
[2023-05-19 09:31:35,076] sopel.irc.backends DEBUG - Shutting down writer.
[2023-05-19 09:31:40,743] sopel.irc.backends ERROR - Unexpected error while shutting down socket.
[2023-05-19 09:31:40,753] sopel.irc.backends DEBUG - All clear, exiting now.
[2023-05-19 09:31:40,758] sopel.irc.backends INFO - Connection backend stopped.
[2023-05-19 09:31:40,758] sopel.bot INFO - Shutting down
[2023-05-19 09:31:40,759] sopel.bot INFO - Stopping the Job Scheduler.
[2023-05-19 09:31:40,865] sopel.bot INFO - Job Scheduler stopped.
[2023-05-19 09:31:40,866] sopel.plugins.jobs DEBUG - Successfully unregistered all jobs
[2023-05-19 09:31:40,866] sopel.bot INFO - Calling shutdown for 2 plugins.
[2023-05-19 09:31:40,866] sopel.bot DEBUG - Calling sopel.modules.find.shutdown
[2023-05-19 09:31:40,867] sopel.bot DEBUG - Calling sopel.coretasks.shutdown
[2023-05-19 09:31:40,867] sopel.cli.run WARNING - Disconnected. Reconnecting in 20 seconds...
[2023-05-19 09:32:00,908] sopel.bot INFO - Loading plugins...
|
Base branch gymnastics to make GH update its (cached, I assume) idea of which commits are new in this PR. Can merge now. |
Description
Tin. Based on #2430, closes #2350.
To elaborate a bit: I realized that having a connection error and other uncaught exception in the run-forever loop (await read task, await write task) would lock the bot, and so it's not just friendly error messages, it is also a better exception handling.
Checklist
make qa(runsmake qualityandmake test)