Improve server start implementation and provide better error reporting

- Report error for spawn() failures
- Register callback for server exit, error and stderr to catch such
  errors
- If error happens report the error with detailed message from stderr
- Do healthCheck in 1 second intervals until it is successful or the
  overall timeout of 10s happens
- Remove unnecessary warning messages when stopping server after startup
  failures.
- Report error if trace server exits unexpectedly

Signed-off-by: Bernd Hufmann <[email protected]>

Author: bhufmann

src/trace-server.ts

@@ -13,7 +13,7 @@ const key = 'pid';
 const millis = 10000;
 const none = -1;
 const prefix = 'Trace Server';
-const suffix = ' failure or so.';
+const suffix = ' failure.';
 const SHUTDOWN_DELAY = 2000;
 
 export class TraceServer {
@@ -24,15 +24,21 @@ export class TraceServer {
         const server = spawn(this.getPath(from), this.getArgs(from));
 
         if (!server.pid) {
-            this.showError(prefix + ' startup' + suffix);
+            // Failed to spawn the child process due to errors, PID is undefined and an error is emitted.
+            await new Promise<void>(resolve => {
+                server.once('error', error => {
+                    this.showError(prefix + ' startup' + suffix + ': ' + error);
+                });
+                resolve();
+            });
             return;
         }
         this.server = server;
         await context?.workspaceState.update(key, this.server.pid);
         await this.waitFor(context);
     }
 
-    async stopOrReset(context: vscode.ExtensionContext | undefined) {
+    async stopOrReset(context: vscode.ExtensionContext | undefined, reportNotStopped: boolean = true) {
         const pid: number | undefined = context?.workspaceState.get(key);
         const not = prefix + ' not stopped as none running or owned by us.';
         if (pid === none) {
@@ -43,6 +49,7 @@ export class TraceServer {
             let id: NodeJS.Timeout;
             // recovering from workspaceState => no this.server set
             if (this.server) {
+                this.server.removeAllListeners();
                 this.server.once('exit', () => {
                     this.showStatus(false);
                     clearTimeout(id);
@@ -56,6 +63,7 @@ export class TraceServer {
                 },
                 async progress => {
                     progress.report({ message: 'stopping...' });
+                    // Remove all existing listeners to avoid unnecessary pop-ups
                     const message = prefix + ' stopping' + suffix + ' Resetting.';
                     treeKill(pid, error => {
                         if (error) {
@@ -66,7 +74,7 @@ export class TraceServer {
                     });
                 }
             );
-        } else {
+        } else if (reportNotStopped) {
             vscode.window.showWarningMessage(not);
         }
         await context?.workspaceState.update(key, none);
@@ -81,6 +89,9 @@ export class TraceServer {
             return;
         }
         if (pid) {
+            // Remove all existing listeners to avoid unnecessary pop-ups
+            this.server?.removeAllListeners();
+
             treeKill(pid);
             // Allow the treeKill to finish collecting and killing all
             // spawned processes.
@@ -160,6 +171,10 @@ export class TraceServer {
     }
 
     private async waitFor(context: vscode.ExtensionContext | undefined) {
+        if (this.server === undefined) {
+            return;
+        }
+        const server = this.server;
         await vscode.window.withProgress(
             {
                 location: vscode.ProgressLocation.Notification,
@@ -168,27 +183,63 @@ export class TraceServer {
             },
             async progress => {
                 progress.report({ message: 'starting up...' });
-                let timeout = false;
-                const timeoutId = setTimeout(() => (timeout = true), millis);
-
-                // eslint-disable-next-line no-constant-condition
-                while (true) {
-                    if (await this.isUp()) {
-                        this.showStatus(true);
-                        clearTimeout(timeoutId);
-                        break;
-                    }
-                    if (timeout) {
-                        this.showError(prefix + ' startup timed-out after ' + millis + 'ms.');
-                        await this.stopOrReset(context);
-                        break;
+                try {
+                    let timeout = false;
+                    const timeoutId = setTimeout(() => (timeout = true), millis);
+
+                    let stderr = '';
+                    server.stderr?.once('data', data => {
+                        stderr = data;
+                    });
+
+                    let errorString: string | undefined;
+                    server.once('exit', code => {
+                        errorString = 'Code: ' + code + (stderr !== '.' ? '. Error message: ' + stderr : '');
+                    });
+
+                    // eslint-disable-next-line no-constant-condition
+                    while (true) {
+                        if (await this.isUp()) {
+                            clearTimeout(timeoutId);
+                            this.showStatus(true);
+                            break;
+                        }
+
+                        // Check if trace server exited with error
+                        if (errorString !== undefined) {
+                            throw new Error(errorString);
+                        }
+
+                        // Check for overall timeout
+                        if (timeout) {
+                            await this.stopOrReset(context, false);
+                            throw new Error(prefix + ' startup timed-out after ' + millis + 'ms.');
+                        }
+
+                        // Wait before trying again
+                        await new Promise<void>(resolve => setTimeout(() => resolve(), 1000));
                     }
+                } catch (error) {
+                    this.showErrorDetailed(prefix + ' starting up' + suffix, error as Error);
+                    return;
+                } finally {
+                    // Remove all listener needed during startup
+                    server.removeAllListeners();
+                    // Add listener to catch when server exits unexpectedly
+                    server.once('exit', code => {
+                        context?.workspaceState.update(key, none);
+                        this.server = undefined;
+                        this.setStatusIfAvailable(false);
+                        vscode.window.showErrorMessage(
+                            prefix + ' exited unexpectedly' + (code !== null ? ' with code ' + code : '') + '!'
+                        );
+                    });
                 }
             }
         );
     }
 
-    private async isUp() {
+    private async isUp(): Promise<boolean> {
         const client = new TspClient(this.getServerUrl());
         const health = await client.checkHealth();
         const status = health.getModel()?.status;
@@ -198,18 +249,15 @@ export class TraceServer {
     private async showError(message: string) {
         console.error(message);
         vscode.window.showErrorMessage(message);
-        const disclaimer = ' running, despite this error.';
         const up = await this.isUp();
         if (up) {
-            vscode.window.showWarningMessage(prefix + ' is still' + disclaimer);
-        } else {
-            vscode.window.showWarningMessage(prefix + ' is not' + disclaimer);
+            vscode.window.showWarningMessage(prefix + ' is still running, despite this error.');
         }
         this.setStatusIfAvailable(up);
     }
 
     private showErrorDetailed(message: string, error: Error) {
-        const details = error.name + ' - ' + error.message;
+        const details = error.name + ' - ' + error?.message;
         vscode.window.showErrorMessage(details);
         console.error(details);
         this.showError(message);