Add overly-safe synchronization #159
Conversation
There was a problem hiding this comment.
This seems like a step in the right direction, but there is a lot wrong. We are locking way more methods than necessary. We should lock only the shared resources. Only the webserver thread is going to ever call register_plugins, for example. The ircbot thread will only call on_welcome, on_pubmsg, on_currenttopic, etc.
The bot object gets shared in every thread. After a cursory glance only the bot.say method is a problem because multiple threads could write to that socket. Having things that we only use in one thread and making them part of the bot object is also not good. We do this with bot.tasks, but that is only used in the account creation thread.
I think it would be worth exploring async, and having parts of the code written with threads in mind. The IRC library we use now supports async. Right now this solution is not good because it is brittle if some semantics in the threads change in the future and it synchronizes methods that don't need to be.
|
Sure, we should only lock the shared resources, but I contend that we should just lock everything. It's not as elegant, there will be a completely negligible performance impact, but it will let me feel safe merging useful features like #158. If you would like to clearly identify all shared resources between the threads, and lock solely those resources, that would be much appreciated. I also disagree that it's "brittle", because it's overly safe. There only bad thing we can do is by causing deadlock by running a synchronized function endlessly (which we don't do). Python has its GIL, which enforces that only one thread can do anything at a time. This code just merely ensures that the only thread switching can occur when the bot is done processing its current thing. I do agree it's worth exploring async, but that would require almost a complete rewrite of ircbot, something which is very much not a priority. |
|
On Mon Nov 4, 2019 at 10:47 PM fydai wrote:
Sure, we should only lock the shared resources, but I contend that we
should just lock everything.
You aren't locking everything. If you want to do that, put a lock on the
bot object.
It's not as elegant, there will be a completely negligible performance
impact, but it will let me feel safe merging useful features
like #158.
We already "feel safe" using bot.say in the timer thread. Introducing
half-baked locks is not going to provide long term safety.
If you would like to clearly identify all shared resources between the
threads, and lock solely those resources, that would be much
appreciated.
No. If you want thread safety you can do it correctly instead of
implying that I ought to do work for you.
I also disagree that it's "brittle", because it's overly safe.
The entire bot object is being shared between threads. It's brittle
because if new attributes of the bot are read/written to in threads you
have the same problem. Further, you have indicated that you have not
identified all cases of shared resource usage, so this might not even
cover all the existing cases. This patch is only overly safe in that you
are locking methods like on_pubmsg that will only ever be called in one
thread.
|
To be safe, we synchronize pretty much everything (responding to messages, timer-triggered "bot.say", etc). This should make it safe to perform bot methods (such as "say") from multiple threads (in particular, the webserver and the timer thread).