From 546cb7f97a2992c736be433bb4f6c8a7d99363ea Mon Sep 17 00:00:00 2001 From: Ske Date: Fri, 1 May 2020 00:00:33 +0200 Subject: [PATCH] Remove webhook rate limit cache The move to DSharpPlus makes it unnecessary, as D#+ can actually do webhook invocations on its own. --- PluralKit.Bot/Bot.cs | 12 +- PluralKit.Bot/Modules.cs | 1 - .../Services/PeriodicStatCollector.cs | 5 +- .../Services/WebhookExecutorService.cs | 4 +- .../Services/WebhookRateLimitService.cs | 128 ------------------ 5 files changed, 3 insertions(+), 147 deletions(-) delete mode 100644 PluralKit.Bot/Services/WebhookRateLimitService.cs diff --git a/PluralKit.Bot/Bot.cs b/PluralKit.Bot/Bot.cs index 4e49b8ac..bde58fe5 100644 --- a/PluralKit.Bot/Bot.cs +++ b/PluralKit.Bot/Bot.cs @@ -102,21 +102,17 @@ namespace PluralKit.Bot { private ILifetimeScope _services; private DiscordShardedClient _client; - private Timer _updateTimer; private IMetrics _metrics; private PeriodicStatCollector _collector; private ILogger _logger; - private WebhookRateLimitService _webhookRateLimit; - private int _periodicUpdateCount; private Task _periodicWorker; - public Bot(ILifetimeScope services, DiscordShardedClient client, IMetrics metrics, PeriodicStatCollector collector, ILogger logger, WebhookRateLimitService webhookRateLimit) + public Bot(ILifetimeScope services, DiscordShardedClient client, IMetrics metrics, PeriodicStatCollector collector, ILogger logger) { _services = services; _client = client; _metrics = metrics; _collector = collector; - _webhookRateLimit = webhookRateLimit; _logger = logger.ForContext(); } @@ -182,12 +178,6 @@ namespace PluralKit.Bot } catch (WebSocketException) { } - // Run webhook rate limit GC every 10 minutes - if (_periodicUpdateCount++ % 10 == 0) - { - var _ = Task.Run(() => _webhookRateLimit.GarbageCollect()); - } - await _collector.CollectStats(); _logger.Information("Submitted metrics to backend"); diff --git a/PluralKit.Bot/Modules.cs b/PluralKit.Bot/Modules.cs index bd90468e..16da31b9 100644 --- a/PluralKit.Bot/Modules.cs +++ b/PluralKit.Bot/Modules.cs @@ -55,7 +55,6 @@ namespace PluralKit.Bot builder.RegisterType().AsSelf().SingleInstance(); builder.RegisterType().AsSelf().SingleInstance(); builder.RegisterType().AsSelf().SingleInstance(); - builder.RegisterType().AsSelf().SingleInstance(); builder.RegisterType().AsSelf().SingleInstance(); builder.RegisterType().AsSelf().SingleInstance(); builder.RegisterType().AsSelf().SingleInstance(); diff --git a/PluralKit.Bot/Services/PeriodicStatCollector.cs b/PluralKit.Bot/Services/PeriodicStatCollector.cs index ef2171b9..73114be3 100644 --- a/PluralKit.Bot/Services/PeriodicStatCollector.cs +++ b/PluralKit.Bot/Services/PeriodicStatCollector.cs @@ -24,13 +24,12 @@ namespace PluralKit.Bot private IDataStore _data; private WebhookCacheService _webhookCache; - private WebhookRateLimitService _webhookRateLimitCache; private DbConnectionCountHolder _countHolder; private ILogger _logger; - public PeriodicStatCollector(DiscordShardedClient client, IMetrics metrics, ILogger logger, WebhookCacheService webhookCache, DbConnectionCountHolder countHolder, IDataStore data, CpuStatService cpu, WebhookRateLimitService webhookRateLimitCache) + public PeriodicStatCollector(DiscordShardedClient client, IMetrics metrics, ILogger logger, WebhookCacheService webhookCache, DbConnectionCountHolder countHolder, IDataStore data, CpuStatService cpu) { _client = client; _metrics = metrics; @@ -38,7 +37,6 @@ namespace PluralKit.Bot _countHolder = countHolder; _data = data; _cpu = cpu; - _webhookRateLimitCache = webhookRateLimitCache; _logger = logger.ForContext(); } @@ -99,7 +97,6 @@ namespace PluralKit.Bot // Other shiz _metrics.Measure.Gauge.SetValue(BotMetrics.WebhookCacheSize, _webhookCache.CacheSize); - _metrics.Measure.Gauge.SetValue(BotMetrics.WebhookRateLimitCacheSize, _webhookRateLimitCache.CacheSize); stopwatch.Stop(); _logger.Information("Updated metrics in {Time}", stopwatch.ElapsedDuration()); diff --git a/PluralKit.Bot/Services/WebhookExecutorService.cs b/PluralKit.Bot/Services/WebhookExecutorService.cs index adc9d67d..4d2c3a15 100644 --- a/PluralKit.Bot/Services/WebhookExecutorService.cs +++ b/PluralKit.Bot/Services/WebhookExecutorService.cs @@ -31,17 +31,15 @@ namespace PluralKit.Bot public class WebhookExecutorService { private WebhookCacheService _webhookCache; - private WebhookRateLimitService _rateLimit; private ILogger _logger; private IMetrics _metrics; private HttpClient _client; - public WebhookExecutorService(IMetrics metrics, WebhookCacheService webhookCache, ILogger logger, HttpClient client, WebhookRateLimitService rateLimit) + public WebhookExecutorService(IMetrics metrics, WebhookCacheService webhookCache, ILogger logger, HttpClient client) { _metrics = metrics; _webhookCache = webhookCache; _client = client; - _rateLimit = rateLimit; _logger = logger.ForContext(); } diff --git a/PluralKit.Bot/Services/WebhookRateLimitService.cs b/PluralKit.Bot/Services/WebhookRateLimitService.cs deleted file mode 100644 index 9468bcaf..00000000 --- a/PluralKit.Bot/Services/WebhookRateLimitService.cs +++ /dev/null @@ -1,128 +0,0 @@ -using System; -using System.Collections.Concurrent; -using System.Globalization; -using System.Linq; -using System.Net; -using System.Net.Http; - -using DSharpPlus.Entities; - -using NodaTime; - -using Serilog; - -namespace PluralKit.Bot -{ - // Simplified rate limit handler for webhooks only, disregards most bucket functionality since scope is limited and just denies requests if too fast. - public class WebhookRateLimitService - { - private ILogger _logger; - private ConcurrentDictionary _info = new ConcurrentDictionary(); - - public WebhookRateLimitService(ILogger logger) - { - _logger = logger.ForContext(); - } - - public int CacheSize => _info.Count; - - public bool TryExecuteWebhook(DiscordWebhook webhook) - { - // If we have nothing saved, just allow it (we'll save something once the response returns) - if (!_info.TryGetValue(webhook.Id, out var info)) return true; - - // If we're past the reset time, allow the request and update the bucket limit - if (SystemClock.Instance.GetCurrentInstant() > info.resetTime) - { - if (!info.hasResetTimeExpired) - info.remaining = info.maxLimit; - info.hasResetTimeExpired = true; - - // We can hit this multiple times if many requests are in flight before a real one gets "back", so we still - // decrement the remaining request count, this basically "blacklists" the channel given continuous spam until *one* of the requests come back with new rate limit headers - info.remaining--; - - return true; - } - - // If we don't have any more requests left, deny the request - if (info.remaining == 0) - { - _logger.Debug("Rate limit bucket for {Webhook} out of requests, denying request", webhook.Id); - return false; - } - - // Otherwise, decrement the request count and allow the request - info.remaining--; - return true; - } - - public void UpdateRateLimitInfo(DiscordWebhook webhook, HttpResponseMessage response) - { - var info = _info.GetOrAdd(webhook.Id, _ => new WebhookRateLimitInfo()); - - if (int.TryParse(GetHeader(response, "X-RateLimit-Limit"), out var limit)) - info.maxLimit = limit; - - // Max "safe" is way above UNIX timestamp values, and we get fractional seconds, hence the double - // but need culture/format specifiers to get around Some Locales (cough, my local PC) having different settings for decimal point... - // We also use Reset-After to avoid issues with clock desync between us and Discord's server, this way it's all relative (plus latency errors work in our favor) - if (double.TryParse(GetHeader(response, "X-RateLimit-Reset-After"), NumberStyles.Float, CultureInfo.InvariantCulture, out var resetTimestampDelta)) - { - var resetTime = SystemClock.Instance.GetCurrentInstant() + Duration.FromSeconds(resetTimestampDelta); - if (resetTime > info.resetTime) - { - // Set to the *latest* reset value we have (for safety), since we rely on relative times this can jitter a bit - info.resetTime = resetTime; - info.hasResetTimeExpired = false; - } - } - - if (int.TryParse(GetHeader(response, "X-RateLimit-Remaining"), out var remainingRequests)) - // Overwrite a negative "we don't know" value with whatever we just got - // Otherwise, *lower* remaining requests takes precedence - if (info.remaining < 0 || remainingRequests < info.remaining) - info.remaining = remainingRequests; - - _logger.Debug("Updated rate limit information for {Webhook}, bucket has {RequestsRemaining} requests remaining, reset in {ResetTime}", webhook.Id, info.remaining, info.resetTime - SystemClock.Instance.GetCurrentInstant()); - - if (response.StatusCode == HttpStatusCode.TooManyRequests) - { - // 429, we're *definitely* out of requests - info.remaining = 0; - _logger.Warning("Got 429 Too Many Requests when invoking webhook {Webhook}, next bucket reset in {ResetTime}", webhook.Id, info.resetTime - SystemClock.Instance.GetCurrentInstant()); - } - } - - public void GarbageCollect() - { - _logger.Information("Garbage-collecting webhook rate limit buckets..."); - - var collected = 0; - foreach (var channel in _info.Keys) - { - if (!_info.TryGetValue(channel, out var info)) continue; - - // Remove all keys that expired more than an hour ago (and of course, haven't been reset) - if (info.resetTime < SystemClock.Instance.GetCurrentInstant() - Duration.FromHours(1)) - if (_info.TryRemove(channel, out _)) collected++; - } - - _logger.Information("Garbage-collected {ChannelCount} channels from the webhook rate limit buckets.", collected); - } - - private string GetHeader(HttpResponseMessage response, string key) - { - var firstPair = response.Headers.FirstOrDefault(pair => pair.Key.Equals(key, StringComparison.InvariantCultureIgnoreCase)); - return firstPair.Value?.FirstOrDefault(); // If key is missing, default value is null - } - - private class WebhookRateLimitInfo - { - public Instant resetTime; - public bool hasResetTimeExpired; - public int remaining = -1; - public int maxLimit = 0; - } - } -} \ No newline at end of file