diff --git a/PluralKit.Bot/Services/ProxyService.cs b/PluralKit.Bot/Services/ProxyService.cs index eddf13a1..ebee3c48 100644 --- a/PluralKit.Bot/Services/ProxyService.cs +++ b/PluralKit.Bot/Services/ProxyService.cs @@ -104,13 +104,13 @@ namespace PluralKit.Bot var messageContents = SanitizeEveryoneMaybe(message, match.InnerText); // Fetch a webhook for this channel, and send the proxied message - var webhook = await _webhookCache.GetWebhook(message.Channel as ITextChannel); + var webhookCacheEntry = await _webhookCache.GetWebhook(message.Channel as ITextChannel); var avatarUrl = match.Member.AvatarUrl ?? match.System.AvatarUrl; var proxyName = match.Member.ProxyName(match.System.Tag); ulong hookMessageId; using (_metrics.Measure.Timer.Time(BotMetrics.WebhookResponseTime)) - hookMessageId = await ExecuteWebhook(webhook, messageContents, proxyName, avatarUrl, message.Attachments.FirstOrDefault()); + hookMessageId = await ExecuteWebhook(webhookCacheEntry, messageContents, proxyName, avatarUrl, message.Attachments.FirstOrDefault()); // Store the message in the database, and log it in the log channel (if applicable) await _messageStorage.Store(message.Author.Id, hookMessageId, message.Channel.Id, message.Id, match.Member); @@ -153,70 +153,53 @@ namespace PluralKit.Bot return true; } - private async Task ExecuteWebhook(IWebhook webhook, string text, string username, string avatarUrl, IAttachment attachment) + private async Task ExecuteWebhook(WebhookCacheService.WebhookCacheEntry cacheEntry, string text, string username, string avatarUrl, IAttachment attachment) { username = FixClyde(username); // TODO: DiscordWebhookClient's ctor does a call to GetWebhook that may be unnecessary, see if there's a way to do this The Hard Way :tm: // TODO: this will probably crash if there are multiple consecutive failures, perhaps have a loop instead? - - _logger.Debug("Instantiating webhook client"); - DiscordWebhookClient client; - try - { - client = new DiscordWebhookClient(webhook); - } - catch (InvalidOperationException) - { - // TODO: does this leak internal stuff in the (now-invalid) client? - // webhook was deleted or invalid - webhook = await _webhookCache.InvalidateAndRefreshWebhook(webhook); - client = new DiscordWebhookClient(webhook); - } - _logger.Debug("Invoking webhook"); // TODO: clean this entire block up - using (client) + ulong messageId; + + try { - ulong messageId; - - try + var client = cacheEntry.Client; + if (attachment != null) { - if (attachment != null) + using (var stream = await _httpClient.GetStreamAsync(attachment.Url)) { - using (var stream = await _httpClient.GetStreamAsync(attachment.Url)) - { - messageId = await client.SendFileAsync(stream, filename: attachment.Filename, text: text, - username: username, avatarUrl: avatarUrl); - } + messageId = await client.SendFileAsync(stream, filename: attachment.Filename, text: text, + username: username, avatarUrl: avatarUrl); } - else - { - messageId = await client.SendMessageAsync(text, username: username, avatarUrl: avatarUrl); - } - - _logger.Information("Invoked webhook {Webhook} in channel {Channel}", webhook.Id, - webhook.ChannelId); - - // Log it in the metrics - _metrics.Measure.Meter.Mark(BotMetrics.MessagesProxied, "success"); } - catch (HttpException e) + else { - _logger.Warning(e, "Error invoking webhook {Webhook} in channel {Channel}", webhook.Id, - webhook.ChannelId); - - // Log failure in metrics and rethrow (we still need to cancel everything else) - _metrics.Measure.Meter.Mark(BotMetrics.MessagesProxied, "failure"); - throw; + messageId = await client.SendMessageAsync(text, username: username, avatarUrl: avatarUrl); } - // TODO: figure out a way to return the full message object (without doing a GetMessageAsync call, which - // doesn't work if there's no permission to) - return messageId; + _logger.Information("Invoked webhook {Webhook} in channel {Channel}", cacheEntry.Webhook.Id, + cacheEntry.Webhook.ChannelId); + + // Log it in the metrics + _metrics.Measure.Meter.Mark(BotMetrics.MessagesProxied, "success"); } + catch (HttpException e) + { + _logger.Warning(e, "Error invoking webhook {Webhook} in channel {Channel}", cacheEntry.Webhook.Id, + cacheEntry.Webhook.ChannelId); + + // Log failure in metrics and rethrow (we still need to cancel everything else) + _metrics.Measure.Meter.Mark(BotMetrics.MessagesProxied, "failure"); + throw; + } + + // TODO: figure out a way to return the full message object (without doing a GetMessageAsync call, which + // doesn't work if there's no permission to) + return messageId; } public Task HandleReactionAddedAsync(Cacheable message, ISocketMessageChannel channel, SocketReaction reaction) diff --git a/PluralKit.Bot/Services/WebhookCacheService.cs b/PluralKit.Bot/Services/WebhookCacheService.cs index 02bf0017..f7eaa888 100644 --- a/PluralKit.Bot/Services/WebhookCacheService.cs +++ b/PluralKit.Bot/Services/WebhookCacheService.cs @@ -3,16 +3,23 @@ using System.Collections.Concurrent; using System.Linq; using System.Threading.Tasks; using Discord; +using Discord.Webhook; using Serilog; namespace PluralKit.Bot { - public class WebhookCacheService + public class WebhookCacheService: IDisposable { + public class WebhookCacheEntry + { + internal DiscordWebhookClient Client; + internal IWebhook Webhook; + } + public static readonly string WebhookName = "PluralKit Proxy Webhook"; private IDiscordClient _client; - private ConcurrentDictionary>> _webhooks; + private ConcurrentDictionary>> _webhooks; private ILogger _logger; @@ -20,56 +27,79 @@ namespace PluralKit.Bot { _client = client; _logger = logger.ForContext(); - _webhooks = new ConcurrentDictionary>>(); + _webhooks = new ConcurrentDictionary>>(); } - public async Task GetWebhook(ulong channelId) + public async Task GetWebhook(ulong channelId) { var channel = await _client.GetChannelAsync(channelId) as ITextChannel; if (channel == null) return null; return await GetWebhook(channel); } - public async Task GetWebhook(ITextChannel channel) + public async Task GetWebhook(ITextChannel channel) { // We cache the webhook through a Lazy>, this way we make sure to only create one webhook per channel // If the webhook is requested twice before it's actually been found, the Lazy wrapper will stop the // webhook from being created twice. var lazyWebhookValue = - _webhooks.GetOrAdd(channel.Id, new Lazy>(() => GetOrCreateWebhook(channel))); + _webhooks.GetOrAdd(channel.Id, new Lazy>(() => GetOrCreateWebhook(channel))); // It's possible to "move" a webhook to a different channel after creation // Here, we ensure it's actually still pointing towards the proper channel, and if not, wipe and refetch one. var webhook = await lazyWebhookValue.Value; - if (webhook.Channel.Id != channel.Id) return await InvalidateAndRefreshWebhook(webhook); + if (webhook.Webhook.ChannelId != channel.Id) return await InvalidateAndRefreshWebhook(webhook); return webhook; } - public async Task InvalidateAndRefreshWebhook(IWebhook webhook) + public async Task InvalidateAndRefreshWebhook(WebhookCacheEntry webhook) { - _logger.Information("Refreshing webhook for channel {Channel}", webhook.ChannelId); + _logger.Information("Refreshing webhook for channel {Channel}", webhook.Webhook.ChannelId); - _webhooks.TryRemove(webhook.ChannelId, out _); - return await GetWebhook(webhook.ChannelId); + _webhooks.TryRemove(webhook.Webhook.ChannelId, out _); + return await GetWebhook(webhook.Webhook.Channel); } - private async Task GetOrCreateWebhook(ITextChannel channel) => - await FindExistingWebhook(channel) ?? await DoCreateWebhook(channel); - + private async Task GetOrCreateWebhook(ITextChannel channel) + { + var webhook = await FindExistingWebhook(channel) ?? await DoCreateWebhook(channel); + return await DoCreateWebhookClient(webhook); + } + private async Task FindExistingWebhook(ITextChannel channel) { _logger.Debug("Finding webhook for channel {Channel}", channel.Id); return (await channel.GetWebhooksAsync()).FirstOrDefault(IsWebhookMine); } - private async Task DoCreateWebhook(ITextChannel channel) + private Task DoCreateWebhook(ITextChannel channel) { _logger.Information("Creating new webhook for channel {Channel}", channel.Id); - return await channel.CreateWebhookAsync(WebhookName); + return channel.CreateWebhookAsync(WebhookName); } - private bool IsWebhookMine(IWebhook arg) => arg.Creator.Id == _client.CurrentUser.Id && arg.Name == WebhookName; + private Task DoCreateWebhookClient(IWebhook webhook) + { + // DiscordWebhookClient's ctor is synchronous despite doing web calls, so we wrap it in a Task + return Task.Run(() => + { + return new WebhookCacheEntry + { + Client = new DiscordWebhookClient(webhook), + Webhook = webhook + }; + }); + } + + private bool IsWebhookMine(IWebhook arg) => arg.Id == _client.CurrentUser.Id && arg.Name == WebhookName; public int CacheSize => _webhooks.Count; + + public void Dispose() + { + foreach (var entry in _webhooks.Values) + if (entry.IsValueCreated) + entry.Value.Result.Client.Dispose(); + } } } \ No newline at end of file