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

Inaccurate HTTP Response Headers #182

Closed
jagrosh opened this issue Nov 22, 2016 · 11 comments
Closed

Inaccurate HTTP Response Headers #182

jagrosh opened this issue Nov 22, 2016 · 11 comments

Comments

@jagrosh
Copy link
Contributor

jagrosh commented Nov 22, 2016

The headers responses for Creating a Message Reaction are inconsistent with the 429 Retry-After value. The X-RateLimit-Reset value is only accurate down to one second, and the X-RateLimit-Limit for creating reactions is 1. After creating a reaction, the provided header in the response lists the X-RateLimit-Reset as exactly 1 second after the provided Date. This would imply that the ratelimit for creating reactions is 1 per 1 second. However, by attempting to create two reactions very close in time (ignoring the headers), a Retry-After value is returned that puts the true ratelimit near 1 reaction per 250 milliseconds.

Any library that utilizes the headers to throttle requests (as they should according to #108) will be providing users with inaccurate timings for the Create Message Reaction endpoint, as well as any other endpoints that have these inconsistencies. This is especially noticeable for reactions, as many bot developers use them for "buttons", polls, and choices, where having the clickable options available as soon as possible is important for visual appeal and functionality.

@jhgg
Copy link
Member

jhgg commented Nov 22, 2016

We don't intend to fix this issue - sub-second rate limits are rare (only this endpoint). It's better to just rate limit it to 1/1 like the rate limit advertises, than to go any faster than that (which is possible - but not what the rate limit headers say).

@jhgg jhgg closed this as completed Nov 22, 2016
@khazhyk
Copy link
Contributor

khazhyk commented Nov 22, 2016

1/1 is a rather low allowance, so pretending the rate limit is that is kind of bad.

Since the true allowance is 4x higher, this really seems like a bug that should be fixed, rather than pretending that the rate limit is much lower. If the point of the headers is to accurately portray the true rate limits, if they don't actually show the true rate limits people are just going to go back to ignoring them, since they don't provide useful information.

(It's also a pretty poor user experience for api users that the rate limit headers are inaccurate)

@jhgg
Copy link
Member

jhgg commented Nov 22, 2016

If you'd like, we can make the rate limit 1/1 for bots so that it becomes accurate. We're not going to change the API response headers to have a decimal - as that will break other things - and already baked in implementations that expect a whole number.

@khazhyk
Copy link
Contributor

khazhyk commented Nov 22, 2016

Why not just make the rate limit 4/1 ?

@Rapptz
Copy link
Contributor

Rapptz commented Nov 22, 2016

If the rate limit is bad can we at least have batch-add reactions to make up for it? Using reactions for polls or UX is really hampered by a low rate limit of 1/1.

@DV8FromTheWorld
Copy link
Member

DV8FromTheWorld commented Nov 22, 2016

@khazhyk makes a valid point. Why not make the rate limit 4/1? You get identical functionality for all rate limit systems across the board. It seems only logical that all rate limits would conform to the same time basis of per second in relation to what the headers state.

Disregarding the above statement about 1/1s being a low allowance, the followup statement about perceived allowance vs true allowance providing a bad experience is very real. For those of us who provide libraries that abide by the new header rate limit system this is like a slap in the face. This disparity between actual limit and perceived limit give insentive to subvert the header system and return to the days of relying solely on the retry after so that libraries can ensure that they are providing the most accurate timings on limitation.

It would be incredibly nice to see this rate limit not changed necessarily, but shifted such that its base timing is not in the milliseconds, but shifted such that the base is in the seconds.

1/250ms -> 4/1s

@SinisterRectus
Copy link
Contributor

4/1 is not the same as 1/0.25. I like the batch reaction add suggestion.

@MinnDevelopment
Copy link
Contributor

@SinisterRectus 4/1 is a step in the right direction. Jake already pointed out that the headers will not be changed to use milliseconds and thus the 4/1 would be the most reasonable change to make in this case.

@jhgg
Copy link
Member

jhgg commented Nov 22, 2016

There is a technical reason as to why it's 1/250ms, and not 4/1. We are not changing it. Bulk adding reactions to use them as buttons is not a use-case of the API we're wanting to support/encourage.

There are talks internally of adding an API to add buttons - but we don't have any official plans yet.

For those curious, the 1/250ms rate limit means that only 1 request can happen concurrently every 250ms, whereas the 4/1 rate limit would mean that 4 requests could happen concurrently every 1 second. This can cause some glitches with update ordering to the client. Hence why it's done that way.

@Cyan101
Copy link

Cyan101 commented Nov 23, 2016

What about letting messages ship reactions already on them? you guys talked about using it for polls but if takes a while to load all the reactions if there is say 5 options

@Vampire
Copy link

Vampire commented Nov 6, 2017

We're not going to change the API response headers to have a decimal - as that will break other things - and already baked in implementations that expect a whole number.

How about adding a second header that carries milliseconds instead of seconds and deprecate the old one. As long as the old one is still sent, the old implementations work like before and new / fixed implementations could use the new header.

Bastian added a commit to Javacord/Javacord that referenced this issue Nov 8, 2017
aquarial added a commit to discord-haskell/discord-haskell that referenced this issue Apr 20, 2019
why the headers are wrong: discord/discord-api-docs#182
why I chose to hardcode it: #16
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

No branches or pull requests

9 participants