-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Connect: Stop execution of main process on child process spawn error #56683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4ec6145
1f525f4
811adcc
7dadba6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,7 +73,7 @@ function waitForMatchInStdout( | |
| let chunks = ''; | ||
|
|
||
| const timeout = setTimeout(() => { | ||
| rejectOnError( | ||
| rejectWithCleanup( | ||
| new ResolveError(requestedAddress, process, 'the operation timed out') | ||
| ); | ||
| }, timeoutMs); | ||
|
|
@@ -94,11 +94,23 @@ function waitForMatchInStdout( | |
| } | ||
| }; | ||
|
|
||
| const rejectOnError = (error: Error) => { | ||
| const rejectWithCleanup = (error: Error) => { | ||
| reject(error); | ||
| removeListeners(); | ||
| }; | ||
|
|
||
| const rejectOnError = (error: Error) => { | ||
| 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, | ||
| }); | ||
|
Comment on lines
+103
to
+110
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
|
||
| rejectWithCleanup(errorToReport); | ||
| }; | ||
|
|
||
| const rejectOnClose = (code: number, signal: NodeJS.Signals) => { | ||
| const codeOrSignal = [ | ||
| // code can be 0, so we cannot just check it the same way as the signal. | ||
|
|
@@ -108,7 +120,7 @@ function waitForMatchInStdout( | |
| .filter(Boolean) | ||
| .join(' '); | ||
| const details = codeOrSignal ? ` with ${codeOrSignal}` : ''; | ||
| rejectOnError( | ||
| rejectWithCleanup( | ||
| new ResolveError( | ||
| requestedAddress, | ||
| process, | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error handling was moved out of here to
main.tsso that we can properly stop execution code after catching an error, asapp.exit(1)does not stop execution immediately (see RCA for more info).