Add a lock to the bot's say function #167
Conversation
| @@ -135,6 +135,9 @@ def __init__( | |||
| self.plugins: Dict[str, ModuleType] = {} | |||
| self.extra_channels: Set[str] = set() # plugins can add stuff here | |||
|
|
|||
| # As we use threads, we should ensure that we use them safely | |||
| self.lock = threading.RLock() | |||
There was a problem hiding this comment.
Why did you use an RLock here? It doesn't look to me like this lock will ever be taken twice in the same thread.
|
We should lock other interactions that use the IRC connection, like changing the topic. |
|
By all means look into it, but if we let perfect be the enemy of the good here the IRC bot will remain stagnant forever. I would argue that any effort spent on getting the synchronization exactly right is better directed towards refactoring the bot entirely. |
This needs a lock because multiple messages are being sent. Changing the topic doesn't create multiple messages. |
| # Find the length of the full message | ||
| msg_len = len(f'PRIVMSG {channel} :{message}\r\n'.encode('utf-8')) | ||
|
|
||
| # The message must be split up if over the length limit | ||
| if msg_len > MAX_CLIENT_MSG: | ||
| messages = split_utf8(message.encode('utf-8'), MAX_CLIENT_MSG) | ||
|
|
There was a problem hiding this comment.
This math part doesn't need the lock. Only sending multiple messages needs the lock.
|
The lock should be held whenever the socket needs to be used (that is, whenever we interact with the IRC server) because sockets in Python are not thread safe. |
We are currently accessing the say function from multiple threads (celery, web server, and standard server) all use it. Adding a lock would remove any possible synchronization issues from using multiple threads.