diff --git a/build/check-csharp-ApplicationExit.sh b/build/check-csharp-ApplicationExit.sh
new file mode 100644
index 000000000000..9337997ff043
--- /dev/null
+++ b/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
diff --git a/src/BloomBrowserUI/package.json b/src/BloomBrowserUI/package.json
index de8646cfabfe..f13b68ddff07 100644
--- a/src/BloomBrowserUI/package.json
+++ b/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."
}
},
diff --git a/src/BloomExe/FatalExceptionHandler.cs b/src/BloomExe/FatalExceptionHandler.cs
index 997e9ef8106e..0a9ad5a079f3 100644
--- a/src/BloomExe/FatalExceptionHandler.cs
+++ b/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
}
diff --git a/src/BloomExe/InstallerSupport.cs b/src/BloomExe/InstallerSupport.cs
index a9b5f739461e..ff59c84ddb67 100644
--- a/src/BloomExe/InstallerSupport.cs
+++ b/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;
}
diff --git a/src/BloomExe/MiscUI/BrowserProgressDialog.cs b/src/BloomExe/MiscUI/BrowserProgressDialog.cs
index 0a4e1c867393..9f705b42b9bf 100644
--- a/src/BloomExe/MiscUI/BrowserProgressDialog.cs
+++ b/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();
diff --git a/src/BloomExe/Program.cs b/src/BloomExe/Program.cs
index f8d131d83422..ccc77f026320 100644
--- a/src/BloomExe/Program.cs
+++ b/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;
}
+ ///
+ /// Normally only used as Main() cleans up on exit.
+ /// Also in forced shutdown on timeout.
+ ///
+ 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
/// 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.
- /// true if we switched collections. (However, in this case we may call Application.Exit(), so the
+ /// true if we switched collections. (However, in this case we may exit the application, so the
/// caller shouldn't do anything unnecessary.)
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()
diff --git a/src/BloomExe/ProgramExit.cs b/src/BloomExe/ProgramExit.cs
new file mode 100644
index 000000000000..494949de20e5
--- /dev/null
+++ b/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;
+
+///
+/// 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.
+///
+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();
+ }
+}
diff --git a/src/BloomExe/ProjectContext.cs b/src/BloomExe/ProjectContext.cs
index 86a0c55c1b70..ae7aba6816bd 100644
--- a/src/BloomExe/ProjectContext.cs
+++ b/src/BloomExe/ProjectContext.cs
@@ -374,7 +374,7 @@ IContainer parentContainer
MessageBoxButtons.OK,
MessageBoxIcon.Error
);
- Application.Exit();
+ ProgramExit.Exit();
}
var server = parentContainer.Resolve();
diff --git a/src/BloomExe/web/BloomServer.cs b/src/BloomExe/web/BloomServer.cs
index 5b223a39a474..94daca5322cd 100644
--- a/src/BloomExe/web/BloomServer.cs
+++ b/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;
}
+ ///
+ /// Close the listener and free up the port. Normally called only by Dispose.
+ /// Also used when normal shutdown times out.
+ ///
+ 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
}
diff --git a/src/BloomExe/web/BloomWebSocketServer.cs b/src/BloomExe/web/BloomWebSocketServer.cs
index 045a098f105d..99d9d2770939 100644
--- a/src/BloomExe/web/BloomWebSocketServer.cs
+++ b/src/BloomExe/web/BloomWebSocketServer.cs
@@ -161,7 +161,7 @@ WebSocketServer server
Environment.NewLine,
server.Port
);
- Application.Exit();
+ ProgramExit.Exit();
}
///