Skip to content

Commit

Permalink
fix: fix #1200 (#1202)
Browse files Browse the repository at this point in the history
* fix: fix #1200

Cause:
  `window.showInformationMessage` reject with `Canceled` during shutdown. This happens even though the message seemed to have gone away.

Two fixes:
1. Do not attempt to log or show a message if it was a canceled promise.
2. Do not log when showing VS Code UI.

* Update errors.test.ts

* test: improve code coverage
  • Loading branch information
Jason3S authored Aug 27, 2021
1 parent 31fc152 commit 3bfe916
Show file tree
Hide file tree
Showing 4 changed files with 153 additions and 45 deletions.
32 changes: 14 additions & 18 deletions packages/client/src/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,24 @@ import {
CodeAction,
commands,
ConfigurationScope,
Diagnostic,
Disposable,
FileType,
QuickPickItem,
QuickPickOptions,
Range,
Selection,
TextDocument,
TextEditorRevealType,
Uri,
window,
workspace,
WorkspaceEdit,
Selection,
Diagnostic,
} from 'vscode';
import { TextEdit } from 'vscode-languageclient/node';
import { ClientSideCommandHandlerApi, SpellCheckerSettingsProperties } from './client';
import * as di from './di';
import { extractMatchingDiagRanges, extractMatchingDiagTexts, getCSpellDiags } from './diags';
import { ClientSideCommandHandlerApi, SpellCheckerSettingsProperties } from './client';
import * as Settings from './settings';
import {
ConfigTargetLegacy,
Expand Down Expand Up @@ -54,7 +54,7 @@ import { normalizeWords } from './settings/CSpellSettings';
import { createDictionaryTargetForFile, DictionaryTarget } from './settings/DictionaryTarget';
import { mapConfigTargetToClientConfigTarget } from './settings/mappers/configTarget';
import { configurationTargetToDictionaryScope, dictionaryScopeToConfigurationTarget } from './settings/targetAndScope';
import { catchErrors, handleErrors, handleErrorsEx, logError, onError, OnErrorHandler } from './util/errors';
import { catchErrors, handleErrors, ignoreError, OnErrorResolver } from './util/errors';
import { performance, toMilliseconds } from './util/perf';
import { scrollToText } from './util/textEditor';
import { findMatchingDocument } from './vscode/findDocument';
Expand Down Expand Up @@ -147,7 +147,7 @@ const commandHandlers: CommandHandler = {
'cSpell.createCSpellConfig': createCSpellConfig,
};

function pVoid<T>(p: Promise<T> | Thenable<T>, context: string, onErrorHandler: OnErrorHandler = onError): Promise<void> {
function pVoid<T>(p: Promise<T> | Thenable<T>, context: string, onErrorHandler: OnErrorResolver = ignoreError): Promise<void> {
const v = Promise.resolve(p).then(() => {});
return handleErrors(v, context, onErrorHandler);
}
Expand Down Expand Up @@ -329,7 +329,7 @@ export function enableDisableLanguageId(
configTarget: ConfigurationTarget | undefined,
enable: boolean
): Promise<void> {
return handleErrorsEx(async () => {
return handleErrors(async () => {
const t = await (configTarget ? tfCfg(configTarget, uri) : targetsForUri(uri));
return Settings.enableLanguageIdForTarget(languageId, enable, t);
}, ctx(`enableDisableLanguageId enable: ${enable}`, configTarget, uri));
Expand All @@ -342,7 +342,7 @@ export function enableDisableLocale(
configScope: ConfigurationScope | undefined,
enable: boolean
): Promise<void> {
return handleErrorsEx(async () => {
return handleErrors(async () => {
const t = await (configTarget ? tfCfg(configTarget, uri, configScope) : targetsForUri(uri));
return Settings.enableLocaleForTarget(locale, enable, t);
}, ctx(`enableDisableLocale enable: ${enable}`, configTarget, uri));
Expand All @@ -355,7 +355,7 @@ export function enableDisableLocaleLegacy(target: ConfigTargetLegacy | boolean,
}

export function enableCurrentLanguage(): Promise<void> {
return handleErrorsEx(async () => {
return handleErrors(async () => {
const document = window.activeTextEditor?.document;
if (!document) return;
const targets = await targetsForTextDocument(document);
Expand All @@ -364,7 +364,7 @@ export function enableCurrentLanguage(): Promise<void> {
}

export function disableCurrentLanguage(): Promise<void> {
return handleErrorsEx(async () => {
return handleErrors(async () => {
const document = window.activeTextEditor?.document;
if (!document) return;
const targets = await targetsForTextDocument(document);
Expand Down Expand Up @@ -482,24 +482,20 @@ async function actionSuggestSpellingCorrections(): Promise<void> {
const matchingRanges = extractMatchingDiagRanges(document, selection, diags);
const r = matchingRanges?.[0] || range;
const matchingDiags = r && diags?.filter((d) => !!d.range.intersection(r));

if (!document || !selection || !r || !matchingDiags) {
return pVoid(window.showWarningMessage('Nothing to suggest.'), 'actionSuggestSpellingCorrections', onError);
return pVoid(window.showInformationMessage('Nothing to suggest.'), 'actionSuggestSpellingCorrections');
}

const actions = await di.get('client').requestSpellingSuggestions(document, r, matchingDiags);
if (!actions || !actions.length) {
return pVoid(
window.showWarningMessage(`No Suggestions Found for ${document.getText(r)}`),
'actionSuggestSpellingCorrections',
logError
);
return pVoid(window.showInformationMessage(`No Suggestions Found for ${document.getText(r)}`), 'actionSuggestSpellingCorrections');
}

const items: SuggestionQuickPickItem[] = actions.map((a) => ({ label: a.title, _action: a }));
const picked = await window.showQuickPick(items);

if (picked && picked._action.command) {
const { command: cmd, arguments: args = [] } = picked._action.command;

commands.executeCommand(cmd, ...args);
}
}
Expand Down Expand Up @@ -560,7 +556,7 @@ async function actionJumpToSpellingError(which: 'next' | 'previous', suggest: bo
const matchingDiags = diags ? (which === 'next' ? nextDiags(diags, selection) : previousDiags(diags, selection)) : undefined;
const range = matchingDiags?.range;
if (!document || !selection || !range || !matchingDiags) {
return pVoid(window.showInformationMessage('No issues found in this document.'), 'actionSuggestSpellingCorrections', onError);
return pVoid(window.showInformationMessage('No issues found in this document.'), 'actionJumpToSpellingError');
}

editor.revealRange(range, TextEditorRevealType.InCenterIfOutsideViewport);
Expand Down
4 changes: 2 additions & 2 deletions packages/client/src/extensionRegEx/extensionRegEx.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as vscode from 'vscode';
import { CSpellClient, CSpellUserSettings } from '../client';
import { extensionId } from '../constants';
import { catchErrors, logError, logErrors, onError } from '../util/errors';
import { catchErrors, logError, logErrors, showError } from '../util/errors';
import { toRegExp } from './evaluateRegExp';
import { PatternMatcherClient } from './patternMatcherClient';
import { RegexpOutlineItem, RegexpOutlineProvider } from './RegexpOutlineProvider';
Expand Down Expand Up @@ -47,7 +47,7 @@ export function activate(context: vscode.ExtensionContext, clientSpellChecker: C
let pattern: string | undefined = undefined;
let history: string[] = [];

const updateDecorations = catchErrors(_updateDecorations, 'updateDecorations', onError);
const updateDecorations = catchErrors(_updateDecorations, 'updateDecorations', showError);

async function _updateDecorations() {
disposeCurrent();
Expand Down
49 changes: 45 additions & 4 deletions packages/client/src/util/errors.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { isError, silenceErrors, logErrors, handleErrors, catchErrors } from './errors';
import { window } from 'vscode';
import { catchErrors, ErrorHandlers, handleErrors, isError, logErrors, Resolvers } from './errors';

const debug = jest.spyOn(console, 'debug').mockImplementation(() => {});
const log = jest.spyOn(console, 'log').mockImplementation(() => {});
Expand Down Expand Up @@ -31,9 +31,9 @@ describe('Validate errors', () => {
${'error'} | ${false} | ${'error'}
${'error'} | ${true} | ${undefined}
`('silenceErrors $message $doThrow', async ({ message, doThrow, expected }) => {
await expect(silenceErrors(e(message, doThrow), 'silenceErrors')).resolves.toBe(expected);
await expect(ErrorHandlers.silenceErrors(e(message, doThrow), 'silenceErrors')).resolves.toBe(expected);
expect(error).toHaveBeenCalledTimes(0);
expect(log).toHaveBeenCalledTimes(expected ? 0 : 1);
expect(log).toHaveBeenCalledTimes(0);
expect(debug).toHaveBeenCalledTimes(0);
});

Expand Down Expand Up @@ -70,9 +70,50 @@ describe('Validate errors', () => {
expect(log).toHaveBeenCalledTimes(0);
expect(debug).toHaveBeenCalledTimes(0);
});

test.each`
message | doThrow | resolver | expected | errorCnt | logCnt
${'error'} | ${false} | ${Resolvers.showError} | ${'error'} | ${0} | ${0}
${'error'} | ${true} | ${Resolvers.showError} | ${undefined} | ${1} | ${0}
${'Canceled'} | ${true} | ${Resolvers.showError} | ${undefined} | ${0} | ${0}
${'error'} | ${false} | ${Resolvers.logError} | ${'error'} | ${0} | ${0}
${'error'} | ${true} | ${Resolvers.logError} | ${undefined} | ${0} | ${1}
${'Canceled'} | ${true} | ${Resolvers.logError} | ${undefined} | ${0} | ${0}
${'error'} | ${false} | ${Resolvers.ignoreError} | ${'error'} | ${0} | ${0}
${'error'} | ${true} | ${Resolvers.ignoreError} | ${undefined} | ${0} | ${0}
${'Canceled'} | ${true} | ${Resolvers.ignoreError} | ${undefined} | ${0} | ${0}
`('Resolvers: $message $doThrow $resolver', async ({ message, doThrow, resolver, expected, errorCnt, logCnt }) => {
await expect(handleErrors(e(message, doThrow), 'handleErrors', resolver)).resolves.toBe(expected);
expect(error).toHaveBeenCalledTimes(errorCnt);
expect(log).toHaveBeenCalledTimes(logCnt);
expect(debug).toHaveBeenCalledTimes(0);
});

test.each`
message | doThrow | handler | expected | errorCnt | logCnt
${'error'} | ${false} | ${ErrorHandlers.showErrors} | ${'error'} | ${0} | ${0}
${'error'} | ${true} | ${ErrorHandlers.showErrors} | ${undefined} | ${1} | ${0}
${'Canceled'} | ${true} | ${ErrorHandlers.showErrors} | ${undefined} | ${0} | ${0}
${'error'} | ${false} | ${ErrorHandlers.logErrors} | ${'error'} | ${0} | ${0}
${'error'} | ${true} | ${ErrorHandlers.logErrors} | ${undefined} | ${0} | ${1}
${'Canceled'} | ${true} | ${ErrorHandlers.logErrors} | ${undefined} | ${0} | ${0}
${'error'} | ${false} | ${ErrorHandlers.silenceErrors} | ${'error'} | ${0} | ${0}
${'error'} | ${true} | ${ErrorHandlers.silenceErrors} | ${undefined} | ${0} | ${0}
${'Canceled'} | ${true} | ${ErrorHandlers.silenceErrors} | ${undefined} | ${0} | ${0}
`('Handlers: $message $doThrow $handler', async ({ message, doThrow, handler, expected, errorCnt, logCnt }) => {
await expect(handler(e(message, doThrow), 'handleErrors')).resolves.toBe(expected);
expect(error).toHaveBeenCalledTimes(errorCnt);
expect(log).toHaveBeenCalledTimes(logCnt);
expect(debug).toHaveBeenCalledTimes(0);
});
});

async function e(message: string, doThrow = true): Promise<string> {
if (doThrow) throw new Error(message);
if (doThrow) {
const error = new Error(message);
error.name = message;
throw error;
}

return message;
}
113 changes: 92 additions & 21 deletions packages/client/src/util/errors.ts
Original file line number Diff line number Diff line change
@@ -1,57 +1,128 @@
import { window } from 'vscode';
import { format } from 'util';

export function isError(e: unknown): e is Error {
if (!e || typeof e !== 'object') return false;
const err = <Error>e;
return err.message !== undefined && err.name !== undefined;
}

export function onError(reason: unknown, context: string): Promise<void> {
if (isError(reason)) {
console.error('Error: context (%s): %o', context, reason);
return silenceErrors(window.showErrorMessage(reason.message), 'onError Handler showErrorMessage').then(() => {});
export const Resolvers = {
logError,
ignoreError,
showError,
};

export const ErrorHandlers = {
logErrors,
silenceErrors,
showErrors,
};

/*
* Rejected Promise Resolvers
*/

/**
* Error Resolver - show an error message before resolving the promise to undefined.
* @param reason - the reason the promise was rejected.
* @param context - the create context of the promise - used to trace the origin of the promise
* @returns Promise - resolves to `undefined` if an error has occurred.
*/
export function showError(reason: unknown, context: string): Promise<void> {
if (!isPromiseCanceledError(reason) && isError(reason)) {
console.error(formatLogMessage(reason, context));
return silenceErrors(window.showErrorMessage(reason.message), 'showError Resolver showErrorMessage').then(() => {});
}
return Promise.resolve();
}

/**
* Error Resolver - log an error message before resolving the promise to undefined.
* @param reason - the reason the promise was rejected.
* @param context - the create context of the promise - used to trace the origin of the promise
* @returns Promise - resolves to `undefined` if an error has occurred.
*/
export function logError(reason: unknown, context: string): Promise<void> {
if (isError(reason)) {
console.log('Error: context (%s): %o', context, reason);
if (!isPromiseCanceledError(reason) && isError(reason)) {
console.log(formatLogMessage(reason, context));
}
return Promise.resolve();
}

export function handleErrors<T>(p: Promise<T>, context: string, onErrorHandler: OnErrorHandler = onError): Promise<T | void> {
return handleErrorsEx(p, context, onErrorHandler);
/**
* Error Resolver - ignores any errors before resolving the promise to undefined.
* @param reason - the reason the promise was rejected.
* @param context - the create context of the promise - used to trace the origin of the promise
* @returns Promise - resolves to `undefined` if an error has occurred.
*/
export function ignoreError(reason: unknown, context: string): Promise<void> {
// The msg is generated for debugging purposes, it is not used.
const msg = formatLogMessage(reason, context);
// bogus logic so the compiler doesn't drop msg
return Promise.resolve(toUndefined(msg));
}

export function handleErrorsEx<T>(p: Promise<T> | (() => Promise<T>), context: string, onErrorHandler = onError): Promise<T | void> {
const q = typeof p === 'function' ? (async () => p())() : p;
return q.catch(withContextOnError(context, onErrorHandler));
export function handleErrors<T>(
promiseOrFn: Promise<T> | (() => T | Promise<T>),
context: string,
onErrorResolver: OnErrorResolver = showError
): Promise<T | void> {
const q = typeof promiseOrFn === 'function' ? (async () => promiseOrFn())() : promiseOrFn;
return q.catch(withContextOnError(context, onErrorResolver));
}

export type ErrorHandler<T> = (p: Promise<T>) => Promise<T | void>;
export type OnErrorHandler = (reason: unknown, context: string) => Promise<void>;
type _OnErrorHandler = (reason: unknown) => Promise<void>;
export type OnErrorResolver = (reason: unknown, context: string) => Promise<void>;
type _OnErrorResolver = (reason: unknown) => Promise<void>;

/**
* This is a wrapper function. It is designed to wrap another and catch any exceptions thrown by that function turning them in Promise<void>
* @param fn - function to be wrapped
* @param context - used to identify the context in which the function is being wrapped.
* @param onErrorResolver - Used to resolve the rejected promise (it can write to a log or pop up a message)
* @returns - a function with the same signature as `fn`
*/
export function catchErrors<P extends Array<any>, R>(
fn: (...p: P) => Promise<R> | R,
context: string,
onErrorHandler: OnErrorHandler = onError
onErrorResolver: OnErrorResolver = showError
): (...p: P) => Promise<R | void> {
return (...p) => handleErrors((async () => fn(...p))(), context, onErrorHandler);
return (...p) => handleErrors<R>(() => fn(...p), context, onErrorResolver);
}

export function logErrors<T>(p: Promise<T> | Thenable<T>, context: string): Promise<T | void> {
return Promise.resolve(p).catch(withContextOnError(context, logError));
export function logErrors<T>(promise: Promise<T> | Thenable<T>, context: string): Promise<T | void> {
return handleErrors(Promise.resolve(promise), context, logError);
}

export function silenceErrors<T>(p: Promise<T> | Thenable<T>, context: string): Promise<T | void> {
return Promise.resolve(p).catch(withContextOnError(context, logError));
export function silenceErrors<T>(promise: Promise<T> | Thenable<T>, context: string): Promise<T | void> {
return handleErrors(Promise.resolve(promise), context, ignoreError);
}

export function withContextOnError(context: string, onErrorHandler: OnErrorHandler): _OnErrorHandler {
export function showErrors<T>(promise: Promise<T> | Thenable<T>, context: string): Promise<T | void> {
return handleErrors(Promise.resolve(promise), context, showError);
}

function withContextOnError(context: string, onErrorResolver: OnErrorResolver): _OnErrorResolver {
return (reason: unknown) => {
return onErrorHandler(reason, context);
return onErrorResolver(reason, context);
};
}

function toUndefined<T>(_v: T) {
return undefined;
}

function formatLogMessage(reason: unknown, context: string) {
return format('Error: context (%s): %o', context, reason);
}

const canceledName = 'Canceled';

/**
* Checks if the given error is a promise in canceled state
* See: [vscode/errors.ts · microsoft/vscode](https://github.com/microsoft/vscode/blob/c15cb13a383dc9ff2dc0828152e374a6b9ecc2b3/src/vs/base/common/errors.ts#L143)
*/
function isPromiseCanceledError(error: any): boolean {
return error instanceof Error && error.name === canceledName && error.message === canceledName;
}

0 comments on commit 3bfe916

Please sign in to comment.