Changed shutdown to properly wait for sub-processes to shutdown

src/main/main.ts

@@ -21,6 +21,11 @@ import { resolveHtmlPath } from '@node/utils/util';
 import MenuBuilder from '@main/menu.model';
 import extensionHostService from '@main/services/extension-host.service';
 import networkObjectService from '@shared/services/network-object.service';
+import { wait } from '@shared/utils/util';
+
+const PROCESS_CLOSE_TIME_OUT = 2000;
+
+let isClosing = false;
 
 logger.info('Starting main');
 
@@ -137,17 +142,31 @@ app.on('window-all-closed', () => {
   // Respect the OSX convention of having the application in memory even
   // after all windows have been closed
   if (process.platform !== 'darwin') {
-    // TODO: cleanly stop the provider (close the ws or send command) - IJH 2022-02-23
-    dotnetDataProvider.kill();
-    extensionHostService.kill();
     app.quit();
   }
 });
 
-app.on('will-quit', () => {
-  // TODO: cleanly stop the provider (close the ws or send command) - IJH 2022-02-23
-  dotnetDataProvider.kill();
-  extensionHostService.kill();
+app.on('will-quit', async (e) => {
+  if (!isClosing) {
+    // Prevent closing before graceful shutdown is complete.
+    // Also, in the future, this should allow a "are you sure?" dialog to display.
+    e.preventDefault();
+    isClosing = true;
+
+    networkService.shutdown();
+    await Promise.all([
+      dotnetDataProvider.wait(PROCESS_CLOSE_TIME_OUT),
+      extensionHostService.wait(PROCESS_CLOSE_TIME_OUT),
+    ]);
+
+    // In development, the dotnet watcher was killed so we have to wait here.
+    if (process.env.NODE_ENV !== 'production') await wait(500);
+
+    app.quit();
+  } else {
+    dotnetDataProvider.kill();
+    extensionHostService.kill();
+  }
 });
 
 // #endregion

src/main/services/dotnet-data-provider.service.ts

@@ -1,12 +1,18 @@
 import { ChildProcessWithoutNullStreams, spawn } from 'child_process';
 import path from 'path';
 import logger, { formatLog } from '@shared/services/logger.service';
+import { waitForDuration } from '@shared/utils/util';
 
 /** Pretty name for the process this service manages. Used in logs */
 const DOTNET_DATA_PROVIDER_NAME = 'dotnet data provider';
 
 let dotnet: ChildProcessWithoutNullStreams | undefined;
 
+let resolveClose: (value: void | PromiseLike<void>) => void;
+const closePromise: Promise<void> = new Promise<void>((resolve) => {
+  resolveClose = resolve;
+});
+
 // log functions for inside the data provider process
 function logProcessError(message: unknown) {
   logger.error(formatLog(message?.toString() || '', DOTNET_DATA_PROVIDER_NAME, 'error'));
@@ -30,15 +36,24 @@ function killDotnetDataProvider() {
   dotnet = undefined;
 }
 
+async function waitForDotnetDataProvider(maxWaitTimeInMS: number) {
+  const didClose = await waitForDuration(async () => {
+    await closePromise;
+    return true;
+  }, maxWaitTimeInMS);
+
+  if (!didClose) killDotnetDataProvider();
+}
+
 /**
  * Starts the Dotnet Data Provider if it isn't already running.
  */
 function startDotnetDataProvider() {
   if (dotnet) return;
 
   // default values for development
-  let command = process.platform.includes('win') ? 'npm.cmd' : 'npm';
-  let args: string[] = ['run', 'start:data'];
+  let command = 'dotnet';
+  let args: string[] = ['watch', '--project', 'c-sharp/ParanextDataProvider.csproj'];
 
   if (process.env.NODE_ENV === 'production') {
     if (process.platform === 'win32') {
@@ -56,6 +71,8 @@ function startDotnetDataProvider() {
   dotnet.stderr.on('data', logProcessError);
 
   dotnet.on('close', (code, signal) => {
+    // In production, this handles the closing of the data provider. However, in development,
+    // this is handling the closing of the dotnet watcher.
     if (signal) {
       logger.info(`'close' event: dotnet data provider terminated with signal ${signal}`);
     } else {
@@ -64,11 +81,13 @@ function startDotnetDataProvider() {
     // TODO: listen for 'exit' event as well?
     // TODO: unsubscribe event listeners
     dotnet = undefined;
+    resolveClose();
   });
 }
 
 const dotnetDataProvider = {
   start: startDotnetDataProvider,
   kill: killDotnetDataProvider,
+  wait: waitForDotnetDataProvider,
 };
 export default dotnetDataProvider;

src/main/services/extension-host.service.ts

@@ -3,6 +3,7 @@
  */
 
 import logger, { formatLog } from '@shared/services/logger.service';
+import { waitForDuration } from '@shared/utils/util';
 import { ChildProcess, ChildProcessByStdio, fork, spawn } from 'child_process';
 import { app } from 'electron';
 import path from 'path';
@@ -13,6 +14,11 @@ const EXTENSION_HOST_NAME = 'extension host';
 
 let extensionHost: ChildProcess | ChildProcessByStdio<null, Readable, Readable> | undefined;
 
+let resolveClose: (value: void | PromiseLike<void>) => void;
+const closePromise: Promise<void> = new Promise<void>((resolve) => {
+  resolveClose = resolve;
+});
+
 // log functions for inside the extension host process
 function logProcessError(message: unknown) {
   logger.error(formatLog(message?.toString() || '', EXTENSION_HOST_NAME, 'error'));
@@ -21,6 +27,15 @@ function logProcessInfo(message: unknown) {
   logger.info(formatLog(message?.toString() || '', EXTENSION_HOST_NAME));
 }
 
+async function waitForExtensionHost(maxWaitTimeInMS: number) {
+  const didClose = await waitForDuration(async () => {
+    await closePromise;
+    return true;
+  }, maxWaitTimeInMS);
+
+  if (!didClose) killExtensionHost();
+}
+
 /**
  * Hard kills the extension host process.
  * TODO: add a more elegant shutdown to avoid this if we possibly can
@@ -54,10 +69,17 @@ function startExtensionHost() {
         },
       )
     : spawn(
-        process.platform.includes('win') ? 'npm.cmd' : 'npm',
-        ['run', 'start:extension-host', '--', ...sharedArgs],
+        'node',
+        [
+          'node_modules/nodemon/bin/nodemon.js',
+          '--transpile-only',
+          './src/extension-host/extension-host.ts',
+          '--',
+          ...sharedArgs,
+        ],
         {
           stdio: ['ignore', 'pipe', 'pipe'],
+          env: { ...process.env, NODE_ENV: 'development' },
         },
       );
 
@@ -79,6 +101,7 @@ function startExtensionHost() {
     // TODO: listen for 'exit' event as well?
     // TODO: unsubscribe event listeners
     extensionHost = undefined;
+    resolveClose();
   });
 }
 
@@ -88,6 +111,7 @@ function startExtensionHost() {
 const extensionHostService = {
   start: startExtensionHost,
   kill: killExtensionHost,
+  wait: waitForExtensionHost,
 };
 
 export default extensionHostService;

src/shared/services/network.service.ts

@@ -579,6 +579,11 @@ export const onDidClientDisconnect = onDidClientDisconnectEmitter.event;
 
 // #endregion
 
+/** Closes the network services gracefully */
+export const shutdown = () => {
+  connectionService.disconnect();
+};
+
 /** Sets up the NetworkService. Runs only once */
 export const initialize = () => {
   if (initializePromise) return initializePromise;

src/shared/services/web-view.service.ts

@@ -9,7 +9,7 @@ import {
   serializeRequestType,
 } from '@shared/utils/papi-util';
 import * as commandService from '@shared/services/command.service';
-import { newGuid, newNonce } from '@shared/utils/util';
+import { newGuid, newNonce, wait } from '@shared/utils/util';
 // We need the papi here to pass it into WebViews. Don't use it anywhere else in this file
 // eslint-disable-next-line import/no-cycle
 import papi from '@shared/services/papi.service';
@@ -77,11 +77,6 @@ const getWebViewPapi = (webViewId: string) => {
 
 // #endregion
 
-const sleep = (ms: number) => {
-  // eslint-disable-next-line no-promise-executor-return
-  return new Promise((resolve) => setTimeout(resolve, ms));
-};
-
 /**
  * Adds a WebView and runs all event handlers who are listening to this event
  * @param webView full html document to set as the webview iframe contents. Can be shortened to just a string
@@ -100,7 +95,7 @@ export const addWebView = async (webView: WebViewContents) => {
         .catch(async (error) => {
           succeeded = false;
           if (attemptsRemaining === 1) throw error;
-          await sleep(1000);
+          await wait(1000);
         });
 
       if (succeeded) return undefined;

src/shared/utils/util.ts

@@ -123,3 +123,24 @@ function toErrorWithMessage(maybeError: unknown): ErrorWithMessage {
 export function getErrorMessage(error: unknown) {
   return toErrorWithMessage(error).message;
 }
+
+/**
+ * Asynchronously waits for the specified number of milliseconds.
+ * (wraps setTimeout in a promise)
+ */
+export function wait(ms: number) {
+  // eslint-disable-next-line no-promise-executor-return
+  return new Promise<void>((resolve) => setTimeout(resolve, ms));
+}
+
+/**
+ * Runs the specified function and will timeout if it takes longer than the specified wait time
+ * @param fn The function to run
+ * @param maxWaitTimeInMS The maximum amount of time to wait for the function to resolve
+ * @returns Promise that resolves to the resolved value of the function or null if it
+ * ran longer than the specified wait time
+ */
+export function waitForDuration<TResult>(fn: () => Promise<TResult>, maxWaitTimeInMS: number) {
+  const timeout = wait(maxWaitTimeInMS).then(() => null);
+  return Promise.any([timeout, fn()]);
+}