From f6bd581f773d84c58854644e77d727a390943fb0 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 2 Sep 2022 18:49:45 -0500 Subject: [PATCH] Better `child_process` error handling v2 - timeouts and actually fail process for error in scope (#62) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to https://github.com/matrix-org/matrix-public-archive/pull/51 Better `child_process` error handling for a couple scenarios with the finger pointing at it 👉 Also make sure we handle all of these scenarios: 1. Child process fork script throws an `uncaughtException` or `unhandledRejection` - These are captured and serialized back to the parent and stored in `childErrors` and exposed if we never get a successful rendered HTML response. 2. Child process fails to startup - Render process is rejected in the `child.on('error', ...` callback 3. 👉 Child process times out and is aborted - Render process is rejected in the `child.on('error', ...` callback and any `childErrors` encountered are logged 4. 👉 Child process fork script throws an error in scope of in `process.on('message', async (renderOptions) => {` - Child exits with code 1 and we reject the render process with the error 5. Child process exits with code 1 (error) - Render process is rejected with any `childError` info 6. Child process exits with code 0 (success) but never sends back any HTML - We have a `returnedData` data check and any child errors encountered are logged --- .../1-render-hydrogen-to-string.js | 103 ++++++++++++------ ...2-render-hydrogen-to-string-fork-script.js | 6 +- server/lib/rethrown-error.js | 2 +- 3 files changed, 74 insertions(+), 37 deletions(-) diff --git a/server/hydrogen-render/1-render-hydrogen-to-string.js b/server/hydrogen-render/1-render-hydrogen-to-string.js index 9595fef..c6e1a02 100644 --- a/server/hydrogen-render/1-render-hydrogen-to-string.js +++ b/server/hydrogen-render/1-render-hydrogen-to-string.js @@ -23,10 +23,48 @@ if (!logOutputFromChildProcesses) { ); } +function assembleErrorAfterChildExitsWithErrors(exitCode, childErrors) { + assert(childErrors); + + let extraErrorsMessage = ''; + if (childErrors.length > 1) { + extraErrorsMessage = ` (somehow we saw ${ + childErrors.length + } errors but we really always expect 1 error)\n${childErrors + .map((childError, index) => `${index}. ${childError.stack}`) + .join('\n')}`; + } + + let childErrorToDisplay; + if (childErrors.length === 0) { + childErrorToDisplay = new Error('No child errors'); + // Clear the stack trace part of the stack string out because this is just a + // note about the lack of errors, not an actual error and is just noisy with + // that extra fluff. + childErrorToDisplay.stack = childErrorToDisplay.message; + } else if (childErrors.length === 1) { + childErrorToDisplay = childErrors[0]; + } else { + childErrorToDisplay = new Error('Multiple child errors listed above ^'); + // Clear the stack trace part of the stack string out because this is just a + // note about the other errors, not an actual error and is just noisy with + // that extra fluff. + childErrorToDisplay.stack = childErrorToDisplay.message; + } + + const childErrorSummary = new RethrownError( + `Child process exited with code ${exitCode}${extraErrorsMessage}`, + childErrorToDisplay + ); + + return childErrorSummary; +} + async function renderHydrogenToString(renderOptions) { + let abortTimeoutId; try { - let data = ''; let childErrors = []; + let childExitCode = '(not set yet)'; const controller = new AbortController(); const { signal } = controller; @@ -60,11 +98,12 @@ async function renderHydrogenToString(renderOptions) { child.send(renderOptions); // Stops the child process if it takes too long - setTimeout(() => { + abortTimeoutId = setTimeout(() => { controller.abort(); }, RENDER_TIMEOUT); - await new Promise((resolve, reject) => { + const returnedData = await new Promise((resolve, reject) => { + let data = ''; // Collect the data passed back by the child child.on('message', function (result) { if (result.error) { @@ -85,53 +124,47 @@ async function renderHydrogenToString(renderOptions) { }); child.on('close', (exitCode) => { + childExitCode = exitCode; // Exited successfully if (exitCode === 0) { resolve(data); } else { - let extraErrorsMessage = ''; - if (childErrors.length > 1) { - extraErrorsMessage = ` (somehow we saw ${ - childErrors.length - } errors but we really always expect 1 error)\n${childErrors - .map((childError, index) => `${index}. ${childError.stack}`) - .join('\n')}`; - } - - let childErrorToDisplay = new Error('No child errors'); - if (childErrors.length === 1) { - childErrorToDisplay = childErrors[0]; - } else if (childErrors.length > 1) { - childErrorToDisplay = new Error('Multiple child errors listed above ^'); - } - - const error = new RethrownError( - `Child process failed with exit code ${exitCode}${extraErrorsMessage}`, - childErrorToDisplay + const childErrorSummary = assembleErrorAfterChildExitsWithErrors( + childExitCode, + childErrors ); - reject(error); + reject(childErrorSummary); } }); // When a problem occurs when spawning the process or gets aborted child.on('error', (err) => { if (err.name === 'AbortError') { - throw new RethrownError( - `Timed out while rendering Hydrogen to string so we aborted the child process after ${RENDER_TIMEOUT}ms`, - err + const childErrorSummary = assembleErrorAfterChildExitsWithErrors( + childExitCode, + childErrors ); + reject( + new RethrownError( + `Timed out while rendering Hydrogen to string so we aborted the child process after ${RENDER_TIMEOUT}ms. Any child errors? (${childErrors.length})`, + childErrorSummary + ) + ); + } else { + reject(err); } - - reject(err); }); }); - assert( - data, - `No HTML sent from child process to render Hydrogen. Any child errors? (${childErrors.length})` - ); + if (!returnedData) { + const childErrorSummary = assembleErrorAfterChildExitsWithErrors(childExitCode, childErrors); + throw new RethrownError( + `No HTML sent from child process to render Hydrogen. Any child errors? (${childErrors.length})`, + childErrorSummary + ); + } - return data; + return returnedData; } catch (err) { throw new RethrownError( `Failed to render Hydrogen to string. In order to reproduce, feed in these arguments into \`renderHydrogenToString(...)\`:\n renderToString arguments: ${JSON.stringify( @@ -139,6 +172,10 @@ async function renderHydrogenToString(renderOptions) { )}`, err ); + } finally { + // We don't have to add a undefined/null check here because `clearTimeout` + // works with any value you give it and doesn't throw an error. + clearTimeout(abortTimeoutId); } } diff --git a/server/hydrogen-render/2-render-hydrogen-to-string-fork-script.js b/server/hydrogen-render/2-render-hydrogen-to-string-fork-script.js index c7349a1..c2fe6d6 100644 --- a/server/hydrogen-render/2-render-hydrogen-to-string-fork-script.js +++ b/server/hydrogen-render/2-render-hydrogen-to-string-fork-script.js @@ -88,10 +88,10 @@ process.on('message', async (renderOptions) => { }); } catch (err) { // We need to wait for the error to completely send to the parent - // process before we throw the error again and exit the process. + // process before we exit the process. await serializeError(err); - // Throw the error so the process fails and exits - throw err; + // Fail the process and exit + process.exit(1); } }); diff --git a/server/lib/rethrown-error.js b/server/lib/rethrown-error.js index b8d5bce..cae581f 100644 --- a/server/lib/rethrown-error.js +++ b/server/lib/rethrown-error.js @@ -32,7 +32,7 @@ class RethrownError extends ExtendedError { super(message); if (!error) throw new Error('RethrownError requires a message and error'); this.original = error; - this.newStack = this.stack; + this.originalStack = this.stack; // The number of lines that make up the message itself. We count this by the // number of `\n` and `+ 1` for the first line because it doesn't start with