Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't clear Logger.NewEntry handlers when flushing #6191

Merged
merged 4 commits into from Feb 22, 2024

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Feb 19, 2024

Instead, NewEntry is cleared only when the GameHost is disposing.

This allows using Logger.Flush() when exporting logs from osu!, the specific use case is to log global statistics:

GlobalStatistics.OutputToLog();
Logger.Flush();

// create the compressed-logs.zip etc.

Clearing NewEntry when exporting logs is undesirable.

Instead, clearing `NewEntry` is done only when the `GameHost` is disposing.
@Susko3 Susko3 changed the title Don't clear Logger.NewEntry when flushing Don't clear Logger.NewEntry handlers when flushing Feb 19, 2024
frenzibyte
frenzibyte previously approved these changes Feb 19, 2024
@peppy peppy self-requested a review February 20, 2024 03:08
/// Pause execution until all logger writes have completed and file handles have been closed.
/// This will also unbind all handlers bound to <see cref="NewEntry"/>.
/// </summary>
public static void Dispose()
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static Dispose feels very wrong to me.

diff --git a/osu.Framework/Logging/Logger.cs b/osu.Framework/Logging/Logger.cs
index 69be3f974..c997ea80b 100644
--- a/osu.Framework/Logging/Logger.cs
+++ b/osu.Framework/Logging/Logger.cs
@@ -479,10 +479,9 @@ public static void Flush()
         }
 
         /// <summary>
-        /// Pause execution until all logger writes have completed and file handles have been closed.
-        /// This will also unbind all handlers bound to <see cref="NewEntry"/>.
+        /// Perform a <see cref="Flush"/> and unbind all events in preparation for game host shutdown.
         /// </summary>
-        public static void Dispose()
+        internal static void FlushForShutdown()
         {
             Flush();
             NewEntry = null;
diff --git a/osu.Framework/Platform/GameHost.cs b/osu.Framework/Platform/GameHost.cs
index e6b4c42ba..09b132787 100644
--- a/osu.Framework/Platform/GameHost.cs
+++ b/osu.Framework/Platform/GameHost.cs
@@ -396,7 +396,7 @@ private void abortExecutionFromException(object sender, Exception exception, boo
             // In the case of an unhandled exception, it's feasible that the disposal flow for `GameHost` doesn't run.
             // This can result in the exception not being logged (or being partially logged) due to the logger running asynchronously.
             // We force flushing the logger here to ensure logging completes (and also unbind in the process since we're aborting execution from here).
-            Logger.Dispose();
+            Logger.FlushForShutdown();
 
             var captured = ExceptionDispatchInfo.Capture(exception);
             var thrownEvent = new ManualResetEventSlim(false);
@@ -1391,7 +1391,7 @@ protected virtual void Dispose(bool disposing)
             Window?.Dispose();
 
             LoadingComponentsLogger.LogAndFlush();
-            Logger.Dispose();
+            Logger.FlushForShutdown();
         }
 
         public void Dispose()

@frenzibyte frenzibyte merged commit 0d38eac into ppy:master Feb 22, 2024
13 checks passed
@Susko3 Susko3 deleted the dont-clear-NewEntry-on-flush branch February 22, 2024 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants