Skip to content

Commit 6abee0f

Browse files
DonJayamanneDonJayamanne
authored
Ensure debounce decorator for async methods returns a promise (#5051)
For #5050
1 parent f5e42c8 commit 6abee0f

5 files changed

Lines changed: 189 additions & 19 deletions

File tree

‎src/client/activation/languageServer/analysisOptions.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import { IWorkspaceService } from '../../common/application/types';
1111
import { isTestExecution, PYTHON_LANGUAGE, STANDARD_OUTPUT_CHANNEL } from '../../common/constants';
1212
import { traceDecorators, traceError } from '../../common/logger';
1313
import { BANNER_NAME_PROPOSE_LS, IConfigurationService, IExtensionContext, IOutputChannel, IPathUtils, IPythonExtensionBanner, Resource } from '../../common/types';
14-
import { debounce } from '../../common/utils/decorators';
14+
import { debounceSync } from '../../common/utils/decorators';
1515
import { IEnvironmentVariablesProvider } from '../../common/variables/types';
1616
import { IInterpreterService } from '../../interpreter/contracts';
1717
import { ILanguageServerAnalysisOptions, ILanguageServerFolderService } from '../types';
@@ -186,7 +186,7 @@ export class LanguageServerAnalysisOptions implements ILanguageServerAnalysisOpt
186186
}
187187
this.onSettingsChanged();
188188
}
189-
@debounce(1000)
189+
@debounceSync(1000)
190190
protected onSettingsChanged(): void {
191191
this.notifyIfSettingsChanged().ignoreErrors();
192192
}
@@ -213,7 +213,7 @@ export class LanguageServerAnalysisOptions implements ILanguageServerAnalysisOpt
213213
}
214214
}
215215

216-
@debounce(1000)
216+
@debounceSync(1000)
217217
protected onEnvVarChange(): void {
218218
this.notifyifEnvPythonPathChanged().ignoreErrors();
219219
}

‎src/client/activation/languageServer/manager.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { inject, injectable } from 'inversify';
77
import '../../common/extensions';
88
import { traceDecorators } from '../../common/logger';
99
import { IDisposable, Resource } from '../../common/types';
10-
import { debounce } from '../../common/utils/decorators';
10+
import { debounceSync } from '../../common/utils/decorators';
1111
import { IServiceContainer } from '../../ioc/types';
1212
import { captureTelemetry } from '../../telemetry';
1313
import { EventName } from '../../telemetry/constants';
@@ -49,7 +49,7 @@ export class LanguageServerManager implements ILanguageServerManager {
4949
this.languageServer.loadExtension(this.lsExtension.loadExtensionArgs);
5050
}
5151
}
52-
@debounce(1000)
52+
@debounceSync(1000)
5353
protected restartLanguageServerDebounced(): void {
5454
this.restartLanguageServer().ignoreErrors();
5555
}

‎src/client/common/configSettings.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import {
2323
IUnitTestSettings,
2424
IWorkspaceSymbolSettings
2525
} from './types';
26-
import { debounce } from './utils/decorators';
26+
import { debounceSync } from './utils/decorators';
2727
import { SystemVariables } from './variables/systemVariables';
2828

2929
// tslint:disable:no-require-imports no-var-requires
@@ -409,7 +409,7 @@ export class PythonSettings implements IPythonSettings {
409409
this.update(initialConfig);
410410
}
411411
}
412-
@debounce(1)
412+
@debounceSync(1)
413413
protected debounceChangeNotification() {
414414
this.changed.fire();
415415
}

‎src/client/common/utils/decorators.ts‎

Lines changed: 65 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,14 @@ import '../../common/extensions';
55
import { isTestExecution } from '../constants';
66
import { traceError, traceVerbose } from '../logger';
77
import { Resource } from '../types';
8+
import { createDeferred, Deferred } from './async';
89
import { InMemoryInterpreterSpecificCache } from './cacheUtils';
910

1011
// tslint:disable-next-line:no-require-imports no-var-requires
1112
const _debounce = require('lodash/debounce') as typeof import('lodash/debounce');
1213

13-
type VoidFunction = (...any: any[]) => void;
14-
type AsyncVoidFunction = (...any: any[]) => Promise<void>;
14+
type VoidFunction = () => any;
15+
type AsyncVoidFunction = () => Promise<any>;
1516

