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

Rate limit outgoing JOIN messages #3115

Merged
merged 22 commits into from Aug 4, 2021
Merged

Rate limit outgoing JOIN messages #3115

merged 22 commits into from Aug 4, 2021

Conversation

zneix
Copy link
Collaborator

@zneix zneix commented Aug 4, 2021

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Added experimental rate-limitting for outgoing JOIN messages with some imitation of Leaky bucket rate limitting. Huge thanks to @leon-richardt for helping me figure out the theory.
Tested it out and Twitch does not kill our connection - we conform to Twitch's 20 JOIN messages / 10s limit by sending at most 18 JOIN messages per 10.5s.

This might not be the best solution and I believe there could be a better one, but after about 24 hours of thinking this is the most sane solution I could come up with and given how important this bug is I think it's better than nothing.

There's still debug prints in the code, to make it easier for reviewers to test. After approving the PR I'll remove them. already removed.

Closes #3107

I tried using Leaky rate limit bucket or however it's called, no idea if this is the best approach, but it's better than nothing as of right now I guess.
Also as advised, make bucket a member of AbstractIrcServer

11:51:38.116 YungLPR: zneix, whenever you use new directly, think twice
@zneix zneix requested review from Mm2PL and pajlada August 4, 2021 10:07
chatterino.pro Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

This solution currently doesn't handle JOINs from when a user opens a new split - let's make sure we solve that in the same PR

Also decided to move the bucket declaration to where AbstractIrcServer itself is initialized to make sure this->bucket_ is initialized at all times.
@zneix
Copy link
Collaborator Author

zneix commented Aug 4, 2021

This solution currently doesn't handle JOINs from when a user opens a new split - let's make sure we solve that in the same PR

Should be resolved in 61b0f39. Also, since that makes the use of this->bucket_ outside of AbstractIrcServer::onReadConnected I think it's better to move initialization of it to where AbstractIrcServer itself is initizalied.

src/providers/irc/AbstractIrcServer.hpp Outdated Show resolved Hide resolved
src/providers/irc/AbstractIrcServer.cpp Outdated Show resolved Hide resolved
src/util/RatelimitBucket.cpp Outdated Show resolved Hide resolved
src/util/RatelimitBucket.hpp Outdated Show resolved Hide resolved
src/util/RatelimitBucket.cpp Outdated Show resolved Hide resolved
@sando
Copy link
Contributor

sando commented Aug 4, 2021

eb3e75c was crashing for me shortly after start up.

a60d806 seems to be working, so far so good! Thanks for the quick work on this. ❤️

46 channels open (and they all appear to be actually joined rather than just "blank" splits), Windows 11 21H2 (22000.100), chatterino-windows-x86-64-5.12.10-cmake

As part of this change, the logic is now that budget_ contains the
amount of calls we could do before we will have to wait for a fillup.

Because of this change, we no longer need to store the `limit_`, but
rather just use that to initially "fill up the budget"
pajlada and others added 2 commits August 4, 2021 16:28
@ElZigSGC
Copy link

ElZigSGC commented Aug 4, 2021

Works fine for me.
Thanks for the fast answer on that issue and for all the work done.

@zneix zneix requested a review from fourtf August 4, 2021 15:46
@zneix zneix requested a review from pajlada August 4, 2021 15:46
Copy link
Collaborator

@Mm2PL Mm2PL left a comment

Choose a reason for hiding this comment

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

looks and works as expected, but I have a couple nit picks

src/providers/irc/AbstractIrcServer.cpp Show resolved Hide resolved
src/util/RatelimitBucket.hpp Outdated Show resolved Hide resolved
src/util/RatelimitBucket.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@talneoran talneoran left a comment

Choose a reason for hiding this comment

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

Seems to work well to fix the issue, tested with 30+ channels and for a few edge cases.

When testing I found it's possible for a duplicate JOIN to be queued and sent. This happens if while a channel is queued and on cooldown to JOIN, the split with it is closed and reopened, causing it to be inserted again into the queue.
In our discussion offline we decided this is a rare enough case and so it can be ignored at least for now, I'm leaving this here for documentation.

@no1dead
Copy link

no1dead commented Aug 4, 2021

Using this change and haven't run into any issues with it.

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

After testing with 80 channels this works well for me

We have some further UX work, but this is a great first step

CHANGELOG.md Show resolved Hide resolved
src/providers/irc/AbstractIrcServer.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@Felanbird Felanbird left a comment

Choose a reason for hiding this comment

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

Changelog is good, functionality works as well, tested with 86 channels. 👍

@zneix zneix enabled auto-merge (squash) August 4, 2021 20:59
@zneix zneix merged commit de4f6a9 into master Aug 4, 2021
@zneix zneix deleted the zneix/fix/limit-joins branch August 4, 2021 21:18
zneix added a commit to SevenTV/chatterino7 that referenced this pull request Aug 4, 2021
Now we're on commit de4f6a9; Changes from upstream we've pulled:

- Major: Fixed constant disconnections with more than 20 channels by rate-limiting outgoing JOIN messages. (Chatterino#3112, Chatterino#3115)
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.

Chatterino keeps disconnecting if in more than 20 channels
10 participants