Skip to content

Commit 3fd09e6

Browse files
jriekenjrieken
committed
send requests per provider so that a hanging provider doesn't block other providers, microsoft#100524 
1 parent de12505 commit 3fd09e6

4 files changed

Lines changed: 130 additions & 46 deletions

File tree

‎src/vs/workbench/api/browser/mainThreadDecorations.ts‎

Lines changed: 27 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -9,33 +9,33 @@ import { IDisposable, dispose } from 'vs/base/common/lifecycle';
99
import { ExtHostContext, MainContext, IExtHostContext, MainThreadDecorationsShape, ExtHostDecorationsShape, DecorationData, DecorationRequest } from '../common/extHost.protocol';
1010
import { extHostNamedCustomer } from 'vs/workbench/api/common/extHostCustomers';
1111
import { IDecorationsService, IDecorationData } from 'vs/workbench/services/decorations/browser/decorations';
12-
import { values } from 'vs/base/common/collections';
1312
import { CancellationToken } from 'vs/base/common/cancellation';
1413

1514
class DecorationRequestsQueue {
1615

1716
private _idPool = 0;
18-
private _requests: { [id: number]: DecorationRequest } = Object.create(null);
19-
private _resolver: { [id: number]: (data: DecorationData) => any } = Object.create(null);
17+
private _requests = new Map<number, DecorationRequest>();
18+
private _resolver = new Map<number, (data: DecorationData) => any>();
2019

2120
private _timer: any;
2221

2322
constructor(
24-
private readonly _proxy: ExtHostDecorationsShape
23+
private readonly _proxy: ExtHostDecorationsShape,
24+
private readonly _handle: number
2525
) {
2626
//
2727
}
2828

29-
enqueue(handle: number, uri: URI, token: CancellationToken): Promise<DecorationData> {
29+
enqueue(uri: URI, token: CancellationToken): Promise<DecorationData> {
3030
const id = ++this._idPool;
3131
const result = new Promise<DecorationData>(resolve => {
32-
this._requests[id] = { id, handle, uri };
33-
this._resolver[id] = resolve;
32+
this._requests.set(id, { id, uri });
33+
this._resolver.set(id, resolve);
3434
this._processQueue();
3535
});
3636
token.onCancellationRequested(() => {
37-
delete this._requests[id];
38-
delete this._resolver[id];
37+
this._requests.delete(id);
38+
this._resolver.delete(id);
3939
});
4040
return result;
4141
}
@@ -49,15 +49,15 @@ class DecorationRequestsQueue {
4949
// make request
5050
const requests = this._requests;
5151
const resolver = this._resolver;
52-
this._proxy.$provideDecorations(values(requests), CancellationToken.None).then(data => {
52+
this._proxy.$provideDecorations(this._handle, [...requests.values()], CancellationToken.None).then(data => {
5353
for (const id in resolver) {
54-
resolver[id](data[id]);
54+
resolver.get(Number(id))!(data[Number(id)]);
5555
}
5656
});
5757

5858
// reset
59-
this._requests = [];
60-
this._resolver = [];
59+
this._requests = new Map();
60+
this._resolver = new Map();
6161
this._timer = undefined;
6262
}, 0);
6363
}
@@ -68,14 +68,12 @@ export class MainThreadDecorations implements MainThreadDecorationsShape {
6868

6969
private readonly _provider = new Map<number, [Emitter<URI[]>, IDisposable]>();
7070
private readonly _proxy: ExtHostDecorationsShape;
71-
private readonly _requestQueue: DecorationRequestsQueue;
7271

7372
constructor(
7473
context: IExtHostContext,
7574
@IDecorationsService private readonly _decorationsService: IDecorationsService
7675
) {
7776
this._proxy = context.getProxy(ExtHostContext.ExtHostDecorations);
78-
this._requestQueue = new DecorationRequestsQueue(this._proxy);
7977
}
8078

8179
dispose() {
@@ -85,23 +83,23 @@ export class MainThreadDecorations implements MainThreadDecorationsShape {
8583

8684
$registerDecorationProvider(handle: number, label: string): void {
8785
const emitter = new Emitter<URI[]>();
86+
const queue = new DecorationRequestsQueue(this._proxy, handle);
8887
const registration = this._decorationsService.registerDecorationsProvider({
8988
label,
9089
onDidChange: emitter.event,
91-
provideDecorations: (uri, token) => {
92-
return this._requestQueue.enqueue(handle, uri, token).then(data => {
93-
if (!data) {
94-
return undefined;
95-
}
96-
const [weight, bubble, tooltip, letter, themeColor] = data;
97-
return <IDecorationData>{
98-
weight: weight || 0,
99-
bubble: bubble || false,
100-
color: themeColor && themeColor.id,
101-
tooltip,
102-
letter
103-
};
104-
});
90+
provideDecorations: async (uri, token) => {
91+
const data = await queue.enqueue(uri, token);
92+
if (!data) {
93+
return undefined;
94+
}
95+
const [weight, bubble, tooltip, letter, themeColor] = data;
96+
return <IDecorationData>{
97+
weight: weight ?? 0,
98+
bubble: bubble ?? false,
99+
color: themeColor?.id,
100+
tooltip,
101+
letter
102+
};
105103
}
106104
});
107105
this._provider.set(handle, [emitter, registration]);

‎src/vs/workbench/api/common/extHost.protocol.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1525,15 +1525,14 @@ export interface ExtHostDebugServiceShape {
15251525

15261526
export interface DecorationRequest {
15271527
readonly id: number;
1528-
readonly handle: number;
15291528
readonly uri: UriComponents;
15301529
}
15311530

15321531
export type DecorationData = [number, boolean, string, string, ThemeColor];
15331532
export type DecorationReply = { [id: number]: DecorationData; };
15341533

15351534
export interface ExtHostDecorationsShape {
1536-
$provideDecorations(requests: DecorationRequest[], token: CancellationToken): Promise<DecorationReply>;
1535+
$provideDecorations(handle: number, requests: DecorationRequest[], token: CancellationToken): Promise<DecorationReply>;
15371536
}
15381537

15391538
export interface ExtHostWindowShape {

‎src/vs/workbench/api/common/extHostDecorations.ts‎

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -52,17 +52,20 @@ export class ExtHostDecorations implements IExtHostDecorations {
5252
});
5353
}
5454

55-
$provideDecorations(requests: DecorationRequest[], token: CancellationToken): Promise<DecorationReply> {
55+
async $provideDecorations(handle: number, requests: DecorationRequest[], token: CancellationToken): Promise<DecorationReply> {
56+
57+
if (!this._provider.has(handle)) {
58+
// might have been unregistered in the meantime
59+
return Object.create(null);
60+
}
61+
5662
const result: DecorationReply = Object.create(null);
57-
return Promise.all(requests.map(request => {
58-
const { handle, uri, id } = request;
59-
const entry = this._provider.get(handle);
60-
if (!entry) {
61-
// might have been unregistered in the meantime
62-
return undefined;
63-
}
64-
const { provider, extensionId } = entry;
65-
return Promise.resolve(provider.provideDecoration(URI.revive(uri), token)).then(data => {
63+
const { provider, extensionId } = this._provider.get(handle)!;
64+
65+
await Promise.all(requests.map(async request => {
66+
try {
67+
const { uri, id } = request;
68+
const data = await Promise.resolve(provider.provideDecoration(URI.revive(uri), token));
6669
if (!data) {
6770
return;
6871
}
@@ -72,13 +75,12 @@ export class ExtHostDecorations implements IExtHostDecorations {
7275
} catch (e) {
7376
this._logService.warn(`INVALID decoration from extension '${extensionId.value}': ${e}`);
7477
}
75-
}, err => {
78+
} catch (err) {
7679
this._logService.error(err);
77-
});
80+
}
81+
}));
7882

79-
})).then(() => {
80-
return result;
81-
});
83+
return result;
8284
}
8385
}
8486

‎src/vs/workbench/test/browser/api/extHostDecorations.test.ts‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
* Licensed under the MIT License. See License.txt in the project root for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
import * as assert from 'assert';
7+
import { timeout } from 'vs/base/common/async';
8+
import { CancellationToken } from 'vs/base/common/cancellation';
9+
import { Event } from 'vs/base/common/event';
10+
import { URI } from 'vs/base/common/uri';
11+
import { mock } from 'vs/base/test/common/mock';
12+
import { NullLogService } from 'vs/platform/log/common/log';
13+
import { MainThreadDecorationsShape } from 'vs/workbench/api/common/extHost.protocol';
14+
import { ExtHostDecorations } from 'vs/workbench/api/common/extHostDecorations';
15+
import { IExtHostRpcService } from 'vs/workbench/api/common/extHostRpcService';
16+
import { nullExtensionDescription } from 'vs/workbench/services/extensions/common/extensions';
17+
18+
suite('ExtHostDecorations', function () {
19+
20+
let mainThreadShape: MainThreadDecorationsShape;
21+
let extHostDecorations: ExtHostDecorations;
22+
let providers = new Set<number>();
23+
24+
setup(function () {
25+
26+
providers.clear();
27+
28+
mainThreadShape = new class extends mock<MainThreadDecorationsShape>() {
29+
$registerDecorationProvider(handle: number) {
30+
providers.add(handle);
31+
}
32+
};
33+
34+
extHostDecorations = new ExtHostDecorations(
35+
new class extends mock<IExtHostRpcService>() {
36+
getProxy(): any {
37+
return mainThreadShape;
38+
}
39+
},
40+
new NullLogService()
41+
);
42+
});
43+
44+
test('SCM Decorations missing #100524', async function () {
45+
46+
let calledA = false;
47+
let calledB = false;
48+
49+
// never returns
50+
extHostDecorations.registerDecorationProvider({
51+
onDidChangeDecorations: Event.None,
52+
provideDecoration() {
53+
calledA = true;
54+
return new Promise(() => { });
55+
}
56+
}, nullExtensionDescription.identifier);
57+
58+
// always returns
59+
extHostDecorations.registerDecorationProvider({
60+
onDidChangeDecorations: Event.None,
61+
provideDecoration() {
62+
calledB = true;
63+
return new Promise(resolve => resolve({ letter: 'H', title: 'Hello' }));
64+
}
65+
}, nullExtensionDescription.identifier);
66+
67+
68+
const requests = [...providers.values()].map((handle, idx) => {
69+
return extHostDecorations.$provideDecorations(handle, [{ id: idx, uri: URI.parse('test:///file') }], CancellationToken.None);
70+
});
71+
72+
assert.equal(calledA, true);
73+
assert.equal(calledB, true);
74+
75+
assert.equal(requests.length, 2);
76+
const [first, second] = requests;
77+
78+
const firstResult = await Promise.race([first, timeout(30).then(() => false)]);
79+
assert.equal(typeof firstResult, 'boolean'); // never finishes...
80+
81+
const secondResult = await Promise.race([second, timeout(30).then(() => false)]);
82+
assert.equal(typeof secondResult, 'object');
83+
});
84+
85+
});

0 commit comments

Comments
 (0)