Prevent zombie Blooms and restore trying many ports (BL-15551)

build/check-csharp-ApplicationExit.sh

@@ -0,0 +1,17 @@
+#!/bin/sh
+# This script is run by Husky from src/BloomBrowserUI/package.json. We need to get to
+# the root of the git repository, which is one level up from where this script lives.
+cd $(dirname $0)/..
+echo Checking for calling Application.Exit rather than Program.Exit.
+# 1) Get the list of C# files that have been staged for commit (added or modified)
+# 2) Screen out the one files that is allowed to use Application.Exit
+# 3) Screen out all the test code since we don't need to worry about zombie processes there
+# 4) For any remaining files, look for possible uses of Application.Exit.
+# 5) If anything is found (grep returns 0), then we stop everything and complain.
+git status --porcelain=v1 --untracked=no --ignored=no --no-renames | grep '^[AM]' | cut -c4- | grep '\.cs$' | \
+  grep -v 'src/BloomExe/ProgramExit.cs$' | \
+  grep -v 'src/BloomTests/' | \
+  xargs grep -H 'Application.Exit'
+status=$?
+# if grep finds instances of unwanted calls, then stop the commit.
+[ $status -eq 0 ] && exit 1 || exit 0

src/BloomBrowserUI/package.json

@@ -262,7 +262,7 @@
     },
     "husky": {
         "hooks": {
-            "pre-commit": "pretty-quick --staged && lint-staged --no-stash && ../../build/check-csharp-robustfile.sh && ../../build/check-csharp-xmlclasses.sh && ../../build/run-csharpier.sh",
+            "pre-commit": "pretty-quick --staged && lint-staged --no-stash && ../../build/check-csharp-robustfile.sh && ../../build/check-csharp-xmlclasses.sh && ../../build/check-csharp-ApplicationExit.sh &&../../build/run-csharpier.sh",
             "pre-commit-comments": "// The '--no-stash' option makes linting much faster and is safe since our lint-staged doesn't currently modify any files. If it did modify files, you could still use --no-stash but there are some corner cases where you'd need to manually resolve conflicts."
         }
     },

src/BloomExe/FatalExceptionHandler.cs

@@ -82,7 +82,7 @@ protected void HandleTopLevelError(object sender, ThreadExceptionEventArgs e)
             {
                 //Are we inside a Application.Run() statement?
                 if (Application.MessageLoop)
-                    Application.Exit();
+                    ProgramExit.Exit();
                 else
                     Environment.Exit(1); //the 1 here is just non-zero
             }

src/BloomExe/InstallerSupport.cs

@@ -217,7 +217,7 @@ internal static void MakeBloomRegistryEntries(string[] programArgs)
                 );
                 ProblemReportApi.ShowProblemDialog(null, exception, "", "fatal");
                 // Not sure these lines are reachable. Just making sure.
-                Application.Exit();
+                ProgramExit.Exit();
                 return;
             }
 

src/BloomExe/MiscUI/BrowserProgressDialog.cs

@@ -120,7 +120,7 @@ public static void DoWorkWithProgressDialog(
                 dlg.ShowDialog(owner); // returns when dialog closed
                 if (progress.HasFatalProblemBeenReported)
                 {
-                    Application.Exit();
+                    ProgramExit.Exit();
                 }
 
                 ProgressDialogApi.SetCancelHandler(null);
@@ -260,7 +260,7 @@ public static void HandleProgressDialogClosed(ApiRequest request)
         {
             if (_progress?.HasFatalProblemBeenReported ?? false)
             {
-                Application.Exit();
+                ProgramExit.Exit();
             }
 
             _doWhenProgressDialogCloses?.Invoke();

src/BloomExe/Program.cs

@@ -11,6 +11,7 @@
 using System.Threading;
 using System.Threading.Tasks;
 using System.Windows.Forms;
+using Bloom.Api;
 using Bloom.CLI;
 using Bloom.Collection;
 using Bloom.Collection.BloomPack;
@@ -103,7 +104,7 @@ static class Program
         static int Main(string[] args1)
         {
             // AttachConsole(-1);	// Enable this to allow Console.Out.WriteLine to be viewable (must run Bloom from terminal, AFAIK)
-            bool gotUniqueToken = false;
+            _gotUniqueToken = false;
             _uiThreadId = Thread.CurrentThread.ManagedThreadId;
             Logger.Init();
             // Configure TempFile to create temp files with a "bloom" prefix so we can
@@ -404,7 +405,7 @@ static int Main(string[] args1)
                                         BloomMessageBox.ShowInfo(msg);
                                         return 1;
                                     }
-                                    gotUniqueToken = true;
+                                    _gotUniqueToken = true;
 
                                     if (
                                         projectContext.TeamCollectionManager.CurrentCollection
@@ -470,7 +471,7 @@ static int Main(string[] args1)
                         // and now we need to get the lock as usual before going on to load the new collection.
                         if (!UniqueToken.AcquireToken(_mutexId, "Bloom"))
                             return 1;
-                        gotUniqueToken = true;
+                        _gotUniqueToken = true;
                     }
                     else if (IsBloomBookOrder(args))
                     {
@@ -480,7 +481,7 @@ static int Main(string[] args1)
                         {
                             // No other instance isrunning. Start up normally (and show the book just downloaded).
                             // See https://silbloom.myjetbrains.com/youtrack/issue/BL-3822
-                            gotUniqueToken = true;
+                            _gotUniqueToken = true;
                         }
                         else if (forEdit)
                         {
@@ -501,7 +502,7 @@ static int Main(string[] args1)
                             // control key is held down so allow second instance to run; note that we're deliberately in this state
                             if (UniqueToken.AcquireTokenQuietly(_mutexId))
                             {
-                                gotUniqueToken = true;
+                                _gotUniqueToken = true;
                             }
                             else
                             {
@@ -511,7 +512,7 @@ static int Main(string[] args1)
                         else if (UniqueToken.AcquireToken(_mutexId, "Bloom"))
                         {
                             // No other instance is running. We own the token and should release it on quitting.
-                            gotUniqueToken = true;
+                            _gotUniqueToken = true;
                         }
                         else
                         {
@@ -642,8 +643,7 @@ static int Main(string[] args1)
             {
                 // Check memory one final time for the benefit of developers.  The user won't see anything.
                 //Bloom.Utils.MemoryManagement.CheckMemory(true, "Bloom finished and exiting", false);
-                if (gotUniqueToken)
-                    UniqueToken.ReleaseToken();
+                ReleaseBloomToken();
 
                 _sentry?.Dispose();
                 // In a debug build we want to be able to see if we're leaving garbage around. (Note: this doesn't seem to be working.)
@@ -652,7 +652,7 @@ static int Main(string[] args1)
                 // - we might delete something in use by the instance that has the token
                 // - we would delete the token itself (since _mutexid and NamePrefix happen to be the same),
                 // allowing a later duplicate process to start normally.
-                if (gotUniqueToken)
+                if (_gotUniqueToken)
                     TempFile.CleanupTempFolder();
 #endif
             }
@@ -661,6 +661,16 @@ static int Main(string[] args1)
             return 0;
         }
 
+        /// <summary>
+        /// Normally only used as Main() cleans up on exit.
+        /// Also in forced shutdown on timeout.
+        /// </summary>
+        public static void ReleaseBloomToken()
+        {
+            if (_gotUniqueToken)
+                UniqueToken.ReleaseToken();
+        }
+
         private static bool IsWebviewMissingOrTooOld()
         {
             string version;
@@ -909,7 +919,7 @@ public static void RestartBloom(bool hardExit, string args = null)
                 Thread.Sleep(2000);
                 if (hardExit || _projectContext?.ProjectWindow == null)
                 {
-                    Application.Exit();
+                    ProgramExit.Exit();
                 }
                 else
                 {
@@ -1376,7 +1386,7 @@ private static void HandleErrorOpeningProjectWindow(Exception error, string proj
         /// <param name="formToClose">If provided, this form will be closed after choosing a
         /// collection and before opening it. Currently, this is used to close the Shell at the proper
         /// time when switching collectons.</param>
-        /// <returns>true if we switched collections. (However, in this case we may call Application.Exit(), so the
+        /// <returns>true if we switched collections. (However, in this case we may exit the application, so the
         /// caller shouldn't do anything unnecessary.)</returns>
         public static bool ChooseACollection(Shell formToClose = null)
         {
@@ -1415,7 +1425,7 @@ public static bool ChooseACollection(Shell formToClose = null)
                             // and we don't want to exit the application. Otherwise, we are in initial startup and
                             // closing the chooser should exit the application.
                             if (formToClose == null)
-                                Application.Exit();
+                                ProgramExit.Exit();
                             return false;
                         }
 
@@ -1480,11 +1490,11 @@ static void HandleProjectWindowClosed(object sender, EventArgs e)
             }
             else if (((Shell)sender).QuitForVersionUpdate)
             {
-                Application.Exit();
+                ProgramExit.Exit();
             }
             else
             {
-                Application.Exit();
+                ProgramExit.Exit();
             }
         }
 
@@ -1734,6 +1744,7 @@ private static bool IsSameActualLanguage(string code1, string code2)
 
         private static bool _errorHandlingHasBeenSetUp;
         private static IDisposable _sentry;
+        private static bool _gotUniqueToken;
 
         /// ------------------------------------------------------------------------------------
         internal static void SetUpErrorHandling()

src/BloomExe/ProgramExit.cs

@@ -0,0 +1,78 @@
+using System;
+using System.Threading;
+using System.Windows.Forms;
+using Bloom.Api;
+using SIL.Reporting;
+
+namespace Bloom;
+
+/// <summary>
+/// This class is responsible for making sure Bloom really exits when we ask it to.
+/// To ensure this, code must use ProgramExit.Exit() rather than Application.Exit() directly.
+/// We have a git hook to enforce this.
+/// </summary>
+static class ProgramExit
+{
+    private static Thread _forceShutdownThread;
+
+    public static void Exit()
+    {
+        EnsureBloomReallyQuits();
+        Application.Exit();
+    }
+
+    private static void EnsureBloomReallyQuits()
+    {
+        if (_forceShutdownThread != null)
+            return; // already started
+        // We've had a problem with Bloom failing to shutdown, leaving a zombie thread that prevents
+        // other instances from starting up. If shutdown takes more than 20 seconds, we will just
+        // forcefully exit.
+        _forceShutdownThread = new Thread(() =>
+        {
+            Thread.Sleep(20000);
+            // We hope to never get here; it means that Bloom failed to fully
+            // quit 20s after we called Application.Exit(). We will now do some
+            // minimal essential cleanup and then force an exit.
+            // It might be nice to tell the user, but since we don't know
+            // what went wrong or where we are in the shutdown process, it's not
+            // obvious what to say. I don't even see how we can reliably show a message,
+            // since we're about to force a shut down. And I don't think there's
+            // anything that will be left in a bad state that the user might need
+            // to deal with.
+            try
+            {
+                Logger.WriteEvent("Forcing Bloom to close after normal shutdown timed out.");
+            }
+            catch (Exception)
+            {
+                // We might have already shut down the logger. If we can't log it, too bad.
+            }
+
+            // These things MUST be done so that Bloom can be started again without problems.
+            // They should be very fast.
+            try
+            {
+                BloomServer._theOneInstance?.CloseListener();
+            }
+            catch (Exception)
+            {
+                // Anything that goes wrong here shouldn't prevent either trying
+                // the next cleanup or exiting.
+            }
+
+            try
+            {
+                Program.ReleaseBloomToken();
+            }
+            catch (Exception) { }
+
+            Environment.Exit(1);
+            // If that doesn't prove drastic enough, an even more forceful option is
+            // Process.GetCurrentProcess().Kill();
+        });
+        _forceShutdownThread.Priority = ThreadPriority.Highest;
+        _forceShutdownThread.IsBackground = true; // so it won't block shutdown
+        _forceShutdownThread.Start();
+    }
+}

src/BloomExe/ProjectContext.cs

@@ -374,7 +374,7 @@ IContainer parentContainer
                     MessageBoxButtons.OK,
                     MessageBoxIcon.Error
                 );
-                Application.Exit();
+                ProgramExit.Exit();
             }
 
             var server = parentContainer.Resolve<BloomServer>();

src/BloomExe/web/BloomServer.cs

@@ -1334,7 +1334,7 @@ public virtual void EnsureListening()
                 ErrorReport.NotifyUserOfProblem(GetServerStartFailureMessage());
                 Logger.WriteEvent("Error: Could not start up internal HTTP Server");
                 Analytics.ReportException(new ApplicationException("Could not start server."));
-                Application.Exit();
+                ProgramExit.Exit();
             }
 
             Logger.WriteEvent("Server will use " + ServerUrlEndingInSlash);
@@ -1387,18 +1387,9 @@ private bool AttemptToOpenPort()
 
         private bool HandleExceptionOpeningPort(Exception error)
         {
-            if (!Program.RunningUnitTests && !Program.RunningSecondInstance)
-                NonFatalProblem.Report(
-                    ModalIf.None,
-                    PassiveIf.Alpha,
-                    "Could not open " + ServerUrlEndingInSlash,
-                    "Could not start server on that port",
-                    error
-                );
-            else
-                Console.WriteLine(
-                    $"Cannot open {ServerUrlEndingInSlash}: {error.Message} ({error.GetType().Name})"
-                );
+            Console.WriteLine(
+                $"Cannot open {ServerUrlEndingInSlash}: {error.Message} ({error.GetType().Name})"
+            );
             try
             {
                 if (_listener != null)
@@ -1437,7 +1428,7 @@ private static void VerifyWeAreNowListening()
             catch (Exception error)
             {
                 ErrorReport.NotifyUserOfProblem(error, GetServerStartFailureMessage());
-                Application.Exit();
+                ProgramExit.Exit();
             }
 
             ServerIsListening = true;
@@ -2216,19 +2207,7 @@ var kvp in _workers.Where(kvp =>
                             }
                         }
 
-                        // stop listening for incoming http requests
-                        Debug.Assert(_listener.IsListening);
-                        if (_listener.IsListening)
-                        {
-                            //In BL-3290, a user quitely failed here each time he exited Bloom, with a Cannot access a disposed object.
-                            //according to http://stackoverflow.com/questions/11164919/why-httplistener-start-method-dispose-stuff-on-exception,
-                            //it's actually just responding to being closed, not disposed.
-                            //I don't know *why* for that user the listener was already stopped.
-                            _listener.Stop();
-                        }
-                        //if we keep getting that exception, we could move the Close() into the previous block
-                        _listener.Close();
-                        _listener = null;
+                        CloseListener();
                     }
                     if (_cache != null)
                     {
@@ -2252,6 +2231,29 @@ var kvp in _workers.Where(kvp =>
             IsDisposed = true;
         }
 
+        /// <summary>
+        /// Close the listener and free up the port. Normally called only by Dispose.
+        /// Also used when normal shutdown times out.
+        /// </summary>
+        public void CloseListener()
+        {
+            if (_listener == null)
+                return; // probably called from shutdown timer, and normal shutdown got this far
+            // stop listening for incoming http requests
+            Debug.Assert(_listener.IsListening);
+            if (_listener.IsListening)
+            {
+                //In BL-3290, a user quitely failed here each time he exited Bloom, with a Cannot access a disposed object.
+                //according to http://stackoverflow.com/questions/11164919/why-httplistener-start-method-dispose-stuff-on-exception,
+                //it's actually just responding to being closed, not disposed.
+                //I don't know *why* for that user the listener was already stopped.
+                _listener.Stop();
+            }
+            //if we keep getting that exception, we could move the Close() into the previous block
+            _listener.Close();
+            _listener = null;
+        }
+
         #endregion
     }
 

src/BloomExe/web/BloomWebSocketServer.cs

@@ -161,7 +161,7 @@ WebSocketServer server
                 Environment.NewLine,
                 server.Port
             );
-            Application.Exit();
+            ProgramExit.Exit();
         }
 
         /// <summary>