Skip to content

Conversation

@Exirel
Copy link
Contributor

@Exirel Exirel commented Jun 11, 2020

Description

We used async_chat.connected attribute directly in the code, which we shouldn't, so I replaced it with a new method, added to the abstract IRC backend interface.

Also, after further inspection, async_chat's close method can be used safely, so now we call it first, then we ask the bot to do whatever is needed with its on_close method. Since this method expect the connection to be closed already, it shouldn't be a problem.

The timeout scheduler doesn't need to be joined, since none of its jobs requires to wait that much. I also modified the bot's interface to have a new method (irc_send) which will ensure that 1) we don't have any conflict with the async_chat.send method, and 2) makes sure to be thread-safe - the abstract class doesn't need to care about that, it's not its job.

For the record, I've run this version of the bot, with these changes, for more than 36h, and I got 0 problem. I connected to Freenode using a secure connection (ssl on), with an authenticated account. So I feel pretty confident about this.

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 Exirel added this to the 7.1.0 milestone Jun 11, 2020
@Exirel Exirel requested a review from a team June 11, 2020 22:29
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.

Well, this looks straightforward. Just one log message could use tweaking, but I'm not applying the change myself so @Exirel can choose whether to defend his choice, accept my suggestion, or counter with a third option. 😁

@Exirel
Copy link
Contributor Author

Exirel commented Jun 16, 2020

whether to defend his choice

I prefer your version. 👍

@Exirel Exirel requested a review from dgw June 16, 2020 17:17
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.

:shipit: (just lmk if you're not planning to squash)

@Exirel Exirel force-pushed the irc-tweak-backend-interface branch from 2699ea7 to b6529b7 Compare June 17, 2020 06:52
@Exirel
Copy link
Contributor Author

Exirel commented Jun 17, 2020

@dgw all good.

@Exirel
Copy link
Contributor Author

Exirel commented Jun 19, 2020

Btw it also fixes #1821 and maybe @dgw needs to take note of that for the release notes or something?

@dgw
Copy link
Member

dgw commented Jun 19, 2020

Not in the style I've been using, not really. It's such a tiny internal change that it'll get lumped in with some other note about refactoring or cleanup. 😁

@Exirel Exirel deleted the irc-tweak-backend-interface branch June 23, 2020 12:36
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.

2 participants