Ratelimits #273

jogramming · 2016-10-19T15:03:40Z

Handles the new ratelimit headers

X-RateLimit-Remaining
X-RateLimit-Reset
X-RateLimit-Global

I stuck it in an internal package, hope that's okay.
This is probably not the best way to do the removal of minor url variables, the alternative is to redo the whole endpoint system.

As for the reliability of this, after sending around 1k requests non-stop i didn't encounter a single 429.

I pad the reset time with 1 second, because if i don't it will still occasionally run into 429's of the size 1ms - 50ms (yeah, i got ratelimited for 1 ms a lot...), padding with anything less than 1 second would still give those tiny ratelimits sometimes.

Handles the new ratelimit headers - X-RateLimit-Remaining - X-RateLimit-Reset - X-RateLimit-Global

iopred

Sorry about the big review, I really do appreciate this though, this looks like a huge improvement.

iopred · 2016-10-19T15:11:48Z

internal/ratelimit/ratelimit.go

+
+	bucketKey := ParseURL(path)
+
+	r.Lock()


Use sync.RWMutex and call r.RLock/r.RUnlock here

Well getBucket will also modify the map if the bucket isn't there already

Ah, then the locking should be in there so the next time someone does r.getBucket() they don't need to replicate the correct locking scheme.

iopred · 2016-10-19T15:14:44Z

structs.go

@@ -17,6 +17,7 @@ import (
 	"sync"
 	"time"

+	"github.com/bwmarrin/discordgo/internal/ratelimit"


Just move ratelimit into the dgo directory, not sure we need to have an internal package.

iopred · 2016-10-19T15:26:47Z

internal/ratelimit/ratelimit.go

+// Ratelimiter holds all ratelimit buckets
+type RateLimiter struct {
+	sync.Mutex
+	buckets         map[string]*Bucket


Might be better to have:
global *Bucket

instead of passing the rate limiter into a bucket, circular references feel wrong.

You mean each bucket having a reference to a global bucket instead of the ratelimiter?

Yeah exactly.

iopred · 2016-10-19T15:27:21Z

internal/ratelimit/ratelimit.go

+
+// ParseURL parses the url, removing everything not relevant to identifying a bucket.
+// such as minor variables
+func ParseURL(url string) string {


Is it possible to pass the endpoint into the ratelimiter in DGO, it seems unfortunate to construct a URL, and then re-parse it with a regex to get the endpoint out.

You mean having request take another argument, the bucket, and have each request method be in charge of the locking of buckets?

It's a much bigger change, but I would add:
RequestWithBucketId()

Doing that instead of changing Request() would be good for backwards compatibilities sake.

That would allow you to modify the private request() to have a nilable bucket ID.

Was this how you meant?

iopred · 2016-10-19T15:27:50Z

internal/ratelimit/ratelimit_test.go

@@ -0,0 +1,131 @@
+package ratelimit


Thank you! 👍

iopred · 2016-10-19T15:28:58Z

internal/ratelimit/ratelimit.go

+	// If we ran out of calls and the reset time is still ahead of us
+	// then we need to take it easy and relax a little
+	for b.remaining < 1 && b.reset.After(time.Now()) {
+		time.Sleep(b.reset.Sub(time.Now()))


I would like to figure out a way to rate limit without sleeping, however I realise that this is still an improvement, so let's keep that in mind for the future.

Hmm yeah making a ratelimiter based on channels instead sounds like a fun challenge :D

iopred · 2016-10-19T15:30:45Z

internal/ratelimit/ratelimit.go

+
+	// If we ran out of calls and the reset time is still ahead of us
+	// then we need to take it easy and relax a little
+	for b.remaining < 1 && b.reset.After(time.Now()) {


Does this need to be a for loop? It looks like you're sleeping enough to be fine.

it doesn't need to be a for loop, no

iopred · 2016-10-19T15:31:03Z

internal/ratelimit/ratelimit.go

+	}
+
+	// Lock and unlock to check for global ratelimites after sleeping
+	r.Lock()


Why is this dance required?

After sleeping, another request might have triggered a global ratelimit

Hmm, I worry about using mutex's for synchronization. Is there not a way to detect and sleep for max(bucketTime, globalTime)?

Well the problem is that while there may not be any global ratelimit when it started sleeping, there may be one after sleeping

iopred · 2016-10-19T15:47:37Z

internal/ratelimit/ratelimit.go

+		}
+
+		// Add a second to account for time desync and such
+		b.reset = time.Unix(unix, 0).Add(time.Second)


Also, can you play with this value.. You mentioned a few milliseconds don't work, what about 100*time.Millisecond, 200? 500?

i've tried all up to 700, and 1s was the only thing that worked 100%. it's strange cause the retry-after duration is always below 50ms

I'm currently doing some tests with 800 and then trying 900

Some data i've collected about this:

with no added time it's around every 15~ request
with 250ms: every 100~ request
with 500ms: every 500~ requests
with 800ms: every 1000~ requests
with 900ms: every 2000~ requests

haven't encountered one with 1s yet, gonna let the test run for a while.

these are just rough averages done in my head, not very accurate.

My thoughts on causes for this:

system time desync between me and discord, my time is synced up using arch's ntp servers though.

unix time rounding on discords end (if they store the time in a higher precision than seconds, but round the time down to seconds when sending it to us)

I'll do some more testing on a vps.

iopred · 2016-10-19T19:14:00Z

That's sufficient, 1s seems reasonable.

On Wed, Oct 19, 2016 at 12:09 PM, jonas747 notifications@github.com wrote:

@jonas747 commented on this pull request.

In internal/ratelimit/ratelimit.go
#273:
// Prefer retryafter because it's more accurate with time sync and whatnot

if retryAfter != "" {
parsedAfter, err := strconv.ParseInt(retryAfter, 10, 64)
if err != nil {
    return err
}
b.reset = time.Now().Add(time.Duration(parsedAfter) \* time.Millisecond)
} else if reset != "" {
unix, err := strconv.ParseInt(reset, 10, 64)
if err != nil {
    return err
}
// Add a second to account for time desync and such
b.reset = time.Unix(unix, 0).Add(time.Second)
Some data i've collected about this:

with no added time it's around every 15~ request
with 250ms: every 100~ request
with 500ms: every 500~ requests
with 800ms: every 1000~ requests
with 900ms: every 2000~ requests

haven't encountered one with 1s yet, gonna let the test run for a while.

these are just rough averages done in my head, not very accurate.

My thoughts on causes for this:

system time desync between me and discord, my time is synced up
using arch's ntp servers though.

unix time rounding on discords end (if they store the time in a
higher precision than seconds, but round the time down to seconds when
sending it to us)

I'll do some more testing on a vps.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#273, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdVggGa6_0LCFGwkELQXdhyqW71KM5dks5q1mrogaJpZM4KbFaW
.

Now each request function will need to specify the bucket id if the endpoint contains minor variables

iopred · 2016-10-19T21:56:57Z

restapi.go

@@ -269,7 +251,7 @@ func (s *Session) Logout() (err error) {
 // userID    : A user ID or "@me" which is a shortcut of current user ID
 func (s *Session) User(userID string) (st *User, err error) {

-	body, err := s.Request("GET", EndpointUser(userID), nil)
+	body, err := s.RequestWithBucketID("GET", EndpointUser(userID), nil, EndpointUser(""))


I would use EndpointUsers instead. same with EndpointGuilds etc. Then you don't need to split the strings or do any modifications.

I updated some of them to this, did you mean to avoid the strings.SplitN call when using Request as well?

jogramming · 2016-10-24T07:13:35Z

Latest commit makes it use the date header instead of requiring synced time, also lowered the extra time padding down to 250ms with that

iopred · 2016-10-25T18:17:35Z

Yeah exactly, just pass in the bucket ID directly, I suggest using
EndpointUsers, EndpointGuilds etc. as the ID's as they are already strings.

On Thu, Oct 20, 2016 at 12:39 PM, jonas747 notifications@github.com wrote:

@jonas747 commented on this pull request.

In restapi.go #273:

@@ -269,7 +251,7 @@ func (s *Session) Logout() (err error) {
// userID : A user ID or "@me" which is a shortcut of current user ID
func (s *Session) User(userID string) (st *User, err error) {

body, err := s.Request("GET", EndpointUser(userID), nil)

body, err := s.RequestWithBucketID("GET", EndpointUser(userID), nil, EndpointUser(""))

I updated some of them to this, did you mean to avoid the strings.SplitN
call when using Request as well?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#273, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdVguQJE71OVwLGtE5EBVzztg_sZjcTks5q18NqgaJpZM4KbFaW
.

jogramming · 2016-10-26T06:25:35Z

so just to be 100% sure, replace stuff like

body, err := s.Request("GET", EndpointGuild(guildID), nil)

with

body, err := s.RequestWithBucketID("GET", EndpointGuild(guildID), nil, EndpointGuild(guildID))

?

iopred · 2016-10-26T15:46:57Z

body, err := s.RequestWithBucketID("GET", EndpointGuild(guildID), nil,
EndpointGuilds)

On Tue, Oct 25, 2016 at 11:25 PM, jonas747 notifications@github.com wrote:

so just to be 100% sure, replace stuff like

body, err := s.Request("GET", EndpointGuild(guildID), nil)

with

body, err := s.RequestWithBucketID("GET", EndpointGuild(guildID), nil, EndpointGuild(guildID))

?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#273 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdVgvKvXWA2XXJOjBimMJISusFxgcSPks5q3vJggaJpZM4KbFaW
.

jogramming · 2016-10-26T16:51:02Z

Then requests for different guilds would be rate limited with the same bucket, guildID is a major variable so they should be rate limited on their own according to the docs

iopred · 2016-10-26T16:55:53Z

I see, I'm trying to remove bucketID = strings.SplitN(urlStr, "?", 2)[0] from RequestWithBucketID, will that be sufficient if you pass EndpointGuild(guildID) in?

If so, Looks good to me :) 👍

jogramming · 2016-10-26T17:41:01Z

the strings.SpltiN is in Request(..) for backwards compatibility, since that function dosent take a separate bucket id.

you mean like

s.RequestWithBucketID("GET", EndpointGuild(guildID), nil, EndpointGuild(guildID))

? if so yeah, that wont call strings.SplitN

iopred · 2016-10-26T18:03:12Z

Yeah I don't think we can avoid it in Request.

On Oct 26, 2016 10:41 AM, "jonas747" notifications@github.com wrote:

the strings.SpltiN is in Request(..) for backwards compatibility, since
that function dosent take a separate bucket id.

you mean like

s.RequestWithBucketID("GET", EndpointGuild(guildID), nil,
EndpointGuild(guildID))

? if so yeah, that wont call strings.SplitN

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#273 (comment),
or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABdVgrhjywu2mknj50JdVEbUKL4WYiHGks5q35CtgaJpZM4KbFaW
.

jogramming · 2016-10-27T05:00:57Z

dunso

iopred

Sweet dude

iopred · 2016-10-27T05:15:24Z

ratelimit.go

+type Bucket struct {
+	Key string
+
+	mu        sync.Mutex


Do:

type Bucket struct {
sync.Mutex
}

Then you can simply call:

bucket.Lock()

iopred · 2016-10-27T05:15:48Z

ratelimit.go

+	}
+
+	// Check for global ratelimits
+	r.global.mu.Lock()


With the comment below this can be changed to:

r.global.Lock()

iopred · 2016-10-27T05:16:37Z

ratelimit.go

+// and locks up the whole thing in case if there's a global ratelimit.
+func (b *Bucket) Release(headers http.Header) error {
+
+	defer b.mu.Unlock()


b.Unlock() with the comment above

iopred · 2016-10-27T05:16:42Z

ratelimit.go

+			// where n is the amount of requests that were going on
+			sleepTo := time.Now().Add(time.Duration(parsedAfter) * time.Millisecond)
+
+			b.global.mu.Lock()


iopred · 2016-10-27T05:16:49Z

ratelimit.go

+				time.Sleep(sleepDuration)
+			}
+
+			b.global.mu.Unlock()


jogramming · 2016-10-27T07:36:01Z

Also done

iopred · 2016-11-04T05:24:13Z

Sorry dude, one last request, could you add Buckets for Reactions/Webhooks <3

jogramming · 2016-11-04T08:41:13Z

finito

iopred · 2016-11-04T15:07:25Z

Phew! Thanks so much dude. This is awesome! <3

CarsonHoffman · 2020-06-04T18:23:00Z

@Distortions81 where exactly is the issue in this change with respect to that?

* Added ratelimiter Handles the new ratelimit headers - X-RateLimit-Remaining - X-RateLimit-Reset - X-RateLimit-Global * Pad only reset time with a second * Moved ratelimiter out of internal package * Change for loop, move global ratelimit check inside sleep check * Moved ratelimiter locking to getBucket * Added global bucket * Changed how bucket id's are done Now each request function will need to specify the bucket id if the endpoint contains minor variables * Allow empty bucketID in request * Remove some uneeded Endpoint* function calls * Added test for global ratelimits * Fixed a silly little mistake causing incorrect ratelimits * Update test comments, Fixed treating a endpoint as 2 in ratelimiting * Use date header from discord instead of relying on sys time sync * Update all REST functions to use RequestWithBucketID * Embed mutex into bucket * Added webhook and reaction buckets

jogramming added 2 commits October 19, 2016 06:17

Added ratelimiter

962867d

Handles the new ratelimit headers - X-RateLimit-Remaining - X-RateLimit-Reset - X-RateLimit-Global

Pad only reset time with a second

b35acdd

iopred reviewed Oct 19, 2016

View reviewed changes

jogramming added 3 commits October 19, 2016 17:50

Moved ratelimiter out of internal package

48d13bf

Change for loop, move global ratelimit check inside sleep check

0e93acf

Moved ratelimiter locking to getBucket

77db069

jogramming added 2 commits October 19, 2016 22:50

Added global bucket

4206e18

Changed how bucket id's are done

0f17b09

Now each request function will need to specify the bucket id if the endpoint contains minor variables

iopred reviewed Oct 19, 2016

View reviewed changes

jogramming added 6 commits October 20, 2016 19:20

Allow empty bucketID in request

45f9a0e

Remove some uneeded Endpoint* function calls

a003e36

Added test for global ratelimits

e447e9e

Fixed a silly little mistake causing incorrect ratelimits

c8f34e3

Update test comments, Fixed treating a endpoint as 2 in ratelimiting

f35710a

Use date header from discord instead of relying on sys time sync

01441c4

Update all REST functions to use RequestWithBucketID

413a12e

iopred requested changes Oct 27, 2016

View reviewed changes

Embed mutex into bucket

05c0542

iopred approved these changes Nov 4, 2016

View reviewed changes

jogramming added 2 commits November 4, 2016 08:10

Merge branch 'develop' into ratelimits

e1f7534

Added webhook and reaction buckets

5db9969

iopred merged commit 56b1907 into bwmarrin:develop Nov 4, 2016

bwmarrin added this to the v0.15.0 milestone Apr 18, 2017

ashishjh-bst deleted the ratelimits branch March 21, 2022 12:48

Ratelimits #273

Ratelimits #273

Conversation

jogramming commented Oct 19, 2016

iopred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogramming Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iopred commented Oct 19, 2016

@jonas747 commented on this pull request.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogramming commented Oct 24, 2016

iopred commented Oct 25, 2016

@jonas747 commented on this pull request.

jogramming commented Oct 26, 2016

iopred commented Oct 26, 2016

jogramming commented Oct 26, 2016

iopred commented Oct 26, 2016

jogramming commented Oct 26, 2016

iopred commented Oct 26, 2016

jogramming commented Oct 27, 2016

iopred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogramming commented Oct 27, 2016

iopred commented Nov 4, 2016

jogramming commented Nov 4, 2016

iopred commented Nov 4, 2016

CarsonHoffman commented Jun 4, 2020

jogramming Oct 19, 2016 •

edited

Loading