Skip to content
This repository has been archived by the owner on Oct 2, 2024. It is now read-only.

Add thread lock to IRC connection #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add thread lock to IRC connection #90

wants to merge 1 commit into from

Conversation

dkess
Copy link
Member

@dkess dkess commented Jan 14, 2019

The irc library uses Python sockets, which aren't thread-safe. This PR adds a lock and acquires the lock for any operation that sends to the IRC server.

Copy link
Member

@kkuehlz kkuehlz left a comment

Choose a reason for hiding this comment

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

Nice. This is a really good change. I think for some of these operations the lock is being acquired too early. I left comments to explain.

message,
).encode('utf-8'))

# The message must be split up if over the length limit
Copy link
Member

Choose a reason for hiding this comment

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

we only really need to acquire the lock here. you aren't using the connection when getting the message length

).encode('utf-8'))

# The message must be split up if over the length limit
if msg_len > MAX_CLIENT_MSG:
Copy link
Member

Choose a reason for hiding this comment

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

can't we just simplify this logic and remove the if statement? with python slices , if we are less than MAX_CLIENT_MSG we will just have an array of size 1.

Then we can simplify the logic further and only acquire the lock before the loop

return '{}: {}'.format(m.group(1), int(m.group(2)) + 1)
with self.sendmsg_lock:
for channel, topic in self.topics.items():
def plusone(m):
Copy link
Member

Choose a reason for hiding this comment

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

nit: would just make this a lambda

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants