Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

debt: lazy creation of services in config #160082

Merged
merged 4 commits into from Sep 5, 2022
Merged

debt: lazy creation of services in config #160082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3de1f2e
debt: lazy creation of services in config
sandy081 Sep 5, 2022
bf8b046
spell check
sandy081 Sep 5, 2022
648a6ab
use same instance of ConfigurationEditing
sandy081 Sep 5, 2022
1aa855e
fix unit tests
sandy081 Sep 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/vs/workbench/services/configuration/browser/configuration.ts
View file
Open in desktop
Expand Up @@ -13,7 +13,6 @@ import { ConfigurationModel, ConfigurationModelParser, ConfigurationParseOptions
import { WorkspaceConfigurationModelParser, StandaloneConfigurationModelParser } from 'vs/workbench/services/configuration/common/configurationModels';
import { TASKS_CONFIGURATION_KEY, FOLDER_SETTINGS_NAME, LAUNCH_CONFIGURATION_KEY, IConfigurationCache, ConfigurationKey, REMOTE_MACHINE_SCOPES, FOLDER_SCOPES, WORKSPACE_SCOPES } from 'vs/workbench/services/configuration/common/configuration';
import { IStoredWorkspaceFolder } from 'vs/platform/workspaces/common/workspaces';
import { JSONEditingService } from 'vs/workbench/services/configuration/common/jsonEditingService';
import { WorkbenchState, IWorkspaceFolder, IWorkspaceIdentifier } from 'vs/platform/workspace/common/workspace';
import { ConfigurationScope, Extensions, IConfigurationRegistry, OVERRIDE_PROPERTY_REGEX } from 'vs/platform/configuration/common/configurationRegistry';
import { equals } from 'vs/base/common/objects';
Expand All @@ -27,6 +26,7 @@ import { Registry } from 'vs/platform/registry/common/platform';
import { IBrowserWorkbenchEnvironmentService } from 'vs/workbench/services/environment/browser/environmentService';
import { isObject } from 'vs/base/common/types';
import { DefaultConfiguration as BaseDefaultConfiguration } from 'vs/platform/configuration/common/configurations';
import { IJSONEditingService } from 'vs/workbench/services/configuration/common/jsonEditing';

export class DefaultConfiguration extends BaseDefaultConfiguration {

Expand Down Expand Up @@ -629,7 +629,7 @@ export class WorkspaceConfiguration extends Disposable {
return this._workspaceConfiguration.getFolders();
}

setFolders(folders: IStoredWorkspaceFolder[], jsonEditingService: JSONEditingService): Promise<void> {
setFolders(folders: IStoredWorkspaceFolder[], jsonEditingService: IJSONEditingService): Promise<void> {
if (this._workspaceIdentifier) {
return jsonEditingService.write(this._workspaceIdentifier.configPath, [{ path: ['folders'], value: folders }], true)
.then(() => this.reload());
Expand Down
39 changes: 18 additions & 21 deletions src/vs/workbench/services/configuration/browser/configurationService.ts
View file
Open in desktop
Expand Up @@ -20,9 +20,8 @@ import { Registry } from 'vs/platform/registry/common/platform';
import { IConfigurationRegistry, Extensions, allSettings, windowSettings, resourceSettings, applicationSettings, machineSettings, machineOverridableSettings, ConfigurationScope, IConfigurationPropertySchema, keyFromOverrideIdentifiers, OVERRIDE_PROPERTY_PATTERN, resourceLanguageSettingsSchemaId, configurationDefaultsSchemaId } from 'vs/platform/configuration/common/configurationRegistry';
import { IStoredWorkspaceFolder, isStoredWorkspaceFolder, IWorkspaceFolderCreationData, getStoredWorkspaceFolder, toWorkspaceFolders } from 'vs/platform/workspaces/common/workspaces';
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
import { ConfigurationEditingService, EditableConfigurationTarget } from 'vs/workbench/services/configuration/common/configurationEditingService';
import { ConfigurationEditing, EditableConfigurationTarget } from 'vs/workbench/services/configuration/common/configurationEditing';
import { WorkspaceConfiguration, FolderConfiguration, RemoteUserConfiguration, UserConfiguration, DefaultConfiguration } from 'vs/workbench/services/configuration/browser/configuration';
import { JSONEditingService } from 'vs/workbench/services/configuration/common/jsonEditingService';
import { IJSONSchema, IJSONSchemaMap } from 'vs/base/common/jsonSchema';
import { mark } from 'vs/base/common/performance';
import { IRemoteAgentService } from 'vs/workbench/services/remote/common/remoteAgentService';
Expand All @@ -45,6 +44,7 @@ import { IPolicyService, NullPolicyService } from 'vs/platform/policy/common/pol
import { IUserDataProfile, IUserDataProfilesService } from 'vs/platform/userDataProfile/common/userDataProfile';
import { updateIgnoredSettings } from 'vs/platform/userDataSync/common/settingsMerge';
import { VSBuffer } from 'vs/base/common/buffer';
import { IJSONEditingService } from 'vs/workbench/services/configuration/common/jsonEditing';

function getLocalUserConfigurationScopes(userDataProfile: IUserDataProfile, hasRemote: boolean): ConfigurationScope[] | undefined {
return userDataProfile.isDefault
Expand Down Expand Up @@ -100,19 +100,16 @@ export class WorkspaceService extends Disposable implements IWorkbenchConfigurat

private readonly configurationRegistry: IConfigurationRegistry;

// TODO@sandeep debt with cyclic dependencies
private configurationEditingService!: ConfigurationEditingService;
private jsonEditingService!: JSONEditingService;
private cyclicDependencyReady!: Function;
private cyclicDependency = new Promise<void>(resolve => this.cyclicDependencyReady = resolve);
private instantiationService: IInstantiationService | undefined;
private configurationEditing: ConfigurationEditing | undefined;

constructor(
{ remoteAuthority, configurationCache }: { remoteAuthority?: string; configurationCache: IConfigurationCache },
environmentService: IWorkbenchEnvironmentService,
private readonly userDataProfileService: IUserDataProfileService,
private readonly userDataProfilesService: IUserDataProfilesService,
private readonly fileService: IFileService,
remoteAgentService: IRemoteAgentService,
private readonly remoteAgentService: IRemoteAgentService,
private readonly uriIdentityService: IUriIdentityService,
private readonly logService: ILogService,
policyService: IPolicyService
Expand Down Expand Up @@ -207,7 +204,6 @@ export class WorkspaceService extends Disposable implements IWorkbenchConfigurat
}

public async updateFolders(foldersToAdd: IWorkspaceFolderCreationData[], foldersToRemove: URI[], index?: number): Promise<void> {
await this.cyclicDependency;
return this.workspaceEditingQueue.queue(() => this.doUpdateFolders(foldersToAdd, foldersToRemove, index));
}

Expand Down Expand Up @@ -303,8 +299,11 @@ export class WorkspaceService extends Disposable implements IWorkbenchConfigurat
}

private async setFolders(folders: IStoredWorkspaceFolder[]): Promise<void> {
await this.cyclicDependency;
await this.workspaceConfiguration.setFolders(folders, this.jsonEditingService);
if (!this.instantiationService) {
throw new Error('Cannot update workspace folders because workspace service is not yet ready to accept writes.');
}

await this.instantiationService.invokeFunction(accessor => this.workspaceConfiguration.setFolders(folders, accessor.get(IJSONEditingService)));
return this.onWorkspaceConfigurationChanged(false);
}

Expand Down Expand Up @@ -333,7 +332,6 @@ export class WorkspaceService extends Disposable implements IWorkbenchConfigurat
updateValue(key: string, value: any, target: ConfigurationTarget): Promise<void>;
updateValue(key: string, value: any, overrides: IConfigurationOverrides | IConfigurationUpdateOverrides, target: ConfigurationTarget, donotNotifyError?: boolean): Promise<void>;
async updateValue(key: string, value: any, arg3?: any, arg4?: any, donotNotifyError?: any): Promise<void> {
await this.cyclicDependency;
const overrides: IConfigurationUpdateOverrides | undefined = isConfigurationUpdateOverrides(arg3) ? arg3
: isConfigurationOverrides(arg3) ? { resource: arg3.resource, overrideIdentifiers: arg3.overrideIdentifier ? [arg3.overrideIdentifier] : undefined } : undefined;
const target: ConfigurationTarget | undefined = overrides ? arg4 : arg3;
Expand Down Expand Up @@ -482,14 +480,7 @@ export class WorkspaceService extends Disposable implements IWorkbenchConfigurat
}

acquireInstantiationService(instantiationService: IInstantiationService): void {
this.configurationEditingService = instantiationService.createInstance(ConfigurationEditingService);
this.jsonEditingService = instantiationService.createInstance(JSONEditingService);

if (this.cyclicDependencyReady) {
this.cyclicDependencyReady();
} else {
this.cyclicDependency = Promise.resolve(undefined);
}
this.instantiationService = instantiationService;
}

private async createWorkspace(arg: IAnyWorkspaceIdentifier): Promise<Workspace> {
Expand Down Expand Up @@ -981,6 +972,10 @@ export class WorkspaceService extends Disposable implements IWorkbenchConfigurat
}

private async writeConfigurationValue(key: string, value: any, target: ConfigurationTarget, overrides: IConfigurationUpdateOverrides | undefined, donotNotifyError: boolean): Promise<void> {
if (!this.instantiationService) {
throw new Error('Cannot write configuration because the configuration service is not yet ready to accept writes.');
}

if (target === ConfigurationTarget.DEFAULT) {
throw new Error('Invalid configuration target');
}
Expand All @@ -1001,7 +996,9 @@ export class WorkspaceService extends Disposable implements IWorkbenchConfigurat
throw new Error('Invalid configuration target');
}

await this.configurationEditingService.writeConfiguration(editableConfigurationTarget, { key, value }, { scopes: overrides, donotNotifyError });
// Use same instance of ConfigurationEditing to make sure all writes go through the same queue
this.configurationEditing = this.configurationEditing ?? this.instantiationService.createInstance(ConfigurationEditing, (await this.remoteAgentService.getEnvironment())?.settingsPath ?? null);
await this.configurationEditing.writeConfiguration(editableConfigurationTarget, { key, value }, { scopes: overrides, donotNotifyError });
switch (editableConfigurationTarget) {
case EditableConfigurationTarget.USER_LOCAL:
if (this.applicationConfiguration && this.configurationRegistry.getConfigurationProperties()[key].scope === ConfigurationScope.APPLICATION) {
Expand Down
11 changes: 2 additions & 9 deletions ...ion/common/configurationEditingService.ts → ...figuration/common/configurationEditing.ts
View file
Open in desktop
Expand Up @@ -21,7 +21,6 @@ import { IEditorService } from 'vs/workbench/services/editor/common/editorServic
import { INotificationService, Severity } from 'vs/platform/notification/common/notification';
import { IOpenSettingsOptions, IPreferencesService } from 'vs/workbench/services/preferences/common/preferences';
import { withUndefinedAsNull, withNullAsUndefined } from 'vs/base/common/types';
import { IRemoteAgentService } from 'vs/workbench/services/remote/common/remoteAgentService';
import { IUriIdentityService } from 'vs/platform/uriIdentity/common/uriIdentity';
import { ITextModel } from 'vs/editor/common/model';
import { IReference } from 'vs/base/common/lifecycle';
Expand Down Expand Up @@ -144,14 +143,14 @@ interface ConfigurationEditingOptions extends IConfigurationEditingOptions {
handleDirtyFile?: 'save' | 'revert';
}

export class ConfigurationEditingService {
export class ConfigurationEditing {

public _serviceBrand: undefined;

private queue: Queue<void>;
private remoteSettingsResource: URI | null = null;

constructor(
private readonly remoteSettingsResource: URI | null,
@IConfigurationService private readonly configurationService: IConfigurationService,
@IWorkspaceContextService private readonly contextService: IWorkspaceContextService,
@IUserDataProfileService private readonly userDataProfileService: IUserDataProfileService,
Expand All @@ -162,15 +161,9 @@ export class ConfigurationEditingService {
@INotificationService private readonly notificationService: INotificationService,
@IPreferencesService private readonly preferencesService: IPreferencesService,
@IEditorService private readonly editorService: IEditorService,
@IRemoteAgentService remoteAgentService: IRemoteAgentService,
@IUriIdentityService private readonly uriIdentityService: IUriIdentityService,
) {
this.queue = new Queue<void>();
remoteAgentService.getEnvironment().then(environment => {
if (environment) {
this.remoteSettingsResource = environment.settingsPath;
}
});
}

async writeConfiguration(target: EditableConfigurationTarget, value: IConfigurationValue, options: IConfigurationEditingOptions = {}): Promise<void> {
Expand Down
8 changes: 4 additions & 4 deletions ...owser/configurationEditingService.test.ts → ...test/browser/configurationEditing.test.ts
View file
Open in desktop
Expand Up @@ -14,7 +14,7 @@ import { TestEnvironmentService, TestTextFileService, workbenchInstantiationServ
import * as uuid from 'vs/base/common/uuid';
import { IConfigurationRegistry, Extensions as ConfigurationExtensions } from 'vs/platform/configuration/common/configurationRegistry';
import { WorkspaceService } from 'vs/workbench/services/configuration/browser/configurationService';
import { ConfigurationEditingService, ConfigurationEditingErrorCode, EditableConfigurationTarget } from 'vs/workbench/services/configuration/common/configurationEditingService';
import { ConfigurationEditing, ConfigurationEditingErrorCode, EditableConfigurationTarget } from 'vs/workbench/services/configuration/common/configurationEditing';
import { WORKSPACE_STANDALONE_CONFIGURATIONS, FOLDER_SETTINGS_PATH, USER_STANDALONE_CONFIGURATIONS, IConfigurationCache } from 'vs/workbench/services/configuration/common/configuration';
import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
import { TestInstantiationService } from 'vs/platform/instantiation/test/common/instantiationServiceMock';
Expand Down Expand Up @@ -56,14 +56,14 @@ export class ConfigurationCache implements IConfigurationCache {
async remove(): Promise<void> { }
}

suite('ConfigurationEditingService', () => {
suite('ConfigurationEditing', () => {

let instantiationService: TestInstantiationService;
let userDataProfileService: IUserDataProfileService;
let environmentService: IWorkbenchEnvironmentService;
let fileService: IFileService;
let workspaceService: WorkspaceService;
let testObject: ConfigurationEditingService;
let testObject: ConfigurationEditing;

const disposables = new DisposableStore();

Expand Down Expand Up @@ -130,7 +130,7 @@ suite('ConfigurationEditingService', () => {
instantiationService.stub(ITextFileService, disposables.add(instantiationService.createInstance(TestTextFileService)));
instantiationService.stub(ITextModelService, <ITextModelService>disposables.add(instantiationService.createInstance(TextModelResolverService)));
instantiationService.stub(ICommandService, CommandService);
testObject = instantiationService.createInstance(ConfigurationEditingService);
testObject = instantiationService.createInstance(ConfigurationEditing, null);
});

teardown(() => disposables.clear());
Expand Down
7 changes: 5 additions & 2 deletions src/vs/workbench/services/configuration/test/browser/configurationService.test.ts
View file
Open in desktop
Expand Up @@ -10,7 +10,7 @@ import { Registry } from 'vs/platform/registry/common/platform';
import { IEnvironmentService } from 'vs/platform/environment/common/environment';
import { IConfigurationRegistry, Extensions as ConfigurationExtensions, ConfigurationScope, keyFromOverrideIdentifiers } from 'vs/platform/configuration/common/configurationRegistry';
import { WorkspaceService } from 'vs/workbench/services/configuration/browser/configurationService';
import { ConfigurationEditingErrorCode } from 'vs/workbench/services/configuration/common/configurationEditingService';
import { ConfigurationEditingErrorCode } from 'vs/workbench/services/configuration/common/configurationEditing';
import { IFileService } from 'vs/platform/files/common/files';
import { IWorkspaceContextService, WorkbenchState, IWorkspaceFoldersChangeEvent, ISingleFolderWorkspaceIdentifier, IWorkspaceIdentifier } from 'vs/platform/workspace/common/workspace';
import { ConfigurationTarget, IConfigurationService, IConfigurationChangeEvent } from 'vs/platform/configuration/common/configuration';
Expand Down Expand Up @@ -271,6 +271,7 @@ suite('WorkspaceContextService - Workspace Editing', () => {
await testObject.initialize(getWorkspaceIdentifier(configResource));
instantiationService.stub(ITextFileService, disposables.add(instantiationService.createInstance(TestTextFileService)));
instantiationService.stub(ITextModelService, disposables.add(instantiationService.createInstance(TextModelResolverService)));
instantiationService.stub(IJSONEditingService, instantiationService.createInstance(JSONEditingService));
testObject.acquireInstantiationService(instantiationService);
});

Expand Down Expand Up @@ -1636,10 +1637,11 @@ suite('WorkspaceConfigurationService-Multiroot', () => {
instantiationService.stub(IKeybindingEditingService, instantiationService.createInstance(KeybindingsEditingService));
instantiationService.stub(ITextFileService, instantiationService.createInstance(TestTextFileService));
instantiationService.stub(ITextModelService, <ITextModelService>instantiationService.createInstance(TextModelResolverService));
jsonEditingServce = instantiationService.createInstance(JSONEditingService);
instantiationService.stub(IJSONEditingService, jsonEditingServce);
workspaceService.acquireInstantiationService(instantiationService);

workspaceContextService = workspaceService;
jsonEditingServce = instantiationService.createInstance(JSONEditingService);
testObject = workspaceService;
});

Expand Down Expand Up @@ -2299,6 +2301,7 @@ suite('WorkspaceConfigurationService - Remote Folder', () => {
await testObject.initialize(convertToWorkspacePayload(folder));
instantiationService.stub(ITextFileService, instantiationService.createInstance(TestTextFileService));
instantiationService.stub(ITextModelService, <ITextModelService>instantiationService.createInstance(TextModelResolverService));
instantiationService.stub(IJSONEditingService, instantiationService.createInstance(JSONEditingService));
testObject.acquireInstantiationService(instantiationService);
}

Expand Down