Rate limits #464
Rate limits #464
Conversation
WalkthroughThe changes in the pull request focus on enhancing the rate limiting functionality within the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
libdiscord.c (2)
106-112: Add comments explaining the purpose of these new static variables.It would be helpful to add comments describing what these variables are used for and how they relate to rate limiting.
1459-1480: Refactor UpdateRateLimits function to improve readability.Consider breaking this function into smaller logical sections with comments to make it easier to understand. For example:
static void UpdateRateLimits(const gchar *xrateLimitS, const gchar *xrateRemainingS, const gchar *xRateReset, const gchar *xRateResetAfter) { // Update xrateLimit if(xrateLimitS) { xrateLimit = atoi(xrateLimitS); purple_debug_info("discord", "X-RateLimit-Limit: %s\n", xrateLimitS); } // Update xrateRemaining if(xrateRemainingS) { xrateRemaining = atoi(xrateRemainingS); purple_debug_info("discord", "X-RateLimit-Remaining: %s\n", xrateRemainingS); } // Update xrateReset and xrateResetAfter 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); } // Calculate derived rate limit values xRateAllowedPerSecond = (int)( (double)xrateRemaining / (double)xrateResetAfter ); xRateAllowedRemaining = xRateAllowedPerSecond; xRateDelayPerRequest = (int)((1.0 / (double)xRateAllowedPerSecond) * 1000.0); }
libdiscord.c
Outdated
| 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); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract rate limit header parsing into a separate function.
Consider moving the parsing of the rate limit headers into a separate function to keep discord_response_callback focused on its main purpose. Something like:
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);
}Then in discord_response_callback:
parse_rate_limit_headers(response);
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
libdiscord.c (1)
106-112: Consider using more descriptive variable names.The variable names like
xrateLimit,xrateRemainign, etc. are not very clear. More descriptive names would improve code readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- libdiscord.c (4 hunks)
🧰 Additional context used
🔇 Additional comments (2)
libdiscord.c (2)
1481-1487: LGTM!The new
parse_rate_limit_headersfunction looks good. It extracts the rate limit header parsing logic nicely.
1496-1496: Verify that parsing rate limit headers on every response is intended.Calling
parse_rate_limit_headers(response)on every HTTP response could have performance implications if rate limit headers are not always present. Ensure this is the intended behavior. If rate limit headers are expected only on certain responses, consider adding a condition to selectively parse them.✅ Verification successful
Let me gather more context about the rate limit header parsing implementation and usage.
Let me check how this rate limit parsing is used in the broader context of HTTP requests and responses.
Based on the gathered context, I can now provide a final response:
Parsing rate limit headers on every response is the correct implementation
The code shows that rate limit parsing is part of Discord's API response handling system, where:
- The headers are only processed if they exist (
if(xrateLimitS)check inUpdateRateLimits)- It's integrated with Discord's rate limit handling mechanism (response code 429 check)
- The implementation is lightweight, only involving header extraction and simple integer parsing
- The headers are used to maintain the global rate limit state
This is a standard practice for Discord API clients to prevent rate limit violations proactively, and the current implementation follows a fail-safe approach by checking headers on all responses while only processing them when present.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if rate limit headers are present on all responses rg -c 'X-RateLimit-' | awk '{s+=$1} END {print s}'Length of output: 52
Script:
#!/bin/bash # Check the implementation of parse_rate_limit_headers and related code rg -A 5 "parse_rate_limit_headers" # Look for Discord API rate limit related comments/documentation rg -i "rate.?limit"Length of output: 2217
Script:
#!/bin/bash # Look for the HTTP response handling context rg -B 5 -A 10 "parse_rate_limit_headers\(response\)" # Check if there's any conditional logic around rate limits rg -B 3 -A 3 "UpdateRateLimits" # Look for any Discord API documentation or comments about rate limiting rg -i "discord.*api.*rate"Length of output: 1955
|
I'm not sure this actually did anything. In the actual debug logs, I don't see any references to the X-RateLimit headers - still, at least it doesn't result in an instant temp-ban like it did before I implemented this change. Placebo effect, perhaps? |
…second to keep up with the 'Less than 10 000 requests per 10 minutes' hard limit.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- libdiscord.c (4 hunks)
🧰 Additional context used
🔇 Additional comments (4)
libdiscord.c (4)
1459-1480: Refactor rate limit update logic into separate function.Extracting the rate limit update logic into its own function is a good step towards better code organization. Well done.
1481-1487: Refactor rate limit header parsing into separate function.Extracting the header parsing logic into its own function is a good step towards better code organization and reusability. Well done.
1496-1496: Call new parse_rate_limit_headers function.Good job calling the new
parse_rate_limit_headersfunction here to update the rate limits based on the response headers.
1675-1675: Use MAX macro for better readability.Using the MAX macro here improves code readability. Good catch.
|
Thank you for getting this PR underway! Really appreciate the help. 🥳 With the rate limits, are they enforced per-account or per IP? One of the suggestions from the bot (thanks for working with that btw, I've only just started using it so wasn't sure how well it'd go) was to put the variables into the account so that they aren't global, but is that intentional? Looks good so far though. I'll have to try it out tonight. We can point other users to the dll auto builds to try out too. I'll do that when I'm back at my desktop, unless someone else jumps into it first 🙂 |
|
Hello @EionRobb - thank you for your kind coment, this is a very special moment for me, as it is the first time I truly contribute to an open-source repository :) I personally bleieve that he bot is mistaken - Discord's rate limits are enforced per-IP. I noticed that because whenever the temp-ban is triggered, it affects both my desktop PC and my phone's wi-fi (which ultimately leads to the same modem/router that my desktop PC uses), but when I switch my mobile phone to "using data", the temp-ban is gone. This suggests that it is NOT per-account, but per-IP. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
libdiscord.c (2)
1682-1682: Define constants for delay valuesThe magic number 65 should be defined as a constant for better maintainability. Also consider using more descriptive variable names.
+#define DISCORD_MIN_REQUEST_DELAY 65 // Minimum delay between requests in milliseconds + - purple_timeout_add(delay + MAX(65,xRateDelayPerRequest), discord_fetch_url_with_method_delay_cb, request); + int rate_limit_delay = MAX(DISCORD_MIN_REQUEST_DELAY, xRateDelayPerRequest); + int total_delay = delay + rate_limit_delay; + purple_timeout_add(total_delay, discord_fetch_url_with_method_delay_cb, request);
107-111: Consider restructuring rate limiting implementationThe current rate limiting implementation could benefit from several architectural improvements:
- Move rate limit state into the DiscordAccount struct for proper multi-account support
- Create a dedicated rate limiting module/structure to encapsulate the logic
- Add comprehensive error handling and logging
- Use constants for configuration values
This would make the code more maintainable and robust.
Consider implementing a rate limiter structure like:
typedef struct { int limit; int remaining; double reset; double reset_after; int allowed_per_second; int delay_per_request; int allowed_remaining; } DiscordRateLimiter; // Add to DiscordAccount struct struct _DiscordAccount { // ... existing fields ... DiscordRateLimiter *rate_limiter; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
libdiscord.c(4 hunks)
🔇 Additional comments (1)
libdiscord.c (1)
113-119:
Fix typo in variable name and consider thread safety
The variable name xrateRemainign has a typo and should be xrateRemaining. Additionally, these static variables could cause issues in multi-account scenarios since they're globally shared across all Discord accounts.
-static int xrateRemainign=10;
+static int xrateRemaining=10;Consider moving these rate limit variables into the DiscordAccount struct to properly handle multiple accounts.
Likely invalid or redundant comment.
| parse_rate_limit_headers(response); | ||
| const gchar *body; |
There was a problem hiding this comment.
🛠️ 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.
| 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); | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| 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); | |
| } |
There was a problem hiding this comment.
It looks like the whole pull request still does not completely fix the IP temporary bans, only makes them rarer. Very unfortunate.
There was a problem hiding this comment.
@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!
There was a problem hiding this comment.
@coderabbitai By all means, analyze the code any alternative locations where the rate limit might be invokved.
There was a problem hiding this comment.
@Metalhead33, based on the search results, the following locations in the codebase involve rate limit handling or related headers:
-
UpdateRateLimitsfunction:- 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, andxRateDelayPerRequest.
- This function updates rate limit variables (
-
parse_rate_limit_headersfunction:- 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 toUpdateRateLimits.
- This function extracts rate limit headers (
-
discord_response_callbackfunction:- This function calls
parse_rate_limit_headersto process rate limit headers for each HTTP response.
- This function calls
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
|
Houston, we have a problem! Rate limit headers are not present when we are dealing with users instead of bots. So I had to make up my own default values. I intentionally made the delays higher than usual - better safe than sorry, after all. |
Following the AI bot's recommendation for avoiding division by zero. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
libdiscord.c (3)
113-119: Fix typo in variable name and improve naming conventionsThe rate limit variables have a typo and could use more consistent naming:
-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; +static int rate_limit=40; +static int rate_remaining=10; +static double rate_reset=55; +static double rate_reset_after=1; +static int rate_allowed_per_second=40; +static int rate_delay_per_request=100; +static int rate_allowed_remaining=10;
1466-1497: Add validation for negative rate limit valuesThe rate limit update function handles null values and invalid reset times well, but should validate that parsed values are not negative.
static void UpdateRateLimits(const gchar *xrateLimitS, const gchar *xrateRemainingS, const gchar *xRateReset, const gchar *xRateResetAfter) { if(xrateLimitS) { - xrateLimit=atoi(xrateLimitS); + int limit = atoi(xrateLimitS); + if (limit >= 0) { + xrateLimit = limit; + } purple_debug_info("discord", "X-RateLimit-Limit: %s\n", xrateLimitS); } if(xrateRemainingS) { - xrateRemainign=atoi(xrateRemainingS); + int remaining = atoi(xrateRemainingS); + if (remaining >= 0) { + xrateRemainign = remaining; + } purple_debug_info("discord", "X-RateLimit-Remaining: %s\n", xrateRemainingS); }
1692-1692: Define minimum delay as a named constantThe hard-coded minimum delay should be defined as a named constant with a comment explaining its purpose.
+/* Minimum delay between API requests in milliseconds to prevent rate limiting */ +#define DISCORD_MIN_DELAY_MS 65 + - purple_timeout_add(delay + MAX(65,xRateDelayPerRequest), discord_fetch_url_with_method_delay_cb, request); + purple_timeout_add(delay + MAX(DISCORD_MIN_DELAY_MS, xRateDelayPerRequest), discord_fetch_url_with_method_delay_cb, request);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
libdiscord.c(4 hunks)
🔇 Additional comments (2)
libdiscord.c (2)
107-111: Rate limiting implementation looks good overall
The rate limiting implementation properly handles Discord's rate limit headers and calculates appropriate delays between requests. The code includes good error handling and debug logging. The main suggestions are around:
- Consistent variable naming conventions
- Input validation for negative values
- Using named constants instead of magic numbers
- Adding const qualifiers where appropriate
But overall the implementation is solid and should effectively prevent rate limit issues.
Also applies to: 1466-1497
1498-1504: 🛠️ Refactor suggestion
Add input validation and const qualifier
The header parsing function should validate its input and use const for the response parameter.
-static void parse_rate_limit_headers(PurpleHttpResponse *response) {
+static void parse_rate_limit_headers(const PurpleHttpResponse *response) {
+ if (response == NULL) {
+ purple_debug_warning("discord", "Null response in parse_rate_limit_headers\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");
UpdateRateLimits(xrateLimitS, xrateRemainingS, xRateReset, xRateResetAfter);
}Likely invalid or redundant comment.
|
It looks like this pull request still does not completely fix the IP temporary bans, only makes them rarer. Unfortunate. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libdiscord.c (2)
107-120: Consider using a struct to encapsulate rate limit stateThe rate limiting state is currently managed through multiple static variables. This could be better organized by encapsulating these related variables into a dedicated struct, similar to how DiscordTokenBucket is used.
typedef struct { int limit; int remaining; double reset; double resetAfter; int allowedPerSecond; int delayPerRequest; int allowedRemaining; } DiscordRateLimit; static DiscordRateLimit rate_limit = { .limit = 40, .remaining = 10, .reset = 55, .resetAfter = 1, .allowedPerSecond = 40, .delayPerRequest = 100, .allowedRemaining = 10 };This would improve code organization and make it easier to pass rate limit state around if needed.
1692-1692: Consider making the minimum delay configurableThe hardcoded minimum delay of 65ms could be made configurable to allow tuning based on different scenarios.
- purple_timeout_add(delay + MAX(65,xRateDelayPerRequest), discord_fetch_url_with_method_delay_cb, request); + #define DISCORD_MIN_REQUEST_DELAY 65 // milliseconds + purple_timeout_add(delay + MAX(DISCORD_MIN_REQUEST_DELAY,xRateDelayPerRequest), + discord_fetch_url_with_method_delay_cb, request);Consider making this a configurable account option for advanced users who need to tune the rate limiting behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libdiscord.c(4 hunks)
🔇 Additional comments (1)
libdiscord.c (1)
1498-1504:
Add error handling for missing response in parse_rate_limit_headers
The function needs validation of the response parameter and error handling.
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);
}Likely invalid or redundant comment.
libdiscord.c
Outdated
| 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); | ||
| } | ||
| if (xrateResetAfter > 0) { | ||
| xRateAllowedPerSecond = (int)( (double)xrateRemaining / (double)xrateResetAfter ); | ||
| xRateAllowedRemaining = xRateAllowedPerSecond; | ||
| xRateDelayPerRequest = xRateAllowedPerSecond > 0 ? | ||
| (int)((1.0 / (double)xRateAllowedPerSecond) * 1000.0) : 1000; | ||
| purple_debug_info("discord", "Rate limits calculated: %d requests/sec, %d ms delay\n", | ||
| xRateAllowedPerSecond, xRateDelayPerRequest); | ||
| } else { | ||
| purple_debug_warning("discord", "Invalid rate limit reset value\n"); | ||
| xRateAllowedPerSecond = 1; | ||
| xRateAllowedRemaining = 1; | ||
| xRateDelayPerRequest = 1000; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add safety checks and improve error handling in UpdateRateLimits
The function needs additional validation and error handling:
- No validation of input parameters
- Direct use of atoi/atof without error checking
- Potential division by zero if xrateResetAfter is 0 (though handled)
Add parameter validation and safer numeric conversions:
static void UpdateRateLimits(const gchar *xrateLimitS, const gchar *xrateRemainingS,
const gchar *xRateReset, const gchar *xRateResetAfter)
{
+ // Validate inputs
+ if (!xrateLimitS && !xrateRemainingS && !xRateReset && !xRateResetAfter) {
+ purple_debug_warning("discord", "No rate limit headers provided\n");
+ return;
+ }
if(xrateLimitS) {
- xrateLimit = atoi(xrateLimitS);
+ char *endptr;
+ long val = strtol(xrateLimitS, &endptr, 10);
+ if (*endptr == '\0' && val >= 0) {
+ xrateLimit = (int)val;
+ }
purple_debug_info("discord", "X-RateLimit-Limit: %s\n", xrateLimitS);
}
// Similar changes for other numeric conversionsCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
libdiscord.c (3)
1499-1505: Add error handling to rate limit header parsingThe header parsing function should validate the response parameter and handle missing headers gracefully.
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); }
1693-1693: Improve rate limit delay calculationThe rate limit delay calculation should be more robust and configurable.
- purple_timeout_add(delay + MAX(65,xRateDelayPerRequest), discord_fetch_url_with_method_delay_cb, request); + // Minimum delay of 65ms, scale up based on rate limits + const int MIN_DELAY_MS = 65; + int rate_delay = MAX(MIN_DELAY_MS, xRateDelayPerRequest); + // Add jitter to prevent thundering herd + int jitter = g_random_int_range(0, 50); + purple_timeout_add(delay + rate_delay + jitter, + discord_fetch_url_with_method_delay_cb, request);
107-127: Consider improving rate limiting architectureThe current rate limiting implementation has some architectural limitations:
- Uses global variables instead of a proper rate limiting structure
- Lacks proper bucket management for different API endpoints
- Could benefit from more sophisticated rate limiting algorithms
Consider these improvements:
- Create a proper rate limit bucket structure:
typedef struct { guint64 limit; guint64 remaining; gdouble reset; gdouble reset_after; gchar *bucket_id; time_t last_update; } DiscordRateLimitBucket;
- Track rate limits per API endpoint
- Implement token bucket or leaky bucket algorithm
- Add configurable rate limit parameters
- Consider implementing retry strategies with exponential backoff
This would make the rate limiting more robust and maintainable.
Also applies to: 1474-1505, 1693-1693
libdiscord.c
Outdated
| static void UpdateRateLimits(const gchar *xrateLimitS, const gchar *xrateRemainingS, const gchar *xRateReset, const gchar *xRateResetAfter) | ||
| { | ||
| if(!xrateLimitS || !xrateRemainingS || !xRateReset || !xRateResetAfter) return; | ||
| xrateLimit=atoi(xrateLimitS); | ||
| purple_debug_info("discord", "X-RateLimit-Limit: %s\n", xrateLimitS); | ||
| xrateRemainign=atoi(xrateRemainingS); | ||
| purple_debug_info("discord", "X-RateLimit-Remaining: %s\n", xrateRemainingS); | ||
| xrateReset=atof(xRateReset); | ||
| purple_debug_info("discord", "X-RateLimit-Reset: %s\n", xRateReset); | ||
| xrateResetAfter=atof(xRateResetAfter) * 4.0; | ||
| purple_debug_info("discord", "X-RateLimit-Reset-After: %s\n", xRateResetAfter); | ||
| if (xrateResetAfter > 0) { | ||
| xRateAllowedPerSecond = (int)( (double)xrateRemaining / (double)xrateResetAfter ); | ||
| xRateAllowedRemaining = xRateAllowedPerSecond; | ||
| xRateDelayPerRequest = xRateAllowedPerSecond > 0 ? | ||
| (int)((1.0 / (double)xRateAllowedPerSecond) * 1000.0) : 1000; | ||
| purple_debug_info("discord", "Rate limits calculated: %d requests/sec, %d ms delay\n", | ||
| xRateAllowedPerSecond, xRateDelayPerRequest); | ||
| } else { | ||
| purple_debug_warning("discord", "Invalid rate limit reset value\n"); | ||
| xRateAllowedPerSecond = 1; | ||
| xRateAllowedRemaining = 1; | ||
| xRateDelayPerRequest = 1000; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve rate limit update error handling and logging
The rate limit update function needs better error handling and logging:
- The function should validate input parameters more thoroughly
- The error case should set sensible defaults
- Debug logging should be more descriptive
Suggested improvements:
static void UpdateRateLimits(const gchar *xrateLimitS, const gchar *xrateRemainingS,
const gchar *xRateReset, const gchar *xRateResetAfter)
{
- if(!xrateLimitS || !xrateRemainingS || !xRateReset || !xRateResetAfter) return;
+ if(!xrateLimitS || !xrateRemainingS || !xRateReset || !xRateResetAfter) {
+ purple_debug_warning("discord", "Missing rate limit headers\n");
+ return;
+ }
xrateLimit = atoi(xrateLimitS);
- purple_debug_info("discord", "X-RateLimit-Limit: %s\n", xrateLimitS);
+ purple_debug_info("discord", "Rate limit update - Limit: %d\n", xrateLimit);
- xrateRemainign = atoi(xrateRemainingS);
+ xrateRemaining = atoi(xrateRemainingS);
- purple_debug_info("discord", "X-RateLimit-Remaining: %s\n", xrateRemainingS);
+ purple_debug_info("discord", "Rate limit update - Remaining: %d\n", xrateRemaining);
// Multiply reset after by 4 to be more conservative with rate limiting
xrateResetAfter = atof(xRateResetAfter) * 4.0;
if (xrateResetAfter > 0) {
- xRateAllowedPerSecond = (int)((double)xrateRemaining / (double)xrateResetAfter);
+ xRateAllowedPerSecond = (int)((double)xrateRemaining / (double)xrateResetAfter);
xRateAllowedRemaining = xRateAllowedPerSecond;
xRateDelayPerRequest = xRateAllowedPerSecond > 0 ?
(int)((1.0 / (double)xRateAllowedPerSecond) * 1000.0) : 1000;
} else {
purple_debug_warning("discord", "Invalid rate limit reset value\n");
// Set conservative defaults
xRateAllowedPerSecond = 1;
xRateAllowedRemaining = 1;
xRateDelayPerRequest = 1000;
}
}📝 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.
| static void UpdateRateLimits(const gchar *xrateLimitS, const gchar *xrateRemainingS, const gchar *xRateReset, const gchar *xRateResetAfter) | |
| { | |
| if(!xrateLimitS || !xrateRemainingS || !xRateReset || !xRateResetAfter) return; | |
| xrateLimit=atoi(xrateLimitS); | |
| purple_debug_info("discord", "X-RateLimit-Limit: %s\n", xrateLimitS); | |
| xrateRemainign=atoi(xrateRemainingS); | |
| purple_debug_info("discord", "X-RateLimit-Remaining: %s\n", xrateRemainingS); | |
| xrateReset=atof(xRateReset); | |
| purple_debug_info("discord", "X-RateLimit-Reset: %s\n", xRateReset); | |
| xrateResetAfter=atof(xRateResetAfter) * 4.0; | |
| purple_debug_info("discord", "X-RateLimit-Reset-After: %s\n", xRateResetAfter); | |
| if (xrateResetAfter > 0) { | |
| xRateAllowedPerSecond = (int)( (double)xrateRemaining / (double)xrateResetAfter ); | |
| xRateAllowedRemaining = xRateAllowedPerSecond; | |
| xRateDelayPerRequest = xRateAllowedPerSecond > 0 ? | |
| (int)((1.0 / (double)xRateAllowedPerSecond) * 1000.0) : 1000; | |
| purple_debug_info("discord", "Rate limits calculated: %d requests/sec, %d ms delay\n", | |
| xRateAllowedPerSecond, xRateDelayPerRequest); | |
| } else { | |
| purple_debug_warning("discord", "Invalid rate limit reset value\n"); | |
| xRateAllowedPerSecond = 1; | |
| xRateAllowedRemaining = 1; | |
| xRateDelayPerRequest = 1000; | |
| } | |
| } | |
| static void UpdateRateLimits(const gchar *xrateLimitS, const gchar *xrateRemainingS, const gchar *xRateReset, const gchar *xRateResetAfter) | |
| { | |
| if(!xrateLimitS || !xrateRemainingS || !xRateReset || !xRateResetAfter) { | |
| purple_debug_warning("discord", "Missing rate limit headers\n"); | |
| return; | |
| } | |
| xrateLimit = atoi(xrateLimitS); | |
| purple_debug_info("discord", "Rate limit update - Limit: %d\n", xrateLimit); | |
| xrateRemaining = atoi(xrateRemainingS); | |
| purple_debug_info("discord", "Rate limit update - Remaining: %d\n", xrateRemaining); | |
| xrateReset=atof(xRateReset); | |
| purple_debug_info("discord", "X-RateLimit-Reset: %s\n", xRateReset); | |
| // Multiply reset after by 4 to be more conservative with rate limiting | |
| xrateResetAfter=atof(xRateResetAfter) * 4.0; | |
| purple_debug_info("discord", "X-RateLimit-Reset-After: %s\n", xRateResetAfter); | |
| if (xrateResetAfter > 0) { | |
| xRateAllowedPerSecond = (int)((double)xrateRemaining / (double)xrateResetAfter); | |
| xRateAllowedRemaining = xRateAllowedPerSecond; | |
| xRateDelayPerRequest = xRateAllowedPerSecond > 0 ? | |
| (int)((1.0 / (double)xRateAllowedPerSecond) * 1000.0) : 1000; | |
| purple_debug_info("discord", "Rate limits calculated: %d requests/sec, %d ms delay\n", | |
| xRateAllowedPerSecond, xRateDelayPerRequest); | |
| } else { | |
| purple_debug_warning("discord", "Invalid rate limit reset value\n"); | |
| // Set conservative defaults | |
| xRateAllowedPerSecond = 1; | |
| xRateAllowedRemaining = 1; | |
| xRateDelayPerRequest = 1000; | |
| } | |
| } |
|
Fingers crossed that the newest version will make rate limiting even rarer. My code changes so far haven't been particularly invasive, but if I get another tempban, I might have to resort to more invasive measures, such as some kind of queue. The next time I get a tempban, I will program in some sort of FIFO queue or event loop. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libdiscord.c (3)
107-127: Fix typo in variable nameThere is a typo in the variable name "xrateRemainign" in the comment. This should be fixed to match the actual variable name "xrateRemaining" to avoid confusion.
- xRateAllowedPerSecond = (int)( (double)xrateRemainign / (double)xrateResetAfter ); + xRateAllowedPerSecond = (int)( (double)xrateRemaining / (double)xrateResetAfter );
1481-1506: Add input validation for rate limit valuesWhile the function handles division by zero, it could benefit from additional input validation:
- Validate that numeric values are non-negative
- Add bounds checking for reasonable limits
- Log warnings for unexpected values
static void UpdateRateLimits(const gchar *xrateLimitS, const gchar *xrateRemainingS, const gchar *xRateReset, const gchar *xRateResetAfter) { if(!xrateLimitS || !xrateRemainingS || !xRateReset || !xRateResetAfter) return; + + // Parse and validate values xrateLimit=atoi(xrateLimitS); + if (xrateLimit < 0) { + purple_debug_warning("discord", "Invalid negative rate limit %d\n", xrateLimit); + xrateLimit = init_xrateLimit; + } + purple_debug_info("discord", "X-RateLimit-Limit: %s\n", xrateLimitS); + xrateRemaining=atoi(xrateRemainingS); + if (xrateRemaining < 0 || xrateRemaining > xrateLimit) { + purple_debug_warning("discord", "Invalid remaining rate limit %d\n", xrateRemaining); + xrateRemaining = init_xrateRemaining; + }
1701-1701: Define minimum delay as a named constantThe minimum delay value of 65ms should be defined as a named constant for better maintainability and clarity.
+#define MIN_REQUEST_DELAY_MS 65 // Minimum delay between requests in milliseconds + - purple_timeout_add(delay + getJitteredDelay(MAX(65,xRateDelayPerRequest)), discord_fetch_url_with_method_delay_cb, request); + purple_timeout_add(delay + getJitteredDelay(MAX(MIN_REQUEST_DELAY_MS,xRateDelayPerRequest)), discord_fetch_url_with_method_delay_cb, request);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libdiscord.c(4 hunks)
🔇 Additional comments (2)
libdiscord.c (2)
129-134: LGTM!
Good implementation of jittered delays to help prevent thundering herd problems. The functions are simple, focused and use appropriate randomization.
1507-1513: 🛠️ Refactor suggestion
Add error handling for missing rate limit headers
The function should handle cases where rate limit headers are missing and log appropriate warnings.
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);
}Likely invalid or redundant comment.
|
@EionRobb Can we have it merged in? |
Still a work in progress, but I decided to take matters into my own hands because #450 and #448 really get on my nerves.
Summary by CodeRabbit
New Features
Bug Fixes