model / working copy - go through managers when resolving again (microsoft#146504)

Author: bpasero

src/vs/workbench/services/textfile/common/textFileEditorModel.ts

@@ -247,7 +247,7 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil
 		const softUndo = options?.soft;
 		if (!softUndo) {
 			try {
-				await this.resolve({ forceReadFromFile: true });
+				await this.forceResolveFromFile();
 			} catch (error) {
 
 				// FileNotFound means the file got deleted meanwhile, so ignore it
@@ -636,6 +636,23 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil
 		}
 	}
 
+	private async forceResolveFromFile(): Promise<void> {
+		if (this.isDisposed()) {
+			return; // return early when the model is invalid
+		}
+
+		// We go through the text file service to make
+		// sure this kind of `resolve` is properly
+		// running in sequence with any other running
+		// `resolve` if any, including subsequent runs
+		// that are triggered right after.
+
+		await this.textFileService.files.resolve(this.resource, {
+			reload: { async: false },
+			forceReadFromFile: true
+		});
+	}
+
 	//#endregion
 
 	//#region Dirty
@@ -1004,10 +1021,6 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil
 		//
 		// (see https://github.com/microsoft/vscode/issues/127936)
 
-		if (this.hasLanguageSetExplicitly) {
-			return; // return early when the language was changed by the user
-		}
-
 		if (!this.configurationService.inspect('files.encoding').overrideIdentifiers?.includes(e.newLanguage)) {
 			return; // only when there is a language specific override for the new language
 		}
@@ -1021,6 +1034,8 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil
 			return; // return early to prevent accident saves in this case
 		}
 
+		this.logService.info(`Adjusting encoding based on configured language override to '${encoding}' for ${this.resource.toString(true)}.`);
+
 		// Re-open with new encoding
 		await this.setEncoding(encoding, EncodingMode.Decode);
 	}
@@ -1058,9 +1073,7 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil
 
 			this.updatePreferredEncoding(encoding);
 
-			await this.resolve({
-				forceReadFromFile: true	// because encoding has changed
-			});
+			await this.forceResolveFromFile();
 		}
 	}
 

src/vs/workbench/services/textfile/common/textfiles.ts

@@ -266,12 +266,7 @@ export interface ITextFileStreamContent extends IBaseTextFileContent {
 	readonly value: ITextBufferFactory;
 }
 
-export interface ITextFileEditorModelResolveOrCreateOptions {
-
-	/**
-	 * Context why the model is being resolved or created.
-	 */
-	readonly reason?: TextFileResolveReason;
+export interface ITextFileEditorModelResolveOrCreateOptions extends ITextFileResolveOptions {
 
 	/**
 	 * The language id to use for the model text content.
@@ -283,13 +278,6 @@ export interface ITextFileEditorModelResolveOrCreateOptions {
 	 */
 	readonly encoding?: string;
 
-	/**
-	 * The contents to use for the model if known. If not
-	 * provided, the contents will be retrieved from the
-	 * underlying resource or backup if present.
-	 */
-	readonly contents?: ITextBufferFactory;
-
 	/**
 	 * If the model was already resolved before, allows to trigger
 	 * a reload of it to fetch the latest contents.
@@ -302,11 +290,6 @@ export interface ITextFileEditorModelResolveOrCreateOptions {
 		 */
 		readonly async: boolean;
 	};
-
-	/**
-	 * Allow to resolve a model even if we think it is a binary file.
-	 */
-	readonly allowBinary?: boolean;
 }
 
 export interface ITextFileSaveEvent extends ITextFileEditorModelSaveEvent {

src/vs/workbench/services/textfile/test/browser/textFileEditorModel.test.ts

@@ -49,6 +49,7 @@ suite('Files - TextFileEditorModel', () => {
 
 	test('basic events', async function () {
 		const model = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/index_async.txt'), 'utf8', undefined);
+		accessor.workingCopyService.unregisterWorkingCopy(model); // causes issues with subsequent resolves otherwise
 
 		let onDidResolveCounter = 0;
 		model.onDidResolve(() => onDidResolveCounter++);
@@ -300,16 +301,24 @@ suite('Files - TextFileEditorModel', () => {
 	});
 
 	test('setEncoding - decode', async function () {
-		const model: TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/index_async.txt'), 'utf8', undefined);
+		let model: TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/index_async.txt'), 'utf8', undefined);
+		accessor.workingCopyService.unregisterWorkingCopy(model); // causes issues with subsequent resolves otherwise
 
 		await model.setEncoding('utf16', EncodingMode.Decode);
 
+		// we have to get the model again from working copy service
+		// because `setEncoding` will resolve it again through the
+		// text file service which is outside our scope
+		model = accessor.workingCopyService.get(model) as TextFileEditorModel;
+
 		assert.ok(model.isResolved()); // model got resolved due to decoding
 		model.dispose();
 	});
 
 	test('setEncoding - decode dirty file saves first', async function () {
 		const model: TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/index_async.txt'), 'utf8', undefined);
+		accessor.workingCopyService.unregisterWorkingCopy(model); // causes issues with subsequent resolves otherwise
+
 		await model.resolve();
 
 		model.updateTextEditorModel(createTextBufferFactory('bar'));
@@ -397,7 +406,7 @@ suite('Files - TextFileEditorModel', () => {
 	test('Revert', async function () {
 		let eventCounter = 0;
 
-		const model = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/index_async.txt'), 'utf8', undefined);
+		let model = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/index_async.txt'), 'utf8', undefined);
 
 		model.onDidRevert(() => eventCounter++);
 
@@ -415,9 +424,16 @@ suite('Files - TextFileEditorModel', () => {
 		assert.strictEqual(accessor.workingCopyService.dirtyCount, 1);
 		assert.strictEqual(accessor.workingCopyService.isDirty(model.resource, model.typeId), true);
 
+		accessor.workingCopyService.unregisterWorkingCopy(model); // causes issues with subsequent resolves otherwise
+
 		await model.revert();
+
+		// we have to get the model again from working copy service
+		// because `setEncoding` will resolve it again through the
+		// text file service which is outside our scope
+		model = accessor.workingCopyService.get(model) as TextFileEditorModel;
+
 		assert.strictEqual(model.isDirty(), false);
-		assert.strictEqual(model.textEditorModel!.getValue(), 'Hello Html');
 		assert.strictEqual(eventCounter, 1);
 
 		assert.ok(workingCopyEvent);

src/vs/workbench/services/textfile/test/browser/textFileEditorModelManager.test.ts

@@ -6,7 +6,6 @@
 import * as assert from 'assert';
 import { URI } from 'vs/base/common/uri';
 import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
-import { TextFileEditorModelManager } from 'vs/workbench/services/textfile/common/textFileEditorModelManager';
 import { workbenchInstantiationService, TestServiceAccessor, TestTextFileEditorModelManager } from 'vs/workbench/test/browser/workbenchTestServices';
 import { TextFileEditorModel } from 'vs/workbench/services/textfile/common/textFileEditorModel';
 import { FileChangesEvent, FileChangeType, FileOperationError, FileOperationResult } from 'vs/platform/files/common/files';
@@ -34,7 +33,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('add, remove, clear, get, getAll', function () {
-		const manager: TestTextFileEditorModelManager = instantiationService.createInstance(TestTextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 
 		const model1: TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/random1.txt'), 'utf8', undefined);
 		const model2: TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/random2.txt'), 'utf8', undefined);
@@ -91,7 +90,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('resolve', async () => {
-		const manager: TestTextFileEditorModelManager = instantiationService.createInstance(TestTextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 		const resource = URI.file('/test.html');
 		const encoding = 'utf8';
 
@@ -131,7 +130,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('resolve (async)', async () => {
-		const manager: TestTextFileEditorModelManager = instantiationService.createInstance(TestTextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 		const resource = URI.file('/path/index.txt');
 
 		await manager.resolve(resource);
@@ -154,7 +153,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('resolve (sync)', async () => {
-		const manager: TestTextFileEditorModelManager = instantiationService.createInstance(TestTextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 		const resource = URI.file('/path/index.txt');
 
 		await manager.resolve(resource);
@@ -171,7 +170,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('resolve (sync) - model disposed when error and first call to resolve', async () => {
-		const manager: TestTextFileEditorModelManager = instantiationService.createInstance(TestTextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 		const resource = URI.file('/path/index.txt');
 
 		accessor.textFileService.setReadStreamErrorOnce(new FileOperationError('fail', FileOperationResult.FILE_OTHER_ERROR));
@@ -188,7 +187,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('resolve (sync) - model not disposed when error and model existed before', async () => {
-		const manager: TestTextFileEditorModelManager = instantiationService.createInstance(TestTextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 		const resource = URI.file('/path/index.txt');
 
 		await manager.resolve(resource);
@@ -207,7 +206,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('resolve with initial contents', async () => {
-		const manager: TestTextFileEditorModelManager = instantiationService.createInstance(TestTextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 		const resource = URI.file('/test.html');
 
 		const model = await manager.resolve(resource, { contents: createTextBufferFactory('Hello World') });
@@ -223,7 +222,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('multiple resolves execute in sequence', async () => {
-		const manager: TestTextFileEditorModelManager = instantiationService.createInstance(TestTextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 		const resource = URI.file('/test.html');
 
 		let resolvedModel: unknown;
@@ -257,7 +256,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('removed from cache when model disposed', function () {
-		const manager: TestTextFileEditorModelManager = instantiationService.createInstance(TestTextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 
 		const model1: TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/random1.txt'), 'utf8', undefined);
 		const model2: TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/random2.txt'), 'utf8', undefined);
@@ -279,7 +278,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('events', async function () {
-		const manager: TextFileEditorModelManager = instantiationService.createInstance(TextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 
 		const resource1 = toResource.call(this, '/path/index.txt');
 		const resource2 = toResource.call(this, '/path/other.txt');
@@ -368,7 +367,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('disposing model takes it out of the manager', async function () {
-		const manager: TextFileEditorModelManager = instantiationService.createInstance(TextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 
 		const resource = toResource.call(this, '/path/index_something.txt');
 
@@ -380,7 +379,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('canDispose with dirty model', async function () {
-		const manager: TextFileEditorModelManager = instantiationService.createInstance(TextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 
 		const resource = toResource.call(this, '/path/index_something.txt');
 
@@ -414,7 +413,7 @@ suite('Files - TextFileEditorModelManager', () => {
 			id: languageId,
 		});
 
-		const manager: TextFileEditorModelManager = instantiationService.createInstance(TextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 
 		const resource = toResource.call(this, '/path/index_something.txt');
 
@@ -429,7 +428,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('file change events trigger reload (on a resolved model)', async () => {
-		const manager: TestTextFileEditorModelManager = instantiationService.createInstance(TestTextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 		const resource = URI.file('/path/index.txt');
 
 		await manager.resolve(resource);
@@ -451,7 +450,7 @@ suite('Files - TextFileEditorModelManager', () => {
 	});
 
 	test('file change events trigger reload (after a model is resolved: https://github.com/microsoft/vscode/issues/132765)', async () => {
-		const manager: TestTextFileEditorModelManager = instantiationService.createInstance(TestTextFileEditorModelManager);
+		const manager = accessor.textFileService.files as TestTextFileEditorModelManager;
 		const resource = URI.file('/path/index.txt');
 
 		manager.resolve(resource);

src/vs/workbench/services/workingCopy/common/storedFileWorkingCopy.ts

@@ -226,6 +226,16 @@ export interface IStoredFileWorkingCopySaveOptions extends ISaveOptions {
 	readonly ignoreErrorHandler?: boolean;
 }
 
+export interface IStoredFileWorkingCopyResolver {
+
+	/**
+	 * Resolves the working copy in a safe way from an external
+	 * working copy manager that can make sure multiple parallel
+	 * resolves execute properly.
+	 */
+	(options?: IStoredFileWorkingCopyResolveOptions): Promise<void>;
+}
+
 export interface IStoredFileWorkingCopyResolveOptions {
 
 	/**
@@ -306,6 +316,7 @@ export class StoredFileWorkingCopy<M extends IStoredFileWorkingCopyModel> extend
 		resource: URI,
 		readonly name: string,
 		private readonly modelFactory: IStoredFileWorkingCopyModelFactory<M>,
+		private readonly externalResolver: IStoredFileWorkingCopyResolver,
 		@IFileService fileService: IFileService,
 		@ILogService private readonly logService: ILogService,
 		@IWorkingCopyFileService private readonly workingCopyFileService: IWorkingCopyFileService,
@@ -716,6 +727,22 @@ export class StoredFileWorkingCopy<M extends IStoredFileWorkingCopyModel> extend
 		this._onDidChangeContent.fire();
 	}
 
+	private async forceResolveFromFile(): Promise<void> {
+		if (this.isDisposed()) {
+			return; // return early when the working copy is invalid
+		}
+
+		// We go through the resolver to make
+		// sure this kind of `resolve` is properly
+		// running in sequence with any other running
+		// `resolve` if any, including subsequent runs
+		// that are triggered right after.
+
+		await this.externalResolver({
+			forceReadFromFile: true
+		});
+	}
+
 	//#endregion
 
 	//#region Backup
@@ -1132,7 +1159,7 @@ export class StoredFileWorkingCopy<M extends IStoredFileWorkingCopyModel> extend
 		const softUndo = options?.soft;
 		if (!softUndo) {
 			try {
-				await this.resolve({ forceReadFromFile: true });
+				await this.forceResolveFromFile();
 			} catch (error) {
 
 				// FileNotFound means the file got deleted meanwhile, so ignore it

src/vs/workbench/services/workingCopy/common/storedFileWorkingCopyManager.ts

@@ -112,17 +112,7 @@ export interface IStoredFileWorkingCopySaveEvent<M extends IStoredFileWorkingCop
 	readonly workingCopy: IStoredFileWorkingCopy<M>;
 }
 
-export interface IStoredFileWorkingCopyManagerResolveOptions {
-
-	/**
-	 * The contents to use for the stored file working copy if known. If not
-	 * provided, the contents will be retrieved from the underlying
-	 * resource or backup if present.
-	 *
-	 * If contents are provided, the stored file working copy will be marked
-	 * as dirty right from the beginning.
-	 */
-	readonly contents?: VSBufferReadableStream;
+export interface IStoredFileWorkingCopyManagerResolveOptions extends IStoredFileWorkingCopyResolveOptions {
 
 	/**
 	 * If the stored file working copy was already resolved before,
@@ -529,6 +519,7 @@ export class StoredFileWorkingCopyManager<M extends IStoredFileWorkingCopyModel>
 				resource,
 				this.labelService.getUriBasenameLabel(resource),
 				this.modelFactory,
+				async options => { await this.resolve(resource, { ...options, reload: { async: false } }); },
 				this.fileService, this.logService, this.workingCopyFileService, this.filesConfigurationService,
 				this.workingCopyBackupService, this.workingCopyService, this.notificationService, this.workingCopyEditorService,
 				this.editorService, this.elevatedFileService