From 474d561c544a9086fe09bf1bab208af075c59228 Mon Sep 17 00:00:00 2001 From: Ske Date: Sat, 21 Dec 2019 18:50:28 +0100 Subject: [PATCH] Execute webhooks directly rather than through D.NET --- PluralKit.Bot/PluralKit.Bot.csproj | 1 - PluralKit.Bot/Services/WebhookCacheService.cs | 1 - .../Services/WebhookExecutorService.cs | 99 +++++++------------ 3 files changed, 36 insertions(+), 65 deletions(-) diff --git a/PluralKit.Bot/PluralKit.Bot.csproj b/PluralKit.Bot/PluralKit.Bot.csproj index 3bc6da4d..a8244a45 100644 --- a/PluralKit.Bot/PluralKit.Bot.csproj +++ b/PluralKit.Bot/PluralKit.Bot.csproj @@ -10,7 +10,6 @@ - diff --git a/PluralKit.Bot/Services/WebhookCacheService.cs b/PluralKit.Bot/Services/WebhookCacheService.cs index 4475bba3..f6cbcdec 100644 --- a/PluralKit.Bot/Services/WebhookCacheService.cs +++ b/PluralKit.Bot/Services/WebhookCacheService.cs @@ -4,7 +4,6 @@ using System.Linq; using System.Net.Http; using System.Threading.Tasks; using Discord; -using Discord.Webhook; using Discord.WebSocket; using Serilog; diff --git a/PluralKit.Bot/Services/WebhookExecutorService.cs b/PluralKit.Bot/Services/WebhookExecutorService.cs index 81556b6b..a77b54b2 100644 --- a/PluralKit.Bot/Services/WebhookExecutorService.cs +++ b/PluralKit.Bot/Services/WebhookExecutorService.cs @@ -3,11 +3,14 @@ using System.Net.Http; using System.Text.RegularExpressions; using System.Threading.Tasks; using App.Metrics; -using App.Metrics.Logging; + using Discord; -using Discord.Net; -using Discord.Webhook; -using Microsoft.Extensions.Caching.Memory; + +using Humanizer; + +using Newtonsoft.Json; +using Newtonsoft.Json.Linq; + using Serilog; namespace PluralKit.Bot @@ -15,14 +18,12 @@ namespace PluralKit.Bot public class WebhookExecutorService: IDisposable { private WebhookCacheService _webhookCache; - private IMemoryCache _cache; private ILogger _logger; private IMetrics _metrics; private HttpClient _client; - public WebhookExecutorService(IMemoryCache cache, IMetrics metrics, WebhookCacheService webhookCache, ILogger logger) + public WebhookExecutorService(IMetrics metrics, WebhookCacheService webhookCache, ILogger logger) { - _cache = cache; _metrics = metrics; _webhookCache = webhookCache; _logger = logger.ForContext(); @@ -48,72 +49,44 @@ namespace PluralKit.Bot private async Task ExecuteWebhookInner(IWebhook webhook, string name, string avatarUrl, string content, IAttachment attachment, bool hasRetried = false) { - var client = await GetClientFor(webhook); - try + var mfd = new MultipartFormDataContent(); + mfd.Add(new StringContent(content.Truncate(2000)), "content"); + mfd.Add(new StringContent(FixClyde(name).Truncate(80)), "username"); + if (avatarUrl != null) mfd.Add(new StringContent(avatarUrl), "avatar_url"); + + if (attachment != null) { - // If we have an attachment, use the special SendFileAsync method - if (attachment != null) - using (var attachmentStream = await _client.GetStreamAsync(attachment.Url)) - using (_metrics.Measure.Timer.Time(BotMetrics.WebhookResponseTime)) - return await client.SendFileAsync(attachmentStream, attachment.Filename, content, - username: FixClyde(name), - avatarUrl: avatarUrl); - - // Otherwise, send normally - return await client.SendMessageAsync(content, username: FixClyde(name), avatarUrl: avatarUrl); + var attachmentResponse = await _client.GetAsync(attachment.Url); + var attachmentStream = await attachmentResponse.Content.ReadAsStreamAsync(); + mfd.Add(new StreamContent(attachmentStream), "file", attachment.Filename); } - catch (HttpException e) + + HttpResponseMessage response; + using (_metrics.Measure.Timer.Time(BotMetrics.WebhookResponseTime)) + response = await _client.PostAsync($"{DiscordConfig.APIUrl}webhooks/{webhook.Id}/{webhook.Token}?wait=true", mfd); + + // TODO: are there cases where an error won't also return a parseable JSON object? + var responseJson = JsonConvert.DeserializeObject(await response.Content.ReadAsStringAsync()); + if (responseJson.ContainsKey("code")) { - // If we hit an error, just retry (if we haven't already) - if (e.DiscordCode == 10015 && !hasRetried) // Error 10015 = "Unknown Webhook" + if (responseJson["code"].Value() == 10015 && !hasRetried) { - _logger.Warning(e, "Error invoking webhook {Webhook} in channel {Channel}", webhook.Id, webhook.ChannelId); + // Error 10015 = "Unknown Webhook" - this likely means the webhook was deleted + // but is still in our cache. Invalidate, refresh, try again + _logger.Warning("Error invoking webhook {Webhook} in channel {Channel}", webhook.Id, webhook.ChannelId); return await ExecuteWebhookInner(await _webhookCache.InvalidateAndRefreshWebhook(webhook), name, avatarUrl, content, attachment, hasRetried: true); } - - throw; + + // TODO: look into what this actually throws, and if this is the correct handling + response.EnsureSuccessStatusCode(); } - } - - private async Task GetClientFor(IWebhook webhook) - { - _logger.Verbose("Looking for client for webhook {Webhook} in cache", webhook.Id); - return await _cache.GetOrCreateAsync($"_webhook_client_{webhook.Id}", - (entry) => MakeCachedClientFor(entry, webhook)); - } - - private Task MakeCachedClientFor(ICacheEntry entry, IWebhook webhook) { - _logger.Information("Client for {Webhook} not found in cache, creating", webhook.Id); - // Define expiration for the client cache - // 10 minutes *without a query* and it gets yoten - entry.SlidingExpiration = TimeSpan.FromMinutes(10); - - // IMemoryCache won't automatically dispose of its values when the cache gets evicted - // so we register a hook to do so here. - entry.RegisterPostEvictionCallback((key, value, reason, state) => (value as IDisposable)?.Dispose()); - - // DiscordWebhookClient has a sync network call in its constructor (!!!) - // and we want to punt that onto a task queue, so we do that. - return Task.Run(async () => - { - try - { - return new DiscordWebhookClient(webhook); - } - catch (InvalidOperationException) - { - // TODO: does this leak stuff inside the DiscordWebhookClient created above? - - // Webhook itself was found in cache, but has been deleted on the channel - // We request a new webhook instead - return new DiscordWebhookClient(await _webhookCache.InvalidateAndRefreshWebhook(webhook)); - } - - }); + // At this point we're sure we have a 2xx status code, so just assume success + // TODO: can we do this without a round-trip to a string? + return responseJson["id"].Value(); } - + private string FixClyde(string name) { // Check if the name contains "Clyde" - if not, do nothing