From 04b68655bcd409b646301263e7d774ced4dcbb60 Mon Sep 17 00:00:00 2001 From: Noi Date: Mon, 19 Jun 2023 01:47:03 -0700 Subject: [PATCH 1/7] Remove unnecessary LongRunning setting --- ShardManager.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ShardManager.cs b/ShardManager.cs index d742725..5fe1abd 100644 --- a/ShardManager.cs +++ b/ShardManager.cs @@ -43,8 +43,7 @@ class ShardManager : IDisposable { // Start status reporting thread _mainCancel = new CancellationTokenSource(); - _statusTask = Task.Factory.StartNew(StatusLoop, _mainCancel.Token, - TaskCreationOptions.LongRunning, TaskScheduler.Default); + _statusTask = Task.Factory.StartNew(StatusLoop, _mainCancel.Token); } public void Dispose() { From cee7bb48357eb5088889efd3729ebf84cd19abe3 Mon Sep 17 00:00:00 2001 From: Noi Date: Sat, 24 Jun 2023 16:04:09 -0700 Subject: [PATCH 2/7] Manually call GC on occasion (experimental) --- BackgroundServices/AutoUserDownload.cs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/BackgroundServices/AutoUserDownload.cs b/BackgroundServices/AutoUserDownload.cs index cc7f588..a396f03 100644 --- a/BackgroundServices/AutoUserDownload.cs +++ b/BackgroundServices/AutoUserDownload.cs @@ -62,5 +62,29 @@ class AutoUserDownload : BackgroundService { } if (processed > 10) Log($"Member list downloads handled for {processed} guilds."); + ConsiderGC(processed); } + + #region Manual garbage collection + private static readonly object _mgcTrackLock = new(); + private static int _mgcProcessedSinceLast = 0; + + // Downloading user information adds up memory-wise, particularly within the + // Gen 2 collection. Here we attempt to balance not calling the GC too much + // while also avoiding dying to otherwise inevitable excessive memory use. + private static void ConsiderGC(int processed) { + const int CallGcAfterProcessingAmt = 1500; + bool trigger; + lock (_mgcTrackLock) { + _mgcProcessedSinceLast += processed; + trigger = _mgcProcessedSinceLast > CallGcAfterProcessingAmt; + if (trigger) _mgcProcessedSinceLast = 0; + } + if (trigger) { + Program.Log(nameof(AutoUserDownload), "Invoking garbage collection..."); + GC.Collect(2, GCCollectionMode.Forced, true, true); + Program.Log(nameof(AutoUserDownload), "Complete."); + } + } + #endregion } From ac1b4aec3209534b30ee497bf7cbd463c351d488 Mon Sep 17 00:00:00 2001 From: Noi Date: Tue, 4 Jul 2023 14:13:44 -0700 Subject: [PATCH 3/7] Shorten library exception reporting --- ShardInstance.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ShardInstance.cs b/ShardInstance.cs index 8cd19d3..62ac25b 100644 --- a/ShardInstance.cs +++ b/ShardInstance.cs @@ -94,7 +94,7 @@ public sealed class ShardInstance : IDisposable { return Task.CompletedTask; } - Log("Discord.Net exception", arg.Exception.ToString()); + Log("Discord.Net exception", $"{arg.Exception.GetType().FullName}: {arg.Exception.Message}"); } return Task.CompletedTask; From 21cbadb355647addc292e585b2d8138d2ab76c3f Mon Sep 17 00:00:00 2001 From: Noi Date: Tue, 4 Jul 2023 15:24:35 -0700 Subject: [PATCH 4/7] Retry skipped guilds after reconnection --- BackgroundServices/AutoUserDownload.cs | 43 +++++++++++++++----------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/BackgroundServices/AutoUserDownload.cs b/BackgroundServices/AutoUserDownload.cs index a396f03..c43f8f6 100644 --- a/BackgroundServices/AutoUserDownload.cs +++ b/BackgroundServices/AutoUserDownload.cs @@ -6,10 +6,18 @@ namespace BirthdayBot.BackgroundServices; /// Proactively fills the user cache for guilds in which any birthday data already exists. /// class AutoUserDownload : BackgroundService { - public AutoUserDownload(ShardInstance instance) : base(instance) { } + private static readonly TimeSpan RequestTimeout = ShardManager.DeadShardThreshold / 3; - private static readonly HashSet _failedDownloads = new(); - private static readonly TimeSpan _singleDlTimeout = ShardManager.DeadShardThreshold / 3; + private readonly HashSet _skippedGuilds = new(); + + public AutoUserDownload(ShardInstance instance) : base(instance) + => Shard.DiscordClient.Disconnected += OnDisconnect; + ~AutoUserDownload() => Shard.DiscordClient.Disconnected -= OnDisconnect; + + private Task OnDisconnect(Exception ex) { + _skippedGuilds.Clear(); + return Task.CompletedTask; + } public override async Task OnTick(int tickCount, CancellationToken token) { // Take action if a guild's cache is incomplete... @@ -22,13 +30,11 @@ class AutoUserDownload : BackgroundService { try { await ConcurrentSemaphore.WaitAsync(token); using var db = new BotDatabaseContext(); - lock (_failedDownloads) - mustFetch = db.UserEntries.AsNoTracking() - .Where(e => incompleteCaches.Contains(e.GuildId)) - .Select(e => e.GuildId) - .Distinct() - .Where(e => !_failedDownloads.Contains(e)) - .ToList(); + mustFetch = db.UserEntries.AsNoTracking() + .Where(e => incompleteCaches.Contains(e.GuildId)) + .Select(e => e.GuildId) + .Where(e => !_skippedGuilds.Contains(e)) + .ToHashSet(); } finally { try { ConcurrentSemaphore.Release(); @@ -38,27 +44,28 @@ class AutoUserDownload : BackgroundService { var processed = 0; var processStartTime = DateTimeOffset.UtcNow; foreach (var item in mustFetch) { - // May cause a disconnect in certain situations. Make no further attempts until the next pass if it happens. + // Take break from processing to avoid getting killed by ShardManager + if (DateTimeOffset.UtcNow - processStartTime > RequestTimeout) break; + + // We're useless if not connected if (Shard.DiscordClient.ConnectionState != ConnectionState.Connected) break; var guild = Shard.DiscordClient.GetGuild(item); if (guild == null) continue; // A guild disappeared...? - await Task.Delay(200, CancellationToken.None); // Delay a bit (reduces the possibility of hanging, somehow). processed++; + + await Task.Delay(200, CancellationToken.None); // Delay a bit (reduces the possibility of hanging, somehow). var dl = guild.DownloadUsersAsync(); - dl.Wait((int)_singleDlTimeout.TotalMilliseconds / 2, token); + dl.Wait((int)RequestTimeout.TotalMilliseconds / 2, token); if (dl.IsFaulted) { Log("Exception thrown by download task: " + dl.Exception); break; } else if (!dl.IsCompletedSuccessfully) { - Log($"Task for guild {guild.Id} is unresponsive. Skipping guild. Members: {guild.MemberCount}. Name: {guild.Name}."); - lock (_failedDownloads) _failedDownloads.Add(guild.Id); + Log($"Task unresponsive, will skip (ID {guild.Id}, with {guild.MemberCount} members)."); + _skippedGuilds.Add(guild.Id); continue; } - - // Prevent unnecessary disconnections by ShardManager if we're taking too long - if (DateTimeOffset.UtcNow - processStartTime > _singleDlTimeout) break; } if (processed > 10) Log($"Member list downloads handled for {processed} guilds."); From b87602f8f68f207f0f752219eb5cd5e8dab4ea55 Mon Sep 17 00:00:00 2001 From: Noi Date: Mon, 4 Sep 2023 15:14:20 -0700 Subject: [PATCH 5/7] Greatly reduce frequency of stale entry removal --- BackgroundServices/DataRetention.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/BackgroundServices/DataRetention.cs b/BackgroundServices/DataRetention.cs index 1ae8ec4..1156e48 100644 --- a/BackgroundServices/DataRetention.cs +++ b/BackgroundServices/DataRetention.cs @@ -14,11 +14,11 @@ class DataRetention : BackgroundService { const int StaleUserThreashold = 360; public DataRetention(ShardInstance instance) : base(instance) { - ProcessInterval = 5400 / Shard.Config.BackgroundInterval; // Process about once per hour and a half + ProcessInterval = 21600 / Shard.Config.BackgroundInterval; // Process about once per six hours } public override async Task OnTick(int tickCount, CancellationToken token) { - // On each tick, run only a set group of guilds, each group still processed every ProcessInterval ticks. + // Run only a subset of shards each time, each running every ProcessInterval ticks. if ((tickCount + Shard.ShardId) % ProcessInterval != 0) return; try { From 1aaca8545e6c82b76e910e245d909d17c5b02ee9 Mon Sep 17 00:00:00 2001 From: Noi Date: Mon, 4 Sep 2023 15:37:52 -0700 Subject: [PATCH 6/7] Extremely minor refactoring --- BackgroundServices/BirthdayRoleUpdate.cs | 40 ++++++++++++------------ 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/BackgroundServices/BirthdayRoleUpdate.cs b/BackgroundServices/BirthdayRoleUpdate.cs index d3c5bc2..ec11c1a 100644 --- a/BackgroundServices/BirthdayRoleUpdate.cs +++ b/BackgroundServices/BirthdayRoleUpdate.cs @@ -15,8 +15,8 @@ class BirthdayRoleUpdate : BackgroundService { /// public override async Task OnTick(int tickCount, CancellationToken token) { try { - await ConcurrentSemaphore.WaitAsync(token); - await ProcessBirthdaysAsync(token); + await ConcurrentSemaphore.WaitAsync(token).ConfigureAwait(false); + await ProcessBirthdaysAsync(token).ConfigureAwait(false); } finally { try { ConcurrentSemaphore.Release(); @@ -25,7 +25,7 @@ class BirthdayRoleUpdate : BackgroundService { } private async Task ProcessBirthdaysAsync(CancellationToken token) { - // For database efficiency, fetch all database information at once before proceeding + // For database efficiency, fetch all pertinent 'global' database information at once before proceeding using var db = new BotDatabaseContext(); var shardGuilds = Shard.DiscordClient.Guilds.Select(g => g.Id).ToHashSet(); var presentGuildSettings = db.GuildConfigurations.Where(s => shardGuilds.Contains(s.GuildId)); @@ -39,35 +39,35 @@ class BirthdayRoleUpdate : BackgroundService { // Check task cancellation here. Processing during a single guild is never interrupted. if (token.IsCancellationRequested) throw new TaskCanceledException(); - if (Shard.DiscordClient.ConnectionState != ConnectionState.Connected) { - Log("Client is not connected. Stopping early."); - return; - } + // Stop if we've disconnected. + if (Shard.DiscordClient.ConnectionState != ConnectionState.Connected) break; try { // Verify that role settings and permissions are usable - SocketRole? role = guild.GetRole((ulong)(settings.BirthdayRole ?? 0)); - if (role == null - || !guild.CurrentUser.GuildPermissions.ManageRoles - || role.Position >= guild.CurrentUser.Hierarchy) continue; + SocketRole? role = guild.GetRole(settings.BirthdayRole ?? 0); + if (role == null) continue; // Role not set. + if (!guild.CurrentUser.GuildPermissions.ManageRoles || role.Position >= guild.CurrentUser.Hierarchy) { + // Quit this guild if insufficient role permissions. + continue; + } if (role.IsEveryone || role.IsManaged) { // Invalid role was configured. Clear the setting and quit. settings.BirthdayRole = null; db.Update(settings); - await db.SaveChangesAsync(CancellationToken.None); + await db.SaveChangesAsync(CancellationToken.None).ConfigureAwait(false); continue; } // Load up user configs and begin processing birthdays - await db.Entry(settings).Collection(t => t.UserEntries).LoadAsync(CancellationToken.None); + await db.Entry(settings).Collection(t => t.UserEntries).LoadAsync(CancellationToken.None).ConfigureAwait(false); var birthdays = GetGuildCurrentBirthdays(settings.UserEntries, settings.GuildTimeZone); // Add or remove roles as appropriate - var announcementList = await UpdateGuildBirthdayRoles(guild, role, birthdays); + var announcementList = await UpdateGuildBirthdayRoles(guild, role, birthdays).ConfigureAwait(false); // Process birthday announcement if (announcementList.Any()) { - await AnnounceBirthdaysAsync(settings, guild, announcementList); + await AnnounceBirthdaysAsync(settings, guild, announcementList).ConfigureAwait(false); } } catch (Exception ex) { // Catch all exceptions per-guild but continue processing, throw at end. @@ -93,9 +93,9 @@ class BirthdayRoleUpdate : BackgroundService { var checkNow = SystemClock.Instance.GetCurrentInstant().InZone(tz); // Special case: If user's birthday is 29-Feb and it's currently not a leap year, check against 1-Mar if (!DateTime.IsLeapYear(checkNow.Year) && record.BirthMonth == 2 && record.BirthDay == 29) { - if (checkNow.Month == 3 && checkNow.Day == 1) birthdayUsers.Add((ulong)record.UserId); + if (checkNow.Month == 3 && checkNow.Day == 1) birthdayUsers.Add(record.UserId); } else if (record.BirthMonth == checkNow.Month && record.BirthDay== checkNow.Day) { - birthdayUsers.Add((ulong)record.UserId); + birthdayUsers.Add(record.UserId); } } @@ -120,14 +120,14 @@ class BirthdayRoleUpdate : BackgroundService { else no_ops.Add(user.Id); } foreach (var user in removals) { - await user.RemoveRoleAsync(r); + await user.RemoveRoleAsync(r).ConfigureAwait(false); } foreach (var target in toApply) { if (no_ops.Contains(target)) continue; var user = g.GetUser(target); if (user == null) continue; // User existing in database but not in guild - await user.AddRoleAsync(r); + await user.AddRoleAsync(r).ConfigureAwait(false); additions.Add(user); } } catch (Discord.Net.HttpException ex) @@ -144,7 +144,7 @@ class BirthdayRoleUpdate : BackgroundService { /// Attempts to send an announcement message. /// internal static async Task AnnounceBirthdaysAsync(GuildConfig settings, SocketGuild g, IEnumerable names) { - var c = g.GetTextChannel((ulong)(settings.AnnouncementChannel ?? 0)); + var c = g.GetTextChannel(settings.AnnouncementChannel ?? 0); if (c == null) return; if (!c.Guild.CurrentUser.GetPermissions(c).SendMessages) return; From b9b057e1f1b98e0c35b423ec7dc9066884f994bb Mon Sep 17 00:00:00 2001 From: Noi Date: Mon, 4 Sep 2023 16:15:48 -0700 Subject: [PATCH 7/7] Remove extraneous log messages In particular, these changes filter out the reporting of any errors occurring during the bot shutdown process. --- BackgroundServices/AutoUserDownload.cs | 6 +++++- ShardInstance.cs | 1 + ShardManager.cs | 2 -- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/BackgroundServices/AutoUserDownload.cs b/BackgroundServices/AutoUserDownload.cs index c43f8f6..a9f8e15 100644 --- a/BackgroundServices/AutoUserDownload.cs +++ b/BackgroundServices/AutoUserDownload.cs @@ -57,7 +57,11 @@ class AutoUserDownload : BackgroundService { await Task.Delay(200, CancellationToken.None); // Delay a bit (reduces the possibility of hanging, somehow). var dl = guild.DownloadUsersAsync(); - dl.Wait((int)RequestTimeout.TotalMilliseconds / 2, token); + try { + dl.Wait((int)RequestTimeout.TotalMilliseconds / 2, token); + } catch (Exception) { } + if (token.IsCancellationRequested) return; // Skip all reporting, error logging on cancellation + if (dl.IsFaulted) { Log("Exception thrown by download task: " + dl.Exception); break; diff --git a/ShardInstance.cs b/ShardInstance.cs index 62ac25b..c05000d 100644 --- a/ShardInstance.cs +++ b/ShardInstance.cs @@ -94,6 +94,7 @@ public sealed class ShardInstance : IDisposable { return Task.CompletedTask; } + if (arg.Exception is TaskCanceledException) return Task.CompletedTask; // We don't ever need to know these... Log("Discord.Net exception", $"{arg.Exception.GetType().FullName}: {arg.Exception.Message}"); } diff --git a/ShardManager.cs b/ShardManager.cs index 5fe1abd..bc9223e 100644 --- a/ShardManager.cs +++ b/ShardManager.cs @@ -61,8 +61,6 @@ class ShardManager : IDisposable { if (!Task.WhenAll(shardDisposes).Wait(30000)) { Log("Warning: Not all shards terminated cleanly after 30 seconds. Continuing..."); } - - Log($"Uptime: {Program.BotUptime}"); } private void Log(string message) => Program.Log(nameof(ShardManager), message);