Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure channel.users is an accurate representation of the NAMES response after updating by removing stale users #118

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

f0x52
Copy link

@f0x52 f0x52 commented Dec 1, 2024

https://github.com/matrix-org/node-irc/blob/master/src/irc.ts#L660
When parsing responses to the NAMES command, (changes to) users are only ever set on the channel.users Map. This causes the channel.users list to perpetually keep growing as new users join, but never remove entries from users that have since left.

onPart() does remove entries, which works most of the time, but it seems these can be missed, most likely due to netsplits.

This fix keeps a separate Map (tmpUsers) where usernames + modes are stored as they come in through onReplyName(). In onReplyNameEnd() the old userlist is cleared, the new entries are copied over (so only the users in the NAMES response remain), and the tmpUsers Map is cleared so it can be used for the next NAMES block.

Found this while debugging why the Matrix side of many bridged rooms has many more IRC users than the IRC side.

…l.users when finished. Ensures no users are left in the list that weren't in the NAMES response
@f0x52 f0x52 requested a review from a team as a code owner December 1, 2024 19:47
@f0x52
Copy link
Author

f0x52 commented Dec 1, 2024

Signed-off-by: f0x [email protected]

@f0x52
Copy link
Author

f0x52 commented Dec 1, 2024

Hmm while trying to yarn link this into matrix-appservice-irc, I'm wondering if it might be better to store the temporary user Map somewhere else, probably doesn't make as much sense to flush it to storage, or even expose it to consumers.

@f0x52
Copy link
Author

f0x52 commented Dec 1, 2024

Alternative approach master...f0x52:clear-names-internal

@progval
Copy link

progval commented Dec 1, 2024

onPart() does remove entries, which works most of the time, but it seems these can be missed, most likely due to netsplits.

That would be a bug in node-irc too. Servers reliably inform clients of every other user who leaves, with a QUIT message.

@f0x52
Copy link
Author

f0x52 commented Dec 1, 2024

Right, it's been difficult to fully debug why this happens, as it does so over time, on production bridges..

I think it might instead be caused by the Client getting reused across connections, so it's missing PART/QUITS while it's offline, and continues to use that older users list

@progval
Copy link

progval commented Dec 1, 2024

If that's the case, then this PR is the right fix. Let's see if it fixes (or reduces) the issue once it's merge

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

Successfully merging this pull request may close these issues.

2 participants