Skip to content

Commit bc4eb21

Browse files
author
Eric Snow
authored
Make sure we keep component init & activation separate. (#14713)
This is a quick cleanup related to the recent change with disposables in the component. Part of the problem was that we did not have a clear enough separation between component initialization and activation. This change makes that clear separation.
1 parent b1e0505 commit bc4eb21

64 files changed

Lines changed: 1302 additions & 1259 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎src/client/common/asyncDisposableRegistry.ts‎

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,26 +2,19 @@
22
// Licensed under the MIT License.
33
'use strict';
44
import { injectable } from 'inversify';
5-
import { IAsyncDisposable, IAsyncDisposableRegistry, IDisposable } from './types';
5+
import { IAsyncDisposableRegistry } from './types';
6+
import { Disposables, IDisposable } from './utils/resourceLifecycle';
67

78
// List of disposables that need to run a promise.
89
@injectable()
910
export class AsyncDisposableRegistry implements IAsyncDisposableRegistry {
10-
private _list: (IDisposable | IAsyncDisposable)[] = [];
11+
private readonly disposables = new Disposables();
1112

12-
public async dispose(): Promise<void> {
13-
const promises = this._list.map((l) => l.dispose());
14-
await Promise.all(promises);
15-
this._list = [];
16-
}
17-
18-
public push(disposable?: IDisposable | IAsyncDisposable) {
19-
if (disposable) {
20-
this._list.push(disposable);
21-
}
13+
public push(...disposables: IDisposable[]): void {
14+
this.disposables.push(...disposables);
2215
}
2316

24-
public get list(): (IDisposable | IAsyncDisposable)[] {
25-
return this._list;
17+
public async dispose(): Promise<void> {
18+
return this.disposables.dispose();
2619
}
2720
}

‎src/client/common/persistentState.ts‎

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,14 @@
55

66
import { inject, injectable, named } from 'inversify';
77
import { Memento } from 'vscode';
8-
import { GLOBAL_MEMENTO, IMemento, IPersistentState, IPersistentStateFactory, WORKSPACE_MEMENTO } from './types';
8+
import {
9+
GLOBAL_MEMENTO,
10+
IExtensionContext,
11+
IMemento,
12+
IPersistentState,
13+
IPersistentStateFactory,
14+
WORKSPACE_MEMENTO
15+
} from './types';
916

1017
export class PersistentState<T> implements IPersistentState<T> {
1118
constructor(
@@ -58,3 +65,28 @@ export class PersistentStateFactory implements IPersistentStateFactory {
5865
return new PersistentState<T>(this.workspaceState, key, defaultValue, expiryDurationMs);
5966
}
6067
}
68+
69+
/////////////////////////////
70+
// a simpler, alternate API
71+
// for components to use
72+
73+
interface IPersistentStorage<T> {
74+
get(): T | undefined;
75+
set(value: T): Promise<void>;
76+
}
77+
78+
/**
79+
* Build a global storage object for the given key.
80+
*/
81+
export function getGlobalStorage<T>(context: IExtensionContext, key: string): IPersistentStorage<T> {
82+
const raw = new PersistentState<T>(context.globalState, key);
83+
return {
84+
// We adapt between PersistentState and IPersistentStorage.
85+
get() {
86+
return raw.value;
87+
},
88+
set(value: T) {
89+
return raw.updateValue(value);
90+
}
91+
};
92+
}

‎src/client/common/platform/fileSystemWatcher.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,7 @@ import * as chokidar from 'chokidar';
77
import * as path from 'path';
88
import { RelativePattern, workspace } from 'vscode';
99
import { traceError, traceVerbose } from '../logger';
10-
import { DisposableRegistry } from '../syncDisposableRegistry';
11-
import { IDisposable } from '../types';
10+
import { Disposables, IDisposable } from '../utils/resourceLifecycle';
1211
import { normCasePath } from './fs-paths';
1312

1413
/**
@@ -41,7 +40,7 @@ function watchLocationUsingVSCodeAPI(
4140
callback: (type: FileChangeType, absPath: string) => void
4241
): IDisposable {
4342
const globPattern = new RelativePattern(baseDir, pattern);
44-
const disposables = new DisposableRegistry();
43+
const disposables = new Disposables();
4544
traceVerbose(`Start watching: ${baseDir} with pattern ${pattern} using VSCode API`);
4645
const watcher = workspace.createFileSystemWatcher(globPattern);
4746
disposables.push(watcher.onDidCreate((e) => callback(FileChangeType.Created, e.fsPath)));

‎src/client/common/syncDisposableRegistry.ts‎

Lines changed: 0 additions & 31 deletions
This file was deleted.

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Licensed under the MIT License.
33

44
import { createDeferred } from './async';
5+
import { IDisposable } from './resourceLifecycle';
56

67
type RequestID = number;
78
type RunFunc = () => Promise<void>;
@@ -13,7 +14,7 @@ type NotifyFunc = () => void;
1314
* The key aspect is that already running or queue requests can be
1415
* re-used instead of creating a duplicate request.
1516
*/
16-
export class BackgroundRequestLooper {
17+
export class BackgroundRequestLooper implements IDisposable {
1718
private readonly opts: {
1819
runDefault: RunFunc;
1920
};
@@ -51,6 +52,12 @@ export class BackgroundRequestLooper {
5152
};
5253
}
5354

55+
public async dispose(): Promise<void> {
56+
if (this.started) {
57+
await this.stop();
58+
}
59+
}
60+
5461
/**
5562
* Start the request execution loop.
5663
*

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
// tslint:disable:max-classes-per-file
5+
6+
import { logWarning } from '../../logging';
7+
8+
/**
9+
* An object that can be disposed, like vscode.Disposable.
10+
*/
11+
export interface IDisposable {
12+
dispose(): void | Promise<void>;
13+
}
14+
15+
/**
16+
* A registry of disposables.
17+
*/
18+
export interface IDisposables extends IDisposable {
19+
push(...disposable: IDisposable[]): void;
20+
}
21+
22+
/**
23+
* Safely dispose each of the disposables.
24+
*/
25+
export async function disposeAll(disposables: IDisposable[]): Promise<void> {
26+
await Promise.all(
27+
disposables.map(async (d, index) => {
28+
try {
29+
await d.dispose();
30+
} catch (err) {
31+
logWarning(`dispose() #${index} failed (${err})`);
32+
}
33+
})
34+
);
35+
}
36+
37+
/**
38+
* A list of disposables.
39+
*/
40+
export class Disposables implements IDisposables {
41+
private disposables: IDisposable[] = [];
42+
43+
constructor(...disposables: IDisposable[]) {
44+
this.disposables.push(...disposables);
45+
}
46+
47+
public push(...disposables: IDisposable[]) {
48+
this.disposables.push(...disposables);
49+
}
50+
51+
public async dispose(): Promise<void> {
52+
const disposables = this.disposables;
53+
this.disposables = [];
54+
await disposeAll(disposables);
55+
}
56+
}

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -239,11 +239,11 @@ class WorkerPool<T, R> implements IWorkerPool<T, R> {
239239
}
240240
}
241241

242-
export function createWorkerPool<T, R>(
242+
export function createRunningWorkerPool<T, R>(
243243
workerFunc: WorkFunc<T, R>,
244-
numWorkers: number = 2,
245-
name: string = 'Worker'
246-
): IWorkerPool<T, R> {
244+
numWorkers?: number,
245+
name?: string
246+
): WorkerPool<T, R> {
247247
const pool = new WorkerPool<T, R>(workerFunc, numWorkers, name);
248248
pool.start();
249249
return pool;

‎src/client/components.ts‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
4+
import { IDisposableRegistry, IExtensionContext } from './common/types';
5+
import { IServiceContainer, IServiceManager } from './ioc/types';
6+
7+
/**
8+
* The global extension state needed by components.
9+
*
10+
*/
11+
export type ExtensionState = {
12+
context: IExtensionContext;
13+
disposables: IDisposableRegistry;
14+
// For now we include the objects dealing with inversify (IOC)
15+
// registration. These will be removed later.
16+
legacyIOC: {
17+
serviceManager: IServiceManager;
18+
serviceContainer: IServiceContainer;
19+
};
20+
};
21+
22+
/**
23+
* The result of activating a component of the extension.
24+
*
25+
* Getting this value means the component has reached a state where it
26+
* may be used by the rest of the extension.
27+
*
28+
* If the component started any non-critical activation-related
29+
* operations during activation then the "fullyReady" property will only
30+
* resolve once all those operations complete.
31+
*
32+
* The component may have also started long-running background helpers.
33+
* Those are not exposed here.
34+
*/
35+
export type ActivationResult = {
36+
fullyReady: Promise<void>;
37+
};

‎src/client/extension.ts‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -99,20 +99,27 @@ async function activateUnsafe(
9999
//===============================================
100100
// activation starts here
101101

102-
const [serviceManager, serviceContainer] = initializeGlobals(context);
103-
activatedServiceContainer = serviceContainer;
104-
initializeCommon(context, serviceManager, serviceContainer);
105-
await initializeComponents(context, serviceManager, serviceContainer);
106-
const { activationPromise } = await activateComponents(context, serviceManager, serviceContainer);
102+
// First we initialize.
103+
const ext = initializeGlobals(context);
104+
activatedServiceContainer = ext.legacyIOC.serviceContainer;
105+
initializeCommon(ext);
106+
const components = initializeComponents(ext);
107+
108+
// Then we finish activating.
109+
const componentsActivated = await activateComponents(ext, components);
110+
const nonBlocking = componentsActivated.map((r) => r.fullyReady);
111+
const activationPromise = (async () => {
112+
await Promise.all(nonBlocking);
113+
})();
107114

108115
//===============================================
109116
// activation ends here
110117

111118
startupDurations.endActivateTime = startupStopWatch.elapsedTime;
112119
activationDeferred.resolve();
113120

114-
const api = buildApi(activationPromise, serviceManager, serviceContainer);
115-
return [api, activationPromise, serviceContainer];
121+
const api = buildApi(activationPromise, ext.legacyIOC.serviceManager, ext.legacyIOC.serviceContainer);
122+
return [api, activationPromise, ext.legacyIOC.serviceContainer];
116123
}
117124

118125
// tslint:disable-next-line:no-any

‎src/client/extensionActivation.ts‎

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import {
2525
IConfigurationService,
2626
IDisposableRegistry,
2727
IExperimentsManager,
28-
IExtensionContext,
2928
IFeatureDeprecationManager,
3029
IOutputChannel
3130
} from './common/types';
@@ -44,7 +43,6 @@ import {
4443
IInterpreterService
4544
} from './interpreter/contracts';
4645
import { registerTypes as interpretersRegisterTypes } from './interpreter/serviceRegistry';
47-
import { IServiceContainer, IServiceManager } from './ioc/types';
4846
import { getLanguageConfiguration } from './language/languageConfiguration';
4947
import { LinterCommands } from './linters/linterCommands';
5048
import { registerTypes as lintersRegisterTypes } from './linters/serviceRegistry';
@@ -65,14 +63,32 @@ import { ITestContextService } from './testing/common/types';
6563
import { ITestCodeNavigatorCommandHandler, ITestExplorerCommandHandler } from './testing/navigation/types';
6664
import { registerTypes as unitTestsRegisterTypes } from './testing/serviceRegistry';
6765

68-
export async function activateComponents(
69-
context: IExtensionContext,
70-
serviceManager: IServiceManager,
71-
serviceContainer: IServiceContainer
72-
) {
73-
// We will be pulling code over from activateLegacy().
66+
// components
67+
import * as pythonEnvironments from './pythonEnvironments';
68+
69+
import { ActivationResult, ExtensionState } from './components';
70+
import { Components } from './extensionInit';
7471

75-
return activateLegacy(context, serviceManager, serviceContainer);
72+
export async function activateComponents(
73+
// `ext` is passed to any extra activation funcs.
74+
ext: ExtensionState,
75+
components: Components
76+
): Promise<ActivationResult[]> {
77+
// Note that each activation returns a promise that resolves
78+
// when that activation completes. However, it might have started
79+
// some non-critical background operations that do not block
80+
// extension activation but do block use of the extension "API".
81+
// Each component activation can't just resolve an "inner" promise
82+
// for those non-critical operations because `await` (and
83+
// `Promise.all()`, etc.) will flatten nested promises. Thus
84+
// activation resolves `ActivationResult`, which can safely wrap
85+
// the "inner" promise.
86+
const promises: Promise<ActivationResult>[] = [
87+
pythonEnvironments.activate(components.pythonEnvs),
88+
// These will go away eventually.
89+
activateLegacy(ext)
90+
];
91+
return Promise.all(promises);
7692
}
7793

7894
/////////////////////////////
@@ -85,11 +101,10 @@ export async function activateComponents(
85101
// init and activation: move them to activateComponents().
86102
// See https://github.com/microsoft/vscode-python/issues/10454.
87103

88-
async function activateLegacy(
89-
context: IExtensionContext,
90-
serviceManager: IServiceManager,
91-
serviceContainer: IServiceContainer
92-
) {
104+
async function activateLegacy(ext: ExtensionState): Promise<ActivationResult> {
105+
const { context, legacyIOC } = ext;
106+
const { serviceManager, serviceContainer } = legacyIOC;
107+
93108
// register "services"
94109

95110
const standardOutputChannel = window.createOutputChannel(OutputChannelNames.python());
@@ -215,5 +230,5 @@ async function activateLegacy(
215230

216231
serviceContainer.get<IDebuggerBanner>(IDebuggerBanner).initialize();
217232

218-
return { activationPromise };
233+
return { fullyReady: activationPromise };
219234
}

0 commit comments

Comments
 (0)