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.

Sign up for GitHub

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 limits #464

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

Rate limits #464

Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8ebd018
First addition of rate-limit stuff. Untested.
Metalhead33 Oct 22, 2024
5610674
Fixed one typoo.
Metalhead33 Oct 22, 2024
2648337
Extracted the header extraction into a separate function, as suggeste…
Metalhead33 Oct 22, 2024
496f220
Changed minimum limit of delay from 30 to 65: at least one 16th of a …
Metalhead33 Oct 22, 2024
af3a8e8
Added default values, just in case.
Metalhead33 Nov 28, 2024
e70e799
Slowed down even further, just in case.
Metalhead33 Nov 28, 2024
3b2293d
Update libdiscord.c
Metalhead33 Nov 28, 2024
0e451b1
At this point, I think this might be necessary to prevent tempbans. N…
Metalhead33 Dec 20, 2024
31262aa
Merge branch 'EionRobb:master' into RateLimits
Metalhead33 Dec 20, 2024
11e8940
Conflict resolution.
Metalhead33 Dec 20, 2024
1ea7cc7
Typo fix.
Metalhead33 Dec 20, 2024
f09fea2
Added a Jitter.
Metalhead33 Dec 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion libdiscord.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,20 @@ static GRegex *natural_mention_regex = NULL;
static GRegex *discord_mention_regex = NULL;
static GRegex *discord_spaced_mention_regex = NULL;

/*
xRateAllowedPerSecond = (int)( (double)xrateRemainign / (double)xrateResetAfter );
xRateAllowedRemaining = xRateAllowedPerSecond;
xRateDelayPerRequest = (int)((1.0 / (double)xRateAllowedPerSecond) * 1000.0);
*/

static int xrateLimit=40;
static int xrateRemainign=10;
static double xrateReset=55;
static double xrateResetAfter=1;
static int xRateAllowedPerSecond=40;
static int xRateDelayPerRequest=100;
static int xRateAllowedRemaining=10;

typedef enum {
OP_DISPATCH = 0,
OP_HEARTBEAT = 1,
Expand Down Expand Up @@ -1449,6 +1463,35 @@ discord_cookies_to_string(DiscordAccount *ya)
}

static void discord_fetch_url_with_method_delay(DiscordAccount *da, const gchar *method, const gchar *url, const gchar *postdata, DiscordProxyCallbackFunc callback, gpointer user_data, guint delay);
static void UpdateRateLimits(const gchar *xrateLimitS, const gchar *xrateRemainingS, const gchar *xRateReset, const gchar *xRateResetAfter)
{
if(xrateLimitS) {
xrateLimit=atoi(xrateLimitS);
purple_debug_info("discord", "X-RateLimit-Limit: %s\n", xrateLimitS);
}
if(xrateRemainingS) {
xrateRemainign=atoi(xrateRemainingS);
purple_debug_info("discord", "X-RateLimit-Remaining: %s\n", xrateRemainingS);
}
if(xRateReset) {
xrateReset=atof(xRateReset);
purple_debug_info("discord", "X-RateLimit-Reset: %s\n", xRateReset);
}
if(xRateResetAfter) {
xrateResetAfter=atof(xRateResetAfter);
purple_debug_info("discord", "X-RateLimit-Reset-After: %s\n", xRateResetAfter);
}
xRateAllowedPerSecond = (int)( (double)xrateRemainign / (double)xrateResetAfter );
xRateAllowedRemaining = xRateAllowedPerSecond;
xRateDelayPerRequest = (int)((1.0 / (double)xRateAllowedPerSecond) * 1000.0);
}
Metalhead33 marked this conversation as resolved.
Show resolved Hide resolved
Metalhead33 marked this conversation as resolved.
Show resolved Hide resolved
static void parse_rate_limit_headers(PurpleHttpResponse *response) {
const gchar *xrateLimitS = purple_http_response_get_header(response,"X-RateLimit-Limit");
const gchar *xrateRemainingS = purple_http_response_get_header(response,"X-RateLimit-Remaining");
const gchar *xRateReset = purple_http_response_get_header(response,"X-RateLimit-Reset");
const gchar *xRateResetAfter = purple_http_response_get_header(response,"X-RateLimit-Reset-After");
UpdateRateLimits(xrateLimitS, xrateRemainingS, xRateReset, xRateResetAfter);
}
Comment on lines +1507 to +1513
Copy link

@coderabbitai coderabbitai bot Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding error handling for missing headers

The parse_rate_limit_headers function should handle cases where headers are missing or invalid.

 static void parse_rate_limit_headers(PurpleHttpResponse *response) {
+    if (!response) {
+        purple_debug_warning("discord", "No response to parse rate limit headers from\n");
+        return;
+    }
     const gchar *xrateLimitS = purple_http_response_get_header(response,"X-RateLimit-Limit");
     const gchar *xrateRemainingS = purple_http_response_get_header(response,"X-RateLimit-Remaining");  
     const gchar *xRateReset = purple_http_response_get_header(response,"X-RateLimit-Reset");
     const gchar *xRateResetAfter = purple_http_response_get_header(response,"X-RateLimit-Reset-After");
+    
+    if (!xrateLimitS && !xrateRemainingS && !xRateReset && !xRateResetAfter) {
+        purple_debug_info("discord", "No rate limit headers found in response\n");
+        return;
+    }
+
     UpdateRateLimits(xrateLimitS, xrateRemainingS, xRateReset, xRateResetAfter);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
static void parse_rate_limit_headers(PurpleHttpResponse *response) {
const gchar *xrateLimitS = purple_http_response_get_header(response,"X-RateLimit-Limit");
const gchar *xrateRemainingS = purple_http_response_get_header(response,"X-RateLimit-Remaining");
const gchar *xRateReset = purple_http_response_get_header(response,"X-RateLimit-Reset");
const gchar *xRateResetAfter = purple_http_response_get_header(response,"X-RateLimit-Reset-After");
UpdateRateLimits(xrateLimitS, xrateRemainingS, xRateReset, xRateResetAfter);
}
static void parse_rate_limit_headers(PurpleHttpResponse *response) {
if (!response) {
purple_debug_warning("discord", "No response to parse rate limit headers from\n");
return;
}
const gchar *xrateLimitS = purple_http_response_get_header(response,"X-RateLimit-Limit");
const gchar *xrateRemainingS = purple_http_response_get_header(response,"X-RateLimit-Remaining");
const gchar *xRateReset = purple_http_response_get_header(response,"X-RateLimit-Reset");
const gchar *xRateResetAfter = purple_http_response_get_header(response,"X-RateLimit-Reset-After");
if (!xrateLimitS && !xrateRemainingS && !xRateReset && !xRateResetAfter) {
purple_debug_info("discord", "No rate limit headers found in response\n");
return;
}
UpdateRateLimits(xrateLimitS, xrateRemainingS, xRateReset, xRateResetAfter);
}

Copy link
Author

Choose a reason for hiding this comment

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

It looks like the whole pull request still does not completely fix the IP temporary bans, only makes them rarer. Very unfortunate.

Copy link

Choose a reason for hiding this comment

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

@Metalhead33, it is indeed unfortunate that the changes only reduce the frequency of IP temporary bans rather than eliminating them entirely. This could indicate that there are additional factors or nuances in Discord's rate-limiting mechanism that are not yet fully accounted for in the implementation.

If you'd like, I can assist in further debugging or refining the logic to better align with Discord's rate-limiting policies. For instance, we could add more detailed logging to capture edge cases or explore alternative strategies for handling rate limits. Let me know how you'd like to proceed!

Copy link
Author

Choose a reason for hiding this comment

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

@coderabbitai By all means, analyze the code any alternative locations where the rate limit might be invokved.

Copy link

Choose a reason for hiding this comment

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

@Metalhead33, based on the search results, the following locations in the codebase involve rate limit handling or related headers:

  1. UpdateRateLimits function:

    • This function updates rate limit variables (xrateLimit, xrateRemaining, xrateReset, xrateResetAfter) based on the headers received in API responses.
    • It also calculates derived values like xRateAllowedPerSecond, xRateAllowedRemaining, and xRateDelayPerRequest.

  2. parse_rate_limit_headers function:

    • This function extracts rate limit headers (X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-RateLimit-Reset-After) from the HTTP response and passes them to UpdateRateLimits.

  3. discord_response_callback function:

    • This function calls parse_rate_limit_headers to process rate limit headers for each HTTP response.

These are the primary locations where rate limit handling is implemented. If you suspect there are additional areas where rate limits might be invoked indirectly or overlooked, we could expand the search to include related terms like "rate", "limit", or "delay" across the codebase. Let me know how you'd like to proceed!

🔗 Analysis chain
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all occurrences of rate limit handling or related headers in the codebase.
rg --context 5 "X-RateLimit" --type-add 'c:*.c' --type c

Length of output: 2326


static void
discord_response_callback(PurpleHttpConnection *http_conn,
Expand All @@ -1457,6 +1500,7 @@ discord_response_callback(PurpleHttpConnection *http_conn,
gsize len;
const gchar *url_text = purple_http_response_get_data(response, &len);
const gchar *error_message = purple_http_response_get_error(response);
parse_rate_limit_headers(response);
const gchar *body;
Comment on lines +1522 to 1523
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve rate limit error handling and logging

The rate limit error handling could be more robust with validation and better logging.

     parse_rate_limit_headers(response);
+    
+    if (response_code == 429) {
+        const gchar *retry_after_s = purple_http_response_get_header(response, "Retry-After");
+        gdouble retry_after = 5.0;  // Default fallback
+        
+        if (retry_after_s) {
+            retry_after = g_ascii_strtod(retry_after_s, NULL);
+            if (retry_after <= 0) {
+                purple_debug_warning("discord", "Invalid Retry-After value: %s\n", retry_after_s);
+                retry_after = 5.0;
+            }
+        }
+        
+        purple_debug_info("discord", "Rate limited, retrying after %.2f seconds\n", retry_after);
+        // ... rest of retry logic
+    }

Committable suggestion skipped: line range outside the PR's diff.

gsize body_len;
DiscordProxyConnection *conn = user_data;
Expand Down Expand Up @@ -1635,7 +1679,7 @@ discord_fetch_url_with_method_delay(DiscordAccount *da, const gchar *method, con
request->url = g_strdup(url);
request->contents = postdata ? g_strdup(postdata) : NULL;

purple_timeout_add(delay + 30, discord_fetch_url_with_method_delay_cb, request);
purple_timeout_add(delay + MAX(65,xRateDelayPerRequest), discord_fetch_url_with_method_delay_cb, request);
}

static void
Expand Down