fix(core): handle plugin errors from isolation correctly (#22890)

nrwl · Apr 19, 2024 · 08f6421 · 08f6421
1 parent a927e93
commit 08f6421
Show file tree

Hide file tree

Showing 8 changed files with 160 additions and 109 deletions.
diff --git a/packages/nx/src/daemon/server/server.ts b/packages/nx/src/daemon/server/server.ts
@@ -112,10 +112,11 @@ async function handleMessage(socket, data: string) {
   }
 
   if (daemonIsOutdated()) {
-    await respondWithErrorAndExit(socket, `Lock files changed`, {
-      name: '',
-      message: 'LOCK-FILES-CHANGED',
-    });
+    await respondWithErrorAndExit(
+      socket,
+      `Lock files changed`,
+      new Error('LOCK-FILES-CHANGED')
+    );
   }
 
   resetInactivityTimeout(handleInactivityTimeout);

diff --git a/packages/nx/src/daemon/socket-utils.ts b/packages/nx/src/daemon/socket-utils.ts
@@ -2,7 +2,7 @@ import { unlinkSync } from 'fs';
 import { platform } from 'os';
 import { join, resolve } from 'path';
 import { DAEMON_SOCKET_PATH, socketDir } from './tmp-dir';
-import { DaemonProjectGraphError } from '../project-graph/error-types';
+import { createSerializableError } from '../utils/serializable-error';
 
 export const isWindows = platform() === 'win32';
 
@@ -27,29 +27,14 @@ export function killSocketOrPath(): void {
   } catch {}
 }
 
-// Include the original stack trace within the serialized error so that the client can show it to the user.
-function serializeError(error: Error | null): string | null {
-  if (!error) {
-    return null;
-  }
-
-  if (error instanceof DaemonProjectGraphError) {
-    error.errors = error.errors.map((e) => JSON.parse(serializeError(e)));
-  }
-
-  return `{${Object.getOwnPropertyNames(error)
-    .map((k) => `"${k}": ${JSON.stringify(error[k])}`)
-    .join(',')}}`;
-}
-
 // Prepare a serialized project graph result for sending over IPC from the server to the client
 export function serializeResult(
   error: Error | null,
   serializedProjectGraph: string | null,
   serializedSourceMaps: string | null
 ): string | null {
   // We do not want to repeat work `JSON.stringify`ing an object containing the potentially large project graph so merge as strings
-  return `{ "error": ${serializeError(
-    error
+  return `{ "error": ${JSON.stringify(
+    error ? createSerializableError(error) : error
   )}, "projectGraph": ${serializedProjectGraph}, "sourceMaps": ${serializedSourceMaps} }`;
 }
diff --git a/packages/nx/src/project-graph/error-types.ts b/packages/nx/src/project-graph/error-types.ts
@@ -235,3 +235,12 @@ export class DaemonProjectGraphError extends Error {
     this.name = this.constructor.name;
   }
 }
+
+export class LoadPluginError extends Error {
+  constructor(public plugin: string, cause: Error) {
+    super(`Could not load plugin ${plugin}`, {
+      cause,
+    });
+    this.name = this.constructor.name;
+  }
+}
diff --git a/packages/nx/src/project-graph/plugins/isolation/messaging.ts b/packages/nx/src/project-graph/plugins/isolation/messaging.ts
@@ -5,6 +5,7 @@ import {
 import { PluginConfiguration } from '../../../config/nx-json';
 import { CreateDependenciesContext, CreateNodesContext } from '../public-api';
 import { LoadedNxPlugin } from '../internal-api';
+import { Serializable } from 'child_process';
 
 export interface PluginWorkerLoadMessage {
   type: 'load';
@@ -26,7 +27,7 @@ export interface PluginWorkerLoadResult {
       }
     | {
         success: false;
-        error: string;
+        error: Error;
       };
 }
 
@@ -49,7 +50,7 @@ export interface PluginWorkerCreateNodesResult {
       }
     | {
         success: false;
-        error: string;
+        error: Error;
         tx: string;
       };
 }
@@ -72,7 +73,7 @@ export interface PluginCreateDependenciesResult {
       }
     | {
         success: false;
-        error: string;
+        error: Error;
         tx: string;
       };
 }
@@ -96,7 +97,7 @@ export interface PluginWorkerProcessProjectGraphResult {
       }
     | {
         success: false;
-        error: string;
+        error: Error;
         tx: string;
       };
 }
@@ -113,6 +114,38 @@ export type PluginWorkerResult =
   | PluginCreateDependenciesResult
   | PluginWorkerProcessProjectGraphResult;
 
+export function isPluginWorkerMessage(
+  message: Serializable
+): message is PluginWorkerMessage {
+  return (
+    typeof message === 'object' &&
+    'type' in message &&
+    typeof message.type === 'string' &&
+    [
+      'load',
+      'createNodes',
+      'createDependencies',
+      'processProjectGraph',
+    ].includes(message.type)
+  );
+}
+
+export function isPluginWorkerResult(
+  message: Serializable
+): message is PluginWorkerResult {
+  return (
+    typeof message === 'object' &&
+    'type' in message &&
+    typeof message.type === 'string' &&
+    [
+      'load-result',
+      'createNodesResult',
+      'createDependenciesResult',
+      'processProjectGraphResult',
+    ].includes(message.type)
+  );
+}
+
 type MaybePromise<T> = T | Promise<T>;
 
 // The handler can return a message to be sent back to the process from which the message originated
@@ -126,28 +159,20 @@ type MessageHandlerReturn<T extends PluginWorkerMessage | PluginWorkerResult> =
 export async function consumeMessage<
   T extends PluginWorkerMessage | PluginWorkerResult
 >(
-  raw: string | T,
+  raw: T,
   handlers: {
     [K in T['type']]: (
       // Extract restricts the type of payload to the payload of the message with the type K
       payload: Extract<T, { type: K }>['payload']
     ) => MessageHandlerReturn<T>;
   }
 ) {
-  const message: T = typeof raw === 'string' ? JSON.parse(raw) : raw;
+  const message: T = raw;
   const handler = handlers[message.type];
   if (handler) {
     const response = await handler(message.payload);
     if (response) {
-      process.send!(createMessage(response));
+      process.send!(response);
     }
-  } else {
-    throw new Error(`Unhandled message type: ${message.type}`);
   }
 }
-
-export function createMessage(
-  message: PluginWorkerMessage | PluginWorkerResult
-): string {
-  return JSON.stringify(message);
-}
diff --git a/packages/nx/src/project-graph/plugins/isolation/plugin-pool.ts b/packages/nx/src/project-graph/plugins/isolation/plugin-pool.ts
@@ -1,4 +1,4 @@
-import { ChildProcess, fork } from 'child_process';
+import { ChildProcess, Serializable, fork } from 'child_process';
 import path = require('path');
 
 import { PluginConfiguration } from '../../../config/nx-json';
@@ -7,7 +7,7 @@ import { PluginConfiguration } from '../../../config/nx-json';
 // import { logger } from '../../utils/logger';
 
 import { LoadedNxPlugin, nxPluginCache } from '../internal-api';
-import { PluginWorkerResult, consumeMessage, createMessage } from './messaging';
+import { consumeMessage, isPluginWorkerResult } from './messaging';
 
 const cleanupFunctions = new Set<() => void>();
 
@@ -45,7 +45,7 @@ export function loadRemoteNxPlugin(
       ...(isWorkerTypescript ? ['-r', 'ts-node/register'] : []),
     ],
   });
-  worker.send(createMessage({ type: 'load', payload: { plugin, root } }));
+  worker.send({ type: 'load', payload: { plugin, root } });
 
   // logger.verbose(`[plugin-worker] started worker: ${worker.pid}`);
 
@@ -103,14 +103,11 @@ function createWorkerHandler(
 ) {
   let pluginName: string;
 
-  return function (message: string) {
-    const parsed = JSON.parse(message);
-    // logger.verbose(
-    //   `[plugin-pool] received message: ${parsed.type} from ${
-    //     pluginName ?? worker.pid
-    //   }`
-    // );
-    consumeMessage<PluginWorkerResult>(parsed, {
+  return function (message: Serializable) {
+    if (!isPluginWorkerResult(message)) {
+      return;
+    }
+    return consumeMessage(message, {
       'load-result': (result) => {
         if (result.success) {
           const { name, createNodesPattern } = result;
@@ -124,12 +121,10 @@ function createWorkerHandler(
                   (configFiles, ctx) => {
                     const tx = pluginName + ':createNodes:' + performance.now();
                     return registerPendingPromise(tx, pending, () => {
-                      worker.send(
-                        createMessage({
-                          type: 'createNodes',
-                          payload: { configFiles, context: ctx, tx },
-                        })
-                      );
+                      worker.send({
+                        type: 'createNodes',
+                        payload: { configFiles, context: ctx, tx },
+                      });
                     });
                   },
                 ]
@@ -139,12 +134,10 @@ function createWorkerHandler(
                   const tx =
                     pluginName + ':createDependencies:' + performance.now();
                   return registerPendingPromise(tx, pending, () => {
-                    worker.send(
-                      createMessage({
-                        type: 'createDependencies',
-                        payload: { context: ctx, tx },
-                      })
-                    );
+                    worker.send({
+                      type: 'createDependencies',
+                      payload: { context: ctx, tx },
+                    });
                   });
                 }
               : undefined,
@@ -153,12 +146,10 @@ function createWorkerHandler(
                   const tx =
                     pluginName + ':processProjectGraph:' + performance.now();
                   return registerPendingPromise(tx, pending, () => {
-                    worker.send(
-                      createMessage({
-                        type: 'processProjectGraph',
-                        payload: { graph, ctx, tx },
-                      })
-                    );
+                    worker.send({
+                      type: 'processProjectGraph',
+                      payload: { graph, ctx, tx },
+                    });
                   });
                 }
               : undefined,

diff --git a/packages/nx/src/project-graph/plugins/isolation/plugin-worker.ts b/packages/nx/src/project-graph/plugins/isolation/plugin-worker.ts
@@ -1,14 +1,18 @@
-import { consumeMessage, PluginWorkerMessage } from './messaging';
+import { consumeMessage, isPluginWorkerMessage } from './messaging';
 import { LoadedNxPlugin } from '../internal-api';
 import { loadNxPlugin } from '../loader';
-import { runCreateNodesInParallel } from '../utils';
+import { Serializable } from 'child_process';
+import { createSerializableError } from '../../../utils/serializable-error';
 
 global.NX_GRAPH_CREATION = true;
 
 let plugin: LoadedNxPlugin;
 
-process.on('message', async (message: string) => {
-  consumeMessage<PluginWorkerMessage>(message, {
+process.on('message', async (message: Serializable) => {
+  if (!isPluginWorkerMessage(message)) {
+    return;
+  }
+  return consumeMessage(message, {
     load: async ({ plugin: pluginConfiguration, root }) => {
       process.chdir(root);
       try {
@@ -31,9 +35,7 @@ process.on('message', async (message: string) => {
           type: 'load-result',
           payload: {
             success: false,
-            error: `Could not load plugin ${plugin} \n ${
-              e instanceof Error ? e.stack : ''
-            }`,
+            error: createSerializableError(e),
           },
         };
       }
@@ -48,7 +50,11 @@ process.on('message', async (message: string) => {
       } catch (e) {
         return {
           type: 'createNodesResult',
-          payload: { success: false, error: e.stack, tx },
+          payload: {
+            success: false,
+            error: createSerializableError(e),
+            tx,
+          },
         };
       }
     },
@@ -62,7 +68,11 @@ process.on('message', async (message: string) => {
       } catch (e) {
         return {
           type: 'createDependenciesResult',
-          payload: { success: false, error: e.stack, tx },
+          payload: {
+            success: false,
+            error: createSerializableError(e),
+            tx,
+          },
         };
       }
     },
@@ -76,7 +86,11 @@ process.on('message', async (message: string) => {
       } catch (e) {
         return {
           type: 'processProjectGraphResult',
-          payload: { success: false, error: e.stack, tx },
+          payload: {
+            success: false,
+            error: createSerializableError(e),
+            tx,
+          },
         };
       }
     },