1617
/**
1718
* Combine multiple sequential calls to the decorated function into one.
@@ -23,23 +24,39 @@ type AsyncVoidFunction = (...any: any[]) => Promise<void>;
2324
* only in a single actual call. Following the most recent call to
2425
* the debounced function, debouncing resets after the "wait" interval
2526
* has elapsed.
26-
*
27-
* The decorated function must return either a void or a promise that
28-
* resolves to a void.
2927
*/
30-
export function debounce(wait?: number) {
28+
export function debounceSync(wait?: number) {
3129
if (isTestExecution()) {
32-
// If running tests, lets not debounce (so tests run fast).
30+
// If running tests, lets debounce until the next cycle in the event loop.
31+
// Same as `setTimeout(()=> {}, 0);` with a value of `0`.
3332
wait = undefined;
34-
// tslint:disable-next-line:no-suspicious-comment
35-
// TODO: We should be able to return a noop decorator instead...
3633
}
3734
return makeDebounceDecorator(wait);
3835
}
3936

37+
/**
38+
* Combine multiple sequential calls to the decorated async function into one.
39+
* @export
40+
* @param {number} [wait] Wait time (milliseconds).
41+
* @returns void
42+
*
43+
* The point is to ensure that successive calls to the function result
44+
* only in a single actual call. Following the most recent call to
45+
* the debounced function, debouncing resets after the "wait" interval
46+
* has elapsed.
47+
*/
48+
export function debounceAsync(wait?: number) {
49+
if (isTestExecution()) {
50+
// If running tests, lets debounce until the next cycle in the event loop.
51+
// Same as `setTimeout(()=> {}, 0);` with a value of `0`.
52+
wait = undefined;
53+
}
54+
return makeDebounceAsyncDecorator(wait);
55+
}
56+
4057
export function makeDebounceDecorator(wait?: number) {
4158
// tslint:disable-next-line:no-any no-function-expression
42-
return function (_target: any, _propertyName: string, descriptor: TypedPropertyDescriptor<VoidFunction> | TypedPropertyDescriptor<AsyncVoidFunction>) {
59+
return function (_target: any, _propertyName: string, descriptor: TypedPropertyDescriptor<VoidFunction>) {
4360
// We could also make use of _debounce() options. For instance,
4461
// the following causes the original method to be called
4562
// immediately:
@@ -64,6 +81,44 @@ export function makeDebounceDecorator(wait?: number) {
6481
};
6582
}
6683

84+
export function makeDebounceAsyncDecorator(wait?: number) {
85+
// tslint:disable-next-line:no-any no-function-expression
86+
return function (_target: any, _propertyName: string, descriptor: TypedPropertyDescriptor<AsyncVoidFunction>) {
87+
type StateInformation = { started: boolean; deferred: Deferred<any> | undefined; timer: number | undefined };
88+
const originalMethod = descriptor.value!;
89+
const state: StateInformation = { started: false, deferred: undefined, timer: undefined };
90+
91+
// Lets defer execution using a setTimeout for the given time.
92+
(descriptor as any).value = function (this: any) {
93+
const existingDeferred: Deferred<any> | undefined = state.deferred;
94+
if (existingDeferred && state.started) {
95+
return existingDeferred.promise;
96+
}
97+
98+
// Clear previous timer.
99+
const existingDeferredCompleted = (existingDeferred && existingDeferred.completed);
100+
const deferred = state.deferred = (!existingDeferred || existingDeferredCompleted) ? createDeferred<any>() : existingDeferred;
101+
if (state.timer) {
102+
clearTimeout(state.timer);
103+
}
104+
105+
state.timer = setTimeout(async () => {
106+
state.started = true;
107+
originalMethod.apply(this)
108+
.then(r => {
109+
state.started = false;
110+
deferred.resolve(r);
111+
})
112+
.catch(ex => {
113+
state.started = false;
114+
deferred.reject(ex);
115+
});
116+
}, wait);
117+
return deferred.promise;
118+
};
119+
};
120+
}
121+
67122
type VSCodeType = typeof import('vscode');
68123
type PromiseFunctionWithFirstArgOfResource = (...any: [Uri | undefined, ...any[]]) => Promise<any>;
69124

‎src/test/common/utils/decorators.unit.test.ts‎

Lines changed: 117 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { Uri } from 'vscode';
88
import { Resource } from '../../../client/common/types';
99
import { clearCache } from '../../../client/common/utils/cacheUtils';
1010
import {
11-
cacheResourceSpecificInterpreterData, makeDebounceDecorator
11+
cacheResourceSpecificInterpreterData, makeDebounceAsyncDecorator, makeDebounceDecorator
1212
} from '../../../client/common/utils/decorators';
1313
import { sleep } from '../../core';
1414

@@ -151,11 +151,33 @@ suite('Common Utils - Decorators', () => {
151151
expect(one.calls).to.deep.equal(['run']);
152152
expect(one.timestamps).to.have.lengthOf(one.calls.length);
153153
});
154+
test('Debounce: one async call & no wait', async () => {
155+
const wait = 100;
156+
// tslint:disable-next-line:max-classes-per-file
157+
class One extends Base {
158+
@makeDebounceAsyncDecorator(wait)
159+
public async run(): Promise<void> {
160+
this._addCall('run');
161+
}
162+
}
163+
const one = new One();
164+
165+
const start = Date.now();
166+
let errored = false;
167+
one.run().catch(() => errored = true);
168+
await waitForCalls(one.timestamps, 1);
169+
const delay = one.timestamps[0] - start;
170+
171+
expect(delay).to.be.at.least(wait);
172+
expect(one.calls).to.deep.equal(['run']);
173+
expect(one.timestamps).to.have.lengthOf(one.calls.length);
174+
expect(errored).to.be.equal(false, 'Exception raised when there shouldn\'t have been any');
175+
});
154176
test('Debounce: one async call', async () => {
155177
const wait = 100;
156178
// tslint:disable-next-line:max-classes-per-file
157179
class One extends Base {
158-
@makeDebounceDecorator(wait)
180+
@makeDebounceAsyncDecorator(wait)
159181
public async run(): Promise<void> {
160182
this._addCall('run');
161183
}
@@ -171,6 +193,99 @@ suite('Common Utils - Decorators', () => {
171193
expect(one.calls).to.deep.equal(['run']);
172194
expect(one.timestamps).to.have.lengthOf(one.calls.length);
173195
});
196+
test('Debounce: one async call and ensure exceptions are re-thrown', async () => {
197+
const wait = 100;
198+
// tslint:disable-next-line:max-classes-per-file
199+
class One extends Base {
200+
@makeDebounceAsyncDecorator(wait)
201+
public async run(): Promise<void> {
202+
this._addCall('run');
203+
throw new Error('Kaboom');
204+
}
205+
}
206+
const one = new One();
207+
208+
const start = Date.now();
209+
let capturedEx: Error | undefined;
210+
await one.run().catch(ex => capturedEx = ex);
211+
await waitForCalls(one.timestamps, 1);
212+
const delay = one.timestamps[0] - start;
213+
214+
expect(delay).to.be.at.least(wait);
215+
expect(one.calls).to.deep.equal(['run']);
216+
expect(one.timestamps).to.have.lengthOf(one.calls.length);
217+
expect(capturedEx).to.not.be.equal(undefined, 'Exception not re-thrown');
218+
});
219+
test('Debounce: multiple async calls', async () => {
220+
const wait = 100;
221+
// tslint:disable-next-line:max-classes-per-file
222+
class One extends Base {
223+
@makeDebounceAsyncDecorator(wait)
224+
public async run(): Promise<void> {
225+
this._addCall('run');
226+
}
227+
}
228+
const one = new One();
229+
230+
const start = Date.now();
231+
let errored = false;
232+
one.run().catch(() => errored = true);
233+
one.run().catch(() => errored = true);
234+
one.run().catch(() => errored = true);
235+
one.run().catch(() => errored = true);
236+
await waitForCalls(one.timestamps, 1);
237+
const delay = one.timestamps[0] - start;
238+
239+
expect(delay).to.be.at.least(wait);
240+
expect(one.calls).to.deep.equal(['run']);
241+
expect(one.timestamps).to.have.lengthOf(one.calls.length);
242+
expect(errored).to.be.equal(false, 'Exception raised when there shouldn\'t have been any');
243+
});
244+
test('Debounce: multiple async calls when awaiting on all', async () => {
245+
const wait = 100;
246+
// tslint:disable-next-line:max-classes-per-file
247+
class One extends Base {
248+
@makeDebounceAsyncDecorator(wait)
249+
public async run(): Promise<void> {
250+
this._addCall('run');
251+
}
252+
}
253+
const one = new One();
254+
255+
const start = Date.now();
256+
await Promise.all([one.run(), one.run(), one.run(), one.run()]);
257+
await waitForCalls(one.timestamps, 1);
258+
const delay = one.timestamps[0] - start;
259+
260+
expect(delay).to.be.at.least(wait);
261+
expect(one.calls).to.deep.equal(['run']);
262+
expect(one.timestamps).to.have.lengthOf(one.calls.length);
263+
});
264+
test('Debounce: multiple async calls & wait on some', async () => {
265+
const wait = 100;
266+
// tslint:disable-next-line:max-classes-per-file
267+
class One extends Base {
268+
@makeDebounceAsyncDecorator(wait)
269+
public async run(): Promise<void> {
270+
this._addCall('run');
271+
}
272+
}
273+
const one = new One();
274+
275+
const start = Date.now();
276+
let errored = false;
277+
one.run().catch(() => errored = true);
278+
await one.run();
279+
one.run().catch(() => errored = true);
280+
one.run().catch(() => errored = true);
281+
await waitForCalls(one.timestamps, 2);
282+
const delay = one.timestamps[1] - start;
283+
284+
expect(delay).to.be.at.least(wait);
285+
expect(one.calls).to.deep.equal(['run', 'run']);
286+
expect(one.timestamps).to.have.lengthOf(one.calls.length);
287+
expect(errored).to.be.equal(false, 'Exception raised when there shouldn\'t have been any');
288+
});
174289
test('Debounce: multiple calls grouped', async () => {
175290
const wait = 100;
176291
// tslint:disable-next-line:max-classes-per-file

0 commit comments

Comments
 (0)