Connect: Stop execution of main process on child process spawn error#56683
Connect: Stop execution of main process on child process spawn error#56683
Conversation
ravicious
added
the
no-changelog
ravicious
force-pushed
the
r7s/main-process-err
branch
3 times, most recently
from
8491137 to
929d4d2
Compare
| } catch (error) { | ||
| const message = 'Could not initialize the main process'; | ||
| logger.error(message, error); | ||
| dialog.showErrorBox(message, ensureError(error).message); |
There was a problem hiding this comment.
I had a TODO item that said:
Investigate why the code from this branch actually doesn't show any error dialog box when tsh cannot start. The error is logged and the process remains in the background but no dialog error is shown, indicating that the app is blocked on the code that's supposed to show the dialog.
After starting the app in dev mode, I discovered this error:
(node:10220) UnhandledPromiseRejectionWarning: TypeError: Error processing argument at index 1, conversion failure from
at Object.showErrorBox (node:electron/js2c/browser_init:2:21451)
at initializeApp (C:\teleport\web\packages\teleterm\build\app\main\index.js:38975:21)
This led to electron/electron#6793 which made me realize that I was passing error as the second argument to dialog.showErrorBox and it was throwing an error because of that. The second argument is supposed to be a string, but since error in this context has type any, TS wasn't complaining about this.
It looks like the ensureError utility is actually paying off!
| const errorToReport = error.message.includes(process.spawnfile) | ||
| ? error | ||
| : // Attach spawnfile so that the process can be identified without looking at the stacktrace. | ||
| // resolveNetworkAddress is the only function that ends up surfacing to the UI the error | ||
| // event of a process. | ||
| new Error(`${process.spawnfile}: ${error.message}`, { | ||
| cause: error, | ||
| }); |
There was a problem hiding this comment.
I'm not sure if this is necessary, but I don't think there's any guarantee that the error message of an error from the error event of a process is going to include the spawnfile.
It certainly happens for EACCESS on macOS as shown in the screenshot, hence why I'm prepending the spawnfile only if the message doesn't already include it. Since it's an async error, the stacktrace itself is not going to include any relevant part like MainProcess.initTshd, that's why I'm trying to ensure that the error message itself identifies the process.
| this.initIpc(); | ||
| } catch (err) { | ||
| this.logger.error('Failed to start main process: ', err.message); | ||
| app.exit(1); |
There was a problem hiding this comment.
Error handling was moved out of here to main.ts so that we can properly stop execution code after catching an error, as app.exit(1) does not stop execution immediately (see RCA for more info).
ravicious
force-pushed
the
r7s/main-process-err
branch
from
929d4d2 to
7dadba6
Compare
|
@ravicious See the table below for backport results.
|
3 participants
Closes #56272
This PR makes it so that when tshd or the shared process fails to spawn, we show a dialog box with a relevant error message rather than with "Cannot read properties of undefined (reading 'then')", as shown in #56272. It makes it so that when a user sends a screenshot of the dialog, we can identify the process which caused the problem instead of having to ask the user for logs.
I posted an RCA under the issue (#56272 (comment)) if you'd like to learn more about the problem. In short, we thought that Electron's
app.exit(1)immediately stops the execution of a program, but that is not the case. On top of that, macOS and Windows behave differently when attempting to spawn a child process (this is described below in the Screenshots section), so the original issue could be reproduced only on Windows.Screenshots
The screenshots both show the error that is shown after trying to start the dev version of the app with
README.mdas the tsh binary.(
E_DO_NOT_USEis just so that I don't execute this by mistake when going through my shell history)On Windows,
child_process.spawnthrows a sync error. The error message doesn't indicate which process failed to spawn, but the stacktrace does include this information (MainProcess.initTshd).On macOS, the error isn't thrown immediately on spawn, rather it's emitted with the error event on a process. Thus the title of the dialog is different as the error is surfaced later in the initialization process. On macOS, the error dialog content is unfortunately centered, so the stacktrace looks a bit weird. We're forced to use it though, as according to the docs of
dialog.showErrorBox: