Skip to content
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

Don't handle uncaught exceptions during quit #208932

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Mar 27, 2024

This fixes a longstanding issue I've been seeing on macos where quitting Insiders will cause any Code - OSS instance to hang using 100% CPU and the only way to close it is by force killing it via the Activity Monitor

This fixes a longstanding issue I've been seeing on macos where quitting Insiders
will cause any Code - OSS instance to hang using 100% CPU and the only way to close
it is by force killing it via the Activity Monitor
@Tyriar Tyriar added this to the April 2024 milestone Mar 27, 2024
@Tyriar Tyriar self-assigned this Mar 27, 2024
@bpasero
Copy link
Member

bpasero commented Mar 28, 2024

Curious, do we need to be looking at the other handlers as well or is it really just the one in app.ts:

image

@@ -359,6 +359,11 @@ export class CodeApplication extends Disposable {
// We handle uncaught exceptions here to prevent electron from opening a dialog to the user
setUnexpectedErrorHandler(error => this.onUnexpectedError(error));
process.on('uncaughtException', error => {
// Don't process the exception if exit is happening as it can cause the main process to
// hang while awaiting an IPC reply.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to rather listen to onWillShutdown as a way to know for sure that we are going down. The quitRequested might be true even though a window still vetos the shutdown.

@deepak1556
Copy link
Contributor

Curious, do we need to be looking at the other handlers as well or is it really just the one in app.ts:

Child processes usually get reaped by the main process, so I think handling only the main process should be fine for now.

I also tried to repro the issue by creating the EIO exception but that didn't cause any loop on exit. @Tyriar if possible can you debug this issue further on your end, basically a couple of things to sanity check

  1. Given there are multiple exceptions raised in your case, maybe print the details of the exception and see for which one return undefined actually resolved the issue.
  2. Also try moving the return statement down the callstack after entering uncaught exception handler and confirm which code path actually triggers the issue.

@deepak1556
Copy link
Contributor

deepak1556 commented Mar 28, 2024

@alexdima reminded me in standup today that there was a similar issue on server ed63059 trying to do a console on broken pipe caused an endless loop. We still have the same handler on client, can you try the following patch instead to confirm if they are the same issue,

diff --git a/src/main.js b/src/main.js
index 90de17b278b..353985233e0 100644
--- a/src/main.js
+++ b/src/main.js
@@ -28,6 +28,8 @@ const { getUNCHost, addUNCHostToAllowlist } = require('./vs/base/node/unc');
 const product = require('../product.json');
 const { app, protocol, crashReporter, Menu } = require('electron');
 
+process.env['VSCODE_HANDLES_SIGPIPE'] = 'true';
+
 // Enable portable support
 const portable = bootstrapNode.configurePortable(product);
 
diff --git a/src/vs/code/electron-main/app.ts b/src/vs/code/electron-main/app.ts
index 578b00a796e..5ddff4f4c5f 100644
--- a/src/vs/code/electron-main/app.ts
+++ b/src/vs/code/electron-main/app.ts
@@ -364,6 +364,13 @@ export class CodeApplication extends Disposable {
                        }
                });
                process.on('unhandledRejection', (reason: unknown) => onUnexpectedError(reason));
+               process.on('SIGPIPE', () => {
+                       // See https://github.com/microsoft/vscode-remote-release/issues/6543
+                       // We would normally install a SIGPIPE listener in bootstrap.js
+                       // But in certain situations, the console itself can be in a broken pipe state
+                       // so logging SIGPIPE to the console will cause an infinite async loop
+                       onUnexpectedError(new Error(`Unexpected SIGPIPE`));
+               });
 
                // Dispose on shutdown
                this.lifecycleMainService.onWillShutdown(() => this.dispose());

@Tyriar Tyriar modified the milestones: April 2024, May 2024 Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants