@jrieken

The typical pattern seems to be something.done(null, errors.onUnexpectedError)) and so instead of renaming that to something.then(null, errors.onUnexpectedError)) it would be nice if we could remove that code entirely and rely on some global handler to print this error properly.

I did a search for unhandledRejection and it looks like we do not handle that event in any place besides:

• electron-main: just a console.log that probably no-one ever sees
• extension-host: has a sophisticated way of handling it (code below)

Would it make sense to have the extension-host handler also in electron-main, electron-browser and any forked process and just call errors.onUnexpectedError?

// Print a console message when rejection isn't handled within N seconds. For details:
// see https://nodejs.org/api/process.html#process_event_unhandledrejection
// and https://nodejs.org/api/process.html#process_event_rejectionhandled
const unhandledPromises: Promise<any>[] = [];
process.on('unhandledRejection', (reason: any, promise: Promise<any>) => {
	unhandledPromises.push(promise);
	setTimeout(() => {
		const idx = unhandledPromises.indexOf(promise);
		if (idx >= 0) {
			unhandledPromises.splice(idx, 1);
			console.warn('rejected promise not handled within 1 second');
			onUnexpectedError(reason);
		}
	}, 1000);
});

process.on('rejectionHandled', (promise: Promise<any>) => {
	const idx = unhandledPromises.indexOf(promise);
	if (idx >= 0) {
		unhandledPromises.splice(idx, 1);
	}
});

Update: looks like we do have a window.addEventListener('unhandledrejection',...) in the renderer that is probably (hopefully) firing.

The typical pattern seems to be something.done(null, errors.onUnexpectedError)) and so instead of renaming that to something.then(null, errors.onUnexpectedError)) it

Yeah, that's what I have seen. More work but will have the better outcome.

Update: looks like we do have a window.addEventListener('unhandledrejection',...) in the renderer that is probably (hopefully) firing.

That - we should maybe also have that in the main-process and maybe argument the error with some little extra information, e.g that it was an unhandled rejection

@jrieken is the logic in the extensionHost something we should use everywhere or is it very specific to extensions? If so, we could extract that to some helper maybe.

Yeah, looks like this became a thing, even in browsers: https://developer.mozilla.org/en-US/docs/Web/Events/unhandledrejection and https://developer.mozilla.org/en-US/docs/Web/Events/rejectionhandled. We just need to make sure to listen to the right listener, window vs process

One thing to note is that as long as we are not on native promises, removing the error handler will result in many of these:

errors.js:42 WARNING: Promise with no error callback:27
errors.js:43 {exception: null, error: Error: nix
    at ResourcesDropHandler.doHandleDrop (file:///Users/bpasero/Development/monaco/out/vs…, promise: ErrorPromise_ctor, handler: undefined, id: 27, …}

But I think we should get rid of the vast amount of done(..., onUnexpectedError) as much as possible. I would only leave the error handler if it does anything else than calling onUnexpectedError.

@bpasero I don't agree with removing the usage of errors.onUnexpectedError everywhere. In the standalone editor case, we cannot install a global unhandledRejection handler because we are just a widget on a webpage.

The problem is that all of our canceled promises will end up in the global handler, and someone who integrates the editor should not have to add a global handler where they check for the Canceled rejection, etc. So, at least for source code that gets shipped in the standalone editor, please continue to use errors.onUnexpectedError at the last place where a promise is not returned to someone else.

@alexandrudima yeah my changes only touch the workbench, I leave editor to you, good point.

PS: to be fair, we probably have tons of code that goes to the standalone editor where we forget to handle errors in promises (because we have no tooling to tell us otherwise), so I would expect that this already happens today.

@bpasero I did a full pas last endgame before shipping the standalone editor.

I went through callers of cancel / users of cancellation token in the code shipped with the standalone editor and made sure those places use errors.onUnexpectedError in 34ca4c7

I added a process.on('undhandledRejection') for our bootstrap scripts (e.g. used for search, file watcher - except for extension host that has its own logic).

Both electron-main and electron-browser already had proper listeners for these applied.

@jrieken I've noticed fc941ae which removes a lot of the handlers I've added manually last endgame. Can we please handle cancelation promises again in the standalone editor code? I don't know how many more commits there are like this.

Yeah, sorry. A little over-eager on my side. I add something back

done

done with done 👯