Skip to content

Conversation

@Exirel
Copy link
Contributor

@Exirel Exirel commented Feb 13, 2022

Description

TL;DR: Tin. Close #1414.

Wouhou! This is it, that's the big PR for Sopel 8 that replaces the backend made with the now deprecated asynchat (and asyncore) module by a backend made with asyncio—and it wasn't easy. It might be complicated to review this, even more with all the added stuffs that feel unnecessary (like a heavy dose of type annotation).

I've tested it myself on two networks: Libera (with SSL on), and a private network (without SSL), and it all worked fine, ping timeout was handled properly, server timeout as well, ctrl+c is working fine as far as I can tell, even tho the sequence is a bit complicated if you try to interrupt the bot at the startup sequence (servers tend to wait quite some time before replying to a QUIT command and closing the connection).

So what's in the box?

  • Asynchat based backends have been removed, and replaced by the AsyncioBackend
  • A lot of parameters have been moved from AbstractBackend.run to the __init__ of AsyncioBackend
  • It is now easier to know what are the connection parameters, all put in one single method
  • SSL is now managed with an SSL Context (this makes parts of backends: allow setting TLS version and ciphers #2246 obsolete, sadly, and I haven't put the TLS minimum version (yet))
  • asyncio's high level API is used when possible, instead of working with the low-level API; this means that I didn't dive into Protocol, Transport, Socket, etc. and I decided to use the StreamReader and StreamWriter object
  • There is still a bit of asyncio.Task and asyncio.TimerHandle management involved (mostly for thread-safety and for timeout management)
  • Signal handlers are now managed by the backend itself (it was impossible otherwise)
  • Methods required on the AbstractBot ABC class and AbstractIRCBackend have been created or moved accordingly
  • I had zero trouble with exiting the bot from the start command, so I went maybe a bit too ballistic on that one—but given it now runs an EventLoop... who knows?

What's missing?

  • TLS management wasn't ported (yet) from backends: allow setting TLS version and ciphers #2246
  • I'm not very happy with how on_irc_error and on_error callbacks are used (when/how)
  • I know it works, but I would like to be a bit more sure about the timeout management
  • I've no idea how to deal with CNAME check; I think @half-duplex asked about it on backends: allow setting TLS version and ciphers #2246 and I've to say... why did we check that? I'm not sure it's a good thing to keep...
  • I know it's possible but I haven't tried yet to run every single rule handlers into an asyncio task, trough asyncio.to_thread, and making bot.on_message a coroutine. That would involve a lot of change, while being 100% backward compatible with the current plugins & rules system. The end goal? To allow plugin to define async plugin callables 😎
  • A better documentation.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

@Exirel
Copy link
Contributor Author

Exirel commented Feb 13, 2022

About AsyncioBackend parameters:

image

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually made it through all the code, no thanks to Refined GitHub messing up my Split View (so I disabled the extension). Along the way I ran into a surprisingly low number of "WTF" moments (but don't worry, some did still happen).

My favorite thing about this might be that there's only one backend class now, instead of separate SSL/non-SSL classes. That means the bot doesn't have to dynamically choose a class to use at runtime. Apparently there was no compelling technical reason to build it that way (even if the forced polymorphism was neat just because you don't get to do things like that very often).

ctrl+c is working fine as far as I can tell, even tho the sequence is a bit complicated if you try to interrupt the bot at the startup sequence (servers tend to wait quite some time before replying to a QUIT command and closing the connection)

I've seen "problems" like this with the current implementation too, so don't worry about it. You're maintaining feature parity. :P

  • I know it's possible but I haven't tried yet to run every single rule handlers into an asyncio task, trough asyncio.to_thread, and making bot.on_message a coroutine. That would involve a lot of change, while being 100% backward compatible with the current plugins & rules system. The end goal? To allow plugin to define async plugin callables 😎

This is a good future goal. And it must be a future goal until we are ready to drop support for Python <=3.8, because asyncio.to_thread() isn't available until Python 3.9.

@Exirel
Copy link
Contributor Author

Exirel commented Feb 14, 2022

This is a good future goal. And it must be a future goal until we are ready to drop support for Python <=3.8, because asyncio.to_thread() isn't available until Python 3.9.

Hm... yes, you're right about to_thread. However, we can already work with thread using loop.run_in_executor() instead, I was just too caught up into "the future" that I forgot that the convenient/shortcut function only exists in 3.9. 😁

@dgw
Copy link
Member

dgw commented Feb 15, 2022

Then it depends on whether you want to write the code twice, or wait until you can use the shortcut and write it only once. :P

Copy link
Member

@half-duplex half-duplex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of abandoning the CNAME stuff. I don't like mucking with things like hostname verification, and it was introduced specifically for Twitch, where it's easily worked around by updating configs to point where the CNAME points.

ℹ️ This drops do_handshake_on_connect=False and suppress_ragged_eofs=True, introduced in 614abe1. asyncio should handle the first, and the second is the default.

I haven't played with this, since I need the other TLS changes from my PR, which I don't want to redo until this one is merged.

@Exirel Exirel force-pushed the irc-backend-asyncio branch from 1a105e5 to 9830e11 Compare May 12, 2022 18:04
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit of IRC chatter to clear up one logic-related question about CA certs, but we are probably going to remove that feature so I'm not bothered about it any more. Just one line note to save some of Exi's sanity during squash.

@dgw dgw requested a review from half-duplex May 13, 2022 19:23
@Exirel Exirel force-pushed the irc-backend-asyncio branch from 39184fa to 7dba103 Compare May 13, 2022 21:59
@Exirel Exirel marked this pull request as ready for review May 13, 2022 22:23
@dgw dgw merged commit 1095ad1 into sopel-irc:master May 15, 2022
@Exirel Exirel deleted the irc-backend-asyncio branch June 12, 2022 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rewrite to use asyncio instead of asynchat

3 participants