diff --git a/src/vs/workbench/api/browser/mainThreadDecorations.ts b/src/vs/workbench/api/browser/mainThreadDecorations.ts index 08d46a9840595..059479044ba1e 100644 --- a/src/vs/workbench/api/browser/mainThreadDecorations.ts +++ b/src/vs/workbench/api/browser/mainThreadDecorations.ts @@ -9,33 +9,33 @@ import { IDisposable, dispose } from 'vs/base/common/lifecycle'; import { ExtHostContext, MainContext, IExtHostContext, MainThreadDecorationsShape, ExtHostDecorationsShape, DecorationData, DecorationRequest } from '../common/extHost.protocol'; import { extHostNamedCustomer } from 'vs/workbench/api/common/extHostCustomers'; import { IDecorationsService, IDecorationData } from 'vs/workbench/services/decorations/browser/decorations'; -import { values } from 'vs/base/common/collections'; import { CancellationToken } from 'vs/base/common/cancellation'; class DecorationRequestsQueue { private _idPool = 0; - private _requests: { [id: number]: DecorationRequest } = Object.create(null); - private _resolver: { [id: number]: (data: DecorationData) => any } = Object.create(null); + private _requests = new Map(); + private _resolver = new Map any>(); private _timer: any; constructor( - private readonly _proxy: ExtHostDecorationsShape + private readonly _proxy: ExtHostDecorationsShape, + private readonly _handle: number ) { // } - enqueue(handle: number, uri: URI, token: CancellationToken): Promise { + enqueue(uri: URI, token: CancellationToken): Promise { const id = ++this._idPool; const result = new Promise(resolve => { - this._requests[id] = { id, handle, uri }; - this._resolver[id] = resolve; + this._requests.set(id, { id, uri }); + this._resolver.set(id, resolve); this._processQueue(); }); token.onCancellationRequested(() => { - delete this._requests[id]; - delete this._resolver[id]; + this._requests.delete(id); + this._resolver.delete(id); }); return result; } @@ -49,15 +49,15 @@ class DecorationRequestsQueue { // make request const requests = this._requests; const resolver = this._resolver; - this._proxy.$provideDecorations(values(requests), CancellationToken.None).then(data => { + this._proxy.$provideDecorations(this._handle, [...requests.values()], CancellationToken.None).then(data => { for (const id in resolver) { - resolver[id](data[id]); + resolver.get(Number(id))!(data[Number(id)]); } }); // reset - this._requests = []; - this._resolver = []; + this._requests = new Map(); + this._resolver = new Map(); this._timer = undefined; }, 0); } @@ -68,14 +68,12 @@ export class MainThreadDecorations implements MainThreadDecorationsShape { private readonly _provider = new Map, IDisposable]>(); private readonly _proxy: ExtHostDecorationsShape; - private readonly _requestQueue: DecorationRequestsQueue; constructor( context: IExtHostContext, @IDecorationsService private readonly _decorationsService: IDecorationsService ) { this._proxy = context.getProxy(ExtHostContext.ExtHostDecorations); - this._requestQueue = new DecorationRequestsQueue(this._proxy); } dispose() { @@ -85,23 +83,23 @@ export class MainThreadDecorations implements MainThreadDecorationsShape { $registerDecorationProvider(handle: number, label: string): void { const emitter = new Emitter(); + const queue = new DecorationRequestsQueue(this._proxy, handle); const registration = this._decorationsService.registerDecorationsProvider({ label, onDidChange: emitter.event, - provideDecorations: (uri, token) => { - return this._requestQueue.enqueue(handle, uri, token).then(data => { - if (!data) { - return undefined; - } - const [weight, bubble, tooltip, letter, themeColor] = data; - return { - weight: weight || 0, - bubble: bubble || false, - color: themeColor && themeColor.id, - tooltip, - letter - }; - }); + provideDecorations: async (uri, token) => { + const data = await queue.enqueue(uri, token); + if (!data) { + return undefined; + } + const [weight, bubble, tooltip, letter, themeColor] = data; + return { + weight: weight ?? 0, + bubble: bubble ?? false, + color: themeColor?.id, + tooltip, + letter + }; } }); this._provider.set(handle, [emitter, registration]); diff --git a/src/vs/workbench/api/common/extHost.protocol.ts b/src/vs/workbench/api/common/extHost.protocol.ts index 6f838767c6839..3e8649520e6b9 100644 --- a/src/vs/workbench/api/common/extHost.protocol.ts +++ b/src/vs/workbench/api/common/extHost.protocol.ts @@ -1525,7 +1525,6 @@ export interface ExtHostDebugServiceShape { export interface DecorationRequest { readonly id: number; - readonly handle: number; readonly uri: UriComponents; } @@ -1533,7 +1532,7 @@ export type DecorationData = [number, boolean, string, string, ThemeColor]; export type DecorationReply = { [id: number]: DecorationData; }; export interface ExtHostDecorationsShape { - $provideDecorations(requests: DecorationRequest[], token: CancellationToken): Promise; + $provideDecorations(handle: number, requests: DecorationRequest[], token: CancellationToken): Promise; } export interface ExtHostWindowShape { diff --git a/src/vs/workbench/api/common/extHostDecorations.ts b/src/vs/workbench/api/common/extHostDecorations.ts index 9ed26739e6dea..d50d8f15dd264 100644 --- a/src/vs/workbench/api/common/extHostDecorations.ts +++ b/src/vs/workbench/api/common/extHostDecorations.ts @@ -52,17 +52,20 @@ export class ExtHostDecorations implements IExtHostDecorations { }); } - $provideDecorations(requests: DecorationRequest[], token: CancellationToken): Promise { + async $provideDecorations(handle: number, requests: DecorationRequest[], token: CancellationToken): Promise { + + if (!this._provider.has(handle)) { + // might have been unregistered in the meantime + return Object.create(null); + } + const result: DecorationReply = Object.create(null); - return Promise.all(requests.map(request => { - const { handle, uri, id } = request; - const entry = this._provider.get(handle); - if (!entry) { - // might have been unregistered in the meantime - return undefined; - } - const { provider, extensionId } = entry; - return Promise.resolve(provider.provideDecoration(URI.revive(uri), token)).then(data => { + const { provider, extensionId } = this._provider.get(handle)!; + + await Promise.all(requests.map(async request => { + try { + const { uri, id } = request; + const data = await Promise.resolve(provider.provideDecoration(URI.revive(uri), token)); if (!data) { return; } @@ -72,13 +75,12 @@ export class ExtHostDecorations implements IExtHostDecorations { } catch (e) { this._logService.warn(`INVALID decoration from extension '${extensionId.value}': ${e}`); } - }, err => { + } catch (err) { this._logService.error(err); - }); + } + })); - })).then(() => { - return result; - }); + return result; } } diff --git a/src/vs/workbench/test/browser/api/extHostDecorations.test.ts b/src/vs/workbench/test/browser/api/extHostDecorations.test.ts new file mode 100644 index 0000000000000..073006e636dcd --- /dev/null +++ b/src/vs/workbench/test/browser/api/extHostDecorations.test.ts @@ -0,0 +1,85 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import * as assert from 'assert'; +import { timeout } from 'vs/base/common/async'; +import { CancellationToken } from 'vs/base/common/cancellation'; +import { Event } from 'vs/base/common/event'; +import { URI } from 'vs/base/common/uri'; +import { mock } from 'vs/base/test/common/mock'; +import { NullLogService } from 'vs/platform/log/common/log'; +import { MainThreadDecorationsShape } from 'vs/workbench/api/common/extHost.protocol'; +import { ExtHostDecorations } from 'vs/workbench/api/common/extHostDecorations'; +import { IExtHostRpcService } from 'vs/workbench/api/common/extHostRpcService'; +import { nullExtensionDescription } from 'vs/workbench/services/extensions/common/extensions'; + +suite('ExtHostDecorations', function () { + + let mainThreadShape: MainThreadDecorationsShape; + let extHostDecorations: ExtHostDecorations; + let providers = new Set(); + + setup(function () { + + providers.clear(); + + mainThreadShape = new class extends mock() { + $registerDecorationProvider(handle: number) { + providers.add(handle); + } + }; + + extHostDecorations = new ExtHostDecorations( + new class extends mock() { + getProxy(): any { + return mainThreadShape; + } + }, + new NullLogService() + ); + }); + + test('SCM Decorations missing #100524', async function () { + + let calledA = false; + let calledB = false; + + // never returns + extHostDecorations.registerDecorationProvider({ + onDidChangeDecorations: Event.None, + provideDecoration() { + calledA = true; + return new Promise(() => { }); + } + }, nullExtensionDescription.identifier); + + // always returns + extHostDecorations.registerDecorationProvider({ + onDidChangeDecorations: Event.None, + provideDecoration() { + calledB = true; + return new Promise(resolve => resolve({ letter: 'H', title: 'Hello' })); + } + }, nullExtensionDescription.identifier); + + + const requests = [...providers.values()].map((handle, idx) => { + return extHostDecorations.$provideDecorations(handle, [{ id: idx, uri: URI.parse('test:///file') }], CancellationToken.None); + }); + + assert.equal(calledA, true); + assert.equal(calledB, true); + + assert.equal(requests.length, 2); + const [first, second] = requests; + + const firstResult = await Promise.race([first, timeout(30).then(() => false)]); + assert.equal(typeof firstResult, 'boolean'); // never finishes... + + const secondResult = await Promise.race([second, timeout(30).then(() => false)]); + assert.equal(typeof secondResult, 'object'); + }); + +});