diff --git a/src/BloomExe/ApplicationContainer.cs b/src/BloomExe/ApplicationContainer.cs index a20bb7cfae8a..b07afac403ed 100644 --- a/src/BloomExe/ApplicationContainer.cs +++ b/src/BloomExe/ApplicationContainer.cs @@ -81,7 +81,25 @@ public ApplicationContainer() _container = builder.Build(); - Application.ApplicationExit += OnApplicationExit; + // Only listen for the application exiting when there IS an application in the GUI sense. + // A command-line verb never calls Application.Run, so the only WinForms message loop in the + // process belongs to some worker -- currently the dedicated thread of the off-screen browser + // PublishHelper uses for page checks, created and disposed once per batch. When that loop + // ends, WinForms decides the application is exiting and raises ApplicationExit, mid-run. Acting + // on that disposed this container, the parent scope of the still-in-use ProjectContext, so the + // next artifact step died with ObjectDisposedException (BL-16668). Not subscribing is safe + // because in that flow the container's lifetime is already bounded by the `using` blocks in the + // CLI command handlers, and the process exits as soon as those unwind. + // + // One knock-on worth knowing: OnApplicationExit is the only caller of + // Program.FinishLocalizationHarvesting(), so a command-line verb no longer runs it. That is + // #if DEBUG code which does nothing unless LocalizationManager.IgnoreExistingEnglishTranslationFiles + // is set, so release CLI runs are unaffected -- but a DEBUG localization-harvesting run driven + // through a CLI verb would no longer merge the English translation files. If we ever want that, + // call it from the CLI path explicitly rather than by leaning on a shutdown event that is not + // really telling us the application is shutting down. + if (!Program.RunningInConsoleMode) + Application.ApplicationExit += OnApplicationExit; // Register the API Handlers that are global to the application (not dependent on knowing a particular project). // Note: it is is a work in progress to transfer more API handlers from ProjectContext to here. @@ -98,6 +116,10 @@ public ApplicationContainer() server.ApiHandler.RecordApplicationLevelHandlers(); } + /// + /// The application is really shutting down, so tear the container down. Only ever subscribed + /// when Bloom is running as a GUI application -- see the constructor for why. + /// private void OnApplicationExit(object sender, EventArgs e) { Application.ApplicationExit -= OnApplicationExit; diff --git a/src/BloomExe/CLI/CreateArtifactsCommand.cs b/src/BloomExe/CLI/CreateArtifactsCommand.cs index 6791cab6f360..2b0ffab4884e 100644 --- a/src/BloomExe/CLI/CreateArtifactsCommand.cs +++ b/src/BloomExe/CLI/CreateArtifactsCommand.cs @@ -195,7 +195,15 @@ private static CreateArtifactsExitCode CreateArtifacts(CreateArtifactsParameters catch (Exception ex) { Console.WriteLine(ex.ToString()); - exitCode = CreateArtifactsExitCode.EpubException; + // |=, not =: these are [Flags] values and every other step here accumulates. + // A plain assignment discarded whatever had already been recorded -- most + // notably BookHtmlNotFound from the bloomdigital step just above -- so the + // harvester was told only "EpubException" and lost the fact that the book's + // HTML was also wrong. That is the combination the harvester actually hits, + // since it always asks for both artifacts. Safe to accumulate from this + // worker: the main thread is blocked on countdownEvent below and does not + // touch exitCode until after we signal. + exitCode |= CreateArtifactsExitCode.EpubException; } countdownEvent.Signal(); // Decrement by one }); diff --git a/src/BloomTests/CLI/CreateArtifactsCommandTests.cs b/src/BloomTests/CLI/CreateArtifactsCommandTests.cs index 6ceb4c15d0a8..15964c1d8e93 100644 --- a/src/BloomTests/CLI/CreateArtifactsCommandTests.cs +++ b/src/BloomTests/CLI/CreateArtifactsCommandTests.cs @@ -11,11 +11,23 @@ namespace BloomTests.CLI [TestFixture] public class CreateArtifactsCommandTests { + [SetUp] + public void SetUp() + { + // Program.Main sets this for a real command-line run, before it dispatches to the verb. These + // tests call HandleInternal directly and so bypass Main, which means they have to stand in for + // it — exactly as TearDown below already does for RunningHarvesterMode. ApplicationContainer + // reads this flag to know there is no GUI application whose exit it should listen for; without + // it set, the container tears itself down mid-run and the epub step fails (BL-16668). + Program.RunningInConsoleMode = true; + } + [TearDown] public void TearDown() { // Without this, subsequent tests will fail because they think the harvester is still running. Program.RunningHarvesterMode = false; + Program.RunningInConsoleMode = false; } [Test] @@ -238,6 +250,99 @@ public void CreateArtifacts_LegacyBookWithInvalidXmatter_ReportsLegacyBookCannot } } + // The harvester never asks for just one artifact: it passes --bloomdOutputPath, + // --bloomDigitalOutputPath AND --epubOutputPath in a single createArtifacts run. That + // combination is what BL-16668 broke, and why every other test here missed it. Making the + // bloomdigital spins up (and then tears down) PublishHelper's off-screen browser, whose + // dedicated thread runs the only WinForms message loop a CLI process has. Ending that loop + // made WinForms raise Application.ApplicationExit, which disposed the ApplicationContainer -- + // the parent scope of our still-in-use ProjectContext -- so the epub step, which runs + // afterwards, died with ObjectDisposedException resolving ProjectContext.BookServer and + // createArtifacts returned EpubException. A test that requests a single artifact cannot + // catch that, because nothing runs after the premature disposal. + [Test] + public void CreateArtifacts_BloomDigitalAndEpubRequestedTogether_BothCreated() + { + using ( + var testFolder = new TemporaryFolder( + "CreateArtifacts_BloomDigitalAndEpubRequestedTogether_BothCreated" + ) + ) + { + var collectionFolderPath = testFolder.Combine("collection"); + + var bookFolderPath = Path.Combine(collectionFolderPath, "book"); + System.IO.Directory.CreateDirectory(bookFolderPath); + var collectionFilePath = Path.Combine( + collectionFolderPath, + "collection.bloomCollection" + ); + var settings = new CollectionSettings(collectionFilePath); + settings.Save(); + var metaData = new BookMetaData(); + metaData.WriteToFolder(bookFolderPath); + var bookPath = System.IO.Path.Combine(bookFolderPath, "book.htm"); + System.IO.File.WriteAllText( + bookPath, + @" + +
+
+
+
+ Hello +
+
+
+
+ + " + ); + + var bloomDigitalOutputPath = Path.Combine(testFolder.FolderPath, "bloomdigital"); + var epubOutputPath = Path.Combine(testFolder.FolderPath, "epub", "book.epub"); + // Sanity check: neither artifact exists yet, so finding them later really does mean + // this run made them. + Assert.That( + Directory.Exists(bloomDigitalOutputPath), + Is.False, + "test setup: bloomdigital output should not exist before the run" + ); + Assert.That( + File.Exists(epubOutputPath), + Is.False, + "test setup: epub output should not exist before the run" + ); + + var result = CreateArtifactsCommand.HandleInternal( + new CreateArtifactsParameters() + { + BookPath = bookFolderPath, + CollectionPath = collectionFilePath, + BloomDigitalOutputPath = bloomDigitalOutputPath, + EpubOutputPath = epubOutputPath, + NoAnalytics = true, + } + ); + + Assert.That( + result, + Is.EqualTo(CreateArtifactsExitCode.Success), + "createArtifacts should succeed when both a bloomdigital and an epub are requested" + ); + Assert.That( + File.Exists(Path.Combine(bloomDigitalOutputPath, "index.htm")), + Is.True, + "the bloomdigital artifact should have been created" + ); + Assert.That( + File.Exists(epubOutputPath), + Is.True, + "the epub artifact should have been created" + ); + } + } + [Test] public void CreateArtifacts_WithJsonOutput_CreatesJsonFile() {