Summary

Use the leaky bucket algorithm to rate limit irc messages.

Review Request #524 — Created Feb. 24, 2021 and submitted April 23, 2022, 10:06 a.m.

Information

Owner

grim

Repository

pidgin/pidgin

Branch

release-2.x.y

Bugs

PIDGIN-11089

Depends On

Commit

e053cb52be57

Reviewers

Groups

pidgin

People

Description

The default values were suggested by an operator of libera.

We don't rate limit the login process, nor parts and quits. However, if you
paste a bunch of text and then part a channel, you will be spammed with a
bunch of "no such nick/channel" error dialogs. I tried to work around this,
but the alternative just makes irc unresponsive until all the pasted messages
are sent. That said, other messages are still delayed while these errors
dialogs are slowly popping up.

Testing Done

Lots

Issues

Description	From	Last Updated
we might need a flag to check if a partial send is in progress before pre-empting that send.	grim	April 21, 2022, 7:54 a.m.
elb is concerned this may lead to an infinite loop an on error.	grim	March 3, 2021, 5:42 a.m.
requeues need to be resized and available messages should NOT be decremented.	grim	March 3, 2021, 5:37 a.m.
we need to make sure this function get called in all close paths.	grim	April 22, 2022, 1:35 a.m.
Can you use gint64 and g_get_monotonic_time?	QuLogic	March 3, 2021, 5:38 a.m.
I mean, the rest of the comment means it's not really dangerous, no?	QuLogic	March 3, 2021, 5:52 a.m.
If the buffer is full and very very slow, this will cause an (almost) infinite loop. I don't know if …	QuLogic	April 22, 2022, 3:52 a.m.
so?	QuLogic	March 3, 2021, 5:51 a.m.
ourselves one?	QuLogic	March 3, 2021, 5:52 a.m.
Every caller to this function has to do strlen, and in the partial send case, it's even ignored. So it …	QuLogic	April 22, 2022, 3:52 a.m.
No message	QuLogic	April 22, 2022, 3:53 a.m.
Should set irc->send_handler = 0 before removing.	QuLogic	April 22, 2022, 3:53 a.m.
"to move past the characters"	QuLogic	April 22, 2022, 3:53 a.m.
s/on/one/?	QuLogic	April 22, 2022, 3:53 a.m.
So this means that messages cannot be sent with a granularity of less than 1 second now?	QuLogic	April 22, 2022, 4:33 a.m.

Bugs:: PIDGIN-11089

Change Summary:

s/commands/messages/ in the account options.

Commit:

bc62765e98a0

89988bab736d

Diff:

Revision 2 (+122 -86)

Show changes

	libpurple/protocols/irc/cmds.c
	libpurple/protocols/irc/irc.h
	libpurple/protocols/irc/irc.c
	libpurple/protocols/irc/msgs.c

libpurple/protocols/irc/irc.c (Diff revision 2)

The issue has been resolved. Show all issues

we might need a flag to check if a partial send is in progress before pre-empting that send.

grim Feb. 25, 2021, 3:20 a.m.

The more I think about this, the more I think this isn't possible in the current version of the code.

libpurple/protocols/irc/irc.c (Diff revision 2)

The issue has been resolved. Show all issues

elb is concerned this may lead to an infinite loop an on error.

grim March 3, 2021, 5:43 a.m.

We should be fine because the condition above is what handles the errors aside from EAGAIN.

libpurple/protocols/irc/irc.c (Diff revision 2)
The issue has been resolved. Show all issues
```
requeues need to be resized and available messages should NOT be decremented.
```

libpurple/protocols/irc/irc.c (Diff revision 2)

The issue has been resolved. Show all issues

we need to make sure this function get called in all close paths.

grim April 15, 2022, 6:18 p.m.

After some digging, purple_connection_error does eventually call the close method in the prpl, but it does so in a timeout callback after 0 seconds for some reason. So in theory, errors could start stacking here. Not really sure how to address that, not sure we need to? Thoughts?

Change Summary:

fix some issues that were identified with partial sends.

Commit:

89988bab736d

89c32aae8e75

Diff:

Revision 3 (+147 -87)

Show changes

	libpurple/protocols/irc/cmds.c
	libpurple/protocols/irc/irc.h
	libpurple/protocols/irc/irc.c
	libpurple/protocols/irc/msgs.c

Change Summary:

apparently I didn't compile...

Commit:

89c32aae8e75

34bb3a6edcff

Diff:

Revision 4 (+147 -87)

Show changes

	libpurple/protocols/irc/cmds.c
	libpurple/protocols/irc/irc.h
	libpurple/protocols/irc/irc.c
	libpurple/protocols/irc/msgs.c

libpurple/protocols/irc/irc.h (Diff revision 4)

The issue has been dropped. Show all issues

Can you use gint64 and g_get_monotonic_time?

grim Feb. 26, 2021, 6:22 a.m.

I can not. Purple 2 depends on glib 2.16.0 and that was added in 2.28.0.

libpurple/protocols/irc/irc.c (Diff revision 4)
The issue has been resolved. Show all issues
```
I mean, the rest of the comment means it's not really dangerous, no?
```

libpurple/protocols/irc/irc.c (Diff revision 4)

The issue has been resolved. Show all issues

If the buffer is full and very very slow, this will cause an (almost) infinite loop. I don't know if it's better to break early, or decrement available anyway here. Or just ignore that case.

grim March 3, 2021, 5:43 a.m.
```
I vote for ignore?
```

grim April 15, 2022, 5:53 p.m.

I have an update coming for the priority stuff that will at least let the user disconnect manually once it's made it through the current partial which would break the slow inifite loop.
But I'm also realizing that the server ping could/will time out which will break the loop too. This is because the priority update queues the priority messages as the next one in the queue if we're currently handling a partial.

libpurple/protocols/irc/irc.c (Diff revision 4)
The issue has been resolved. Show all issues
```
so?
```
libpurple/protocols/irc/irc.c (Diff revision 4)
The issue has been resolved. Show all issues
```
ourselves

one?
```

Change Summary:

Adressed typos and stuff.

Commit:

34bb3a6edcff

5bbd0f3a7d4f

Diff:

Revision 5 (+146 -87)

Show changes

	libpurple/protocols/irc/cmds.c
	libpurple/protocols/irc/irc.h
	libpurple/protocols/irc/irc.c
	libpurple/protocols/irc/msgs.c

Change Summary:

rebased

Commit:

5bbd0f3a7d4f

64cb1df9b8e9

Diff:

Revision 6 (+146 -87)

Show changes

	libpurple/protocols/irc/cmds.c
	libpurple/protocols/irc/irc.h
	libpurple/protocols/irc/irc.c
	libpurple/protocols/irc/msgs.c

Description:: ~
The default values were suggested by an operator of freenode.
~
The default values were suggested by an operator of libera.

We don't rate limit the login process, nor parts and quits. However, if you
paste a bunch of text and then part a channel, you will be spammed with a
bunch of "no such nick/channel" error dialogs. I tried to work around this,
but the alternative just makes irc unresponsive until all the pasted messages
are sent. That said, other messages are still delayed while these errors
dialogs are slowly popping up.

Change Summary:

if we're currently in a partial send, queue prioity commands as the next command.

Commit:

64cb1df9b8e9

a0a9701fa441

Diff:

Revision 7 (+155 -87)

Show changes

	libpurple/protocols/irc/cmds.c
	libpurple/protocols/irc/irc.h
	libpurple/protocols/irc/irc.c
	libpurple/protocols/irc/msgs.c

libpurple/protocols/irc/irc.c (Diff revision 7)

The issue has been resolved. Show all issues

Every caller to this function has to do strlen, and in the partial send case, it's even ignored. So it would be better to have the strlen in this function's do_send call, and drop the len argument.

libpurple/protocols/irc/irc.c (Diff revision 7)
The issue has been resolved. Show all issues
```
No message
```
libpurple/protocols/irc/irc.c (Diff revision 7)
The issue has been resolved. Show all issues
```
Should set irc->send_handler = 0 before removing.
```
libpurple/protocols/irc/irc.c (Diff revision 7)
The issue has been resolved. Show all issues
```
"to move past the characters"
```
libpurple/protocols/irc/irc.c (Diff revision 7)
The issue has been resolved. Show all issues
```
s/on/one/?
```

libpurple/protocols/irc/irc.c (Diff revision 7)

The issue has been dropped. Show all issues

So this means that messages cannot be sent with a granularity of less than 1 second now?

grim April 22, 2022, 1:42 a.m.

it should always be less than or equal to one second as g_timeout_add_seconds happens at the second mark. The only time it'll go over one second is when we're rate limited.

We could lower this value, but I have no idea what a good value would be off the top of my head. I know historically we moved to using g_timeout_add_seconds everywhere because we were destroying laptop batteries because we kept waking up the process for timeouts.

Change Summary:

Addressed issues.

Commit:

a0a9701fa441

e053cb52be57

Diff:

Revision 8 (+157 -87)

Show changes

	libpurple/protocols/irc/cmds.c
	libpurple/protocols/irc/irc.h
	libpurple/protocols/irc/irc.c
	libpurple/protocols/irc/msgs.c

Ship it!

```
Ship It!
```

Status:: Completed