Handle open preference files

CHANGELOG.md

@@ -5,13 +5,15 @@
 [1.21.0 Milestone](https://github.com/eclipse-theia/theia/milestone/29)
 
 - [core, editor, editor-preview] additional commands added to tabbar context menu for editor widgets. [#10394](https://github.com/eclipse-theia/theia/pull/10394)
+- [preferences] Updated `AbstractResourcePreferenceProvider` to handle multiple preference settings in the same tick and handle open preference files. It will save the file exactly once, and prompt the user if the file is dirty when a programmatic setting is attempted. [#7775](https://github.com/eclipse-theia/theia/pull/7775)
 
 <a name="breaking_changes_1.21.0">[Breaking Changes:](#breaking_changes_1.21.0)</a>
 
 - [webpack] Source maps for the frontend renamed from `webpack://[namespace]/[resource-filename]...`
   to `webpack:///[resource-path]?[loaders]` where `resource-path` is the path to the file relative
   to your application package's root.
 - [core] `SelectionService` added to constructor arguments of `TabBarRenderer`. [#10394](https://github.com/eclipse-theia/theia/pull/10394)
+- [preferences] Removed `PreferenceProvider#pendingChanges` field. It was previously set unreliably and caused race conditions. If a `PreferenceProvider` needs a mechanism for deferring the resolution of `PreferenceProvider#setPreference`, it should implement its own system. See PR for example implementation in `AbstractResourcePreferenceProvider`. [#7775](https://github.com/eclipse-theia/theia/pull/7775)
 
 ## v1.20.0 - 11/25/2021
 

packages/core/src/browser/preferences/preference-provider.ts

@@ -75,10 +75,6 @@ export abstract class PreferenceProvider implements Disposable {
     }
 
     protected deferredChanges: PreferenceProviderDataChanges | undefined;
-    protected _pendingChanges: Promise<boolean> = Promise.resolve(false);
-    get pendingChanges(): Promise<boolean> {
-        return this._pendingChanges;
-    }
 
     /**
      * Informs the listeners that one or more preferences of this provider are changed.
@@ -94,7 +90,7 @@ export abstract class PreferenceProvider implements Disposable {
                 this.mergePreferenceProviderDataChange(changes[preferenceName]);
             }
         }
-        return this._pendingChanges = this.fireDidPreferencesChanged();
+        return this.fireDidPreferencesChanged();
     }
 
     protected mergePreferenceProviderDataChange(change: PreferenceProviderDataChange): void {

packages/core/src/browser/preferences/preference-service.spec.ts

@@ -193,7 +193,6 @@ describe('Preference Service', () => {
         assert.strictEqual(prefService.get('editor.insertSpaces'), undefined, 'get after');
         assert.strictEqual(prefService.get('[go].editor.insertSpaces'), undefined, 'get after overridden');
 
-        assert.strictEqual(await prefSchema.pendingChanges, false);
         assert.deepStrictEqual([], events.map(e => ({
             preferenceName: e.preferenceName,
             newValue: e.newValue,
@@ -488,7 +487,6 @@ describe('Preference Service', () => {
 
         it('onPreferenceChanged #0', async () => {
             const { preferences, schema } = prepareServices();
-            await schema.pendingChanges;
 
             const events: PreferenceChange[] = [];
             preferences.onPreferenceChanged(event => events.push(event));
@@ -511,7 +509,6 @@ describe('Preference Service', () => {
 
         it('onPreferenceChanged #1', async () => {
             const { preferences, schema } = prepareServices();
-            await schema.pendingChanges;
 
             const events: PreferenceChange[] = [];
             preferences.onPreferenceChanged(event => events.push(event));

packages/monaco/src/browser/monaco-workspace.ts

@@ -205,14 +205,14 @@ export class MonacoWorkspace {
      * Applies given edits to the given model.
      * The model is saved if no editors is opened for it.
      */
-    applyBackgroundEdit(model: MonacoEditorModel, editOperations: monaco.editor.IIdentifiedSingleEditOperation[]): Promise<void> {
+    applyBackgroundEdit(model: MonacoEditorModel, editOperations: monaco.editor.IIdentifiedSingleEditOperation[], shouldSave = true): Promise<void> {
         return this.suppressOpenIfDirty(model, async () => {
             const editor = MonacoEditor.findByDocument(this.editorManager, model)[0];
             const cursorState = editor && editor.getControl().getSelections() || [];
             model.textEditorModel.pushStackElement();
             model.textEditorModel.pushEditOperations(cursorState, editOperations, () => cursorState);
             model.textEditorModel.pushStackElement();
-            if (!editor) {
+            if (!editor && shouldSave) {
                 await model.save();
             }
         });

packages/preferences/package.json

@@ -9,6 +9,7 @@
     "@theia/monaco": "1.20.0",
     "@theia/userstorage": "1.20.0",
     "@theia/workspace": "1.20.0",
+    "async-mutex": "^0.3.1",
     "jsonc-parser": "^2.2.0"
   },
   "publishConfig": {

packages/preferences/src/browser/abstract-resource-preference-provider.spec.ts

@@ -36,6 +36,7 @@ import { MonacoTextModelService } from '@theia/monaco/lib/browser/monaco-text-mo
 import { Disposable, MessageService } from '@theia/core/lib/common';
 import { MonacoWorkspace } from '@theia/monaco/lib/browser/monaco-workspace';
 import { PreferenceSchemaProvider } from '@theia/core/lib/browser';
+import { EditorManager } from '@theia/editor/lib/browser';
 
 disableJSDOM();
 
@@ -84,6 +85,7 @@ describe('AbstractResourcePreferenceProvider', () => {
         testContainer.bind(<any>MonacoTextModelService).toConstantValue(new MockTextModelService);
         testContainer.bind(<any>MessageService).toConstantValue(undefined);
         testContainer.bind(<any>MonacoWorkspace).toConstantValue(undefined);
+        testContainer.bind(<any>EditorManager).toConstantValue(undefined);
         provider = testContainer.resolve(<any>LessAbstractPreferenceProvider);
     });
 

packages/preferences/src/browser/abstract-resource-preference-provider.ts

@@ -18,6 +18,7 @@
 /* eslint-disable no-null/no-null */
 
 import * as jsoncparser from 'jsonc-parser';
+import { Mutex, MutexInterface } from 'async-mutex';
 import { inject, injectable, postConstruct } from '@theia/core/shared/inversify';
 import { MessageService } from '@theia/core/lib/common/message-service';
 import { Disposable } from '@theia/core/lib/common/disposable';
@@ -29,6 +30,21 @@ import { MonacoEditorModel } from '@theia/monaco/lib/browser/monaco-editor-model
 import { MonacoWorkspace } from '@theia/monaco/lib/browser/monaco-workspace';
 import { Deferred } from '@theia/core/lib/common/promise-util';
 import { FileService } from '@theia/filesystem/lib/browser/file-service';
+import { CancellationError, nls } from '@theia/core';
+import { EditorManager } from '@theia/editor/lib/browser';
+
+export interface FilePreferenceProviderLocks {
+    /**
+     * Defined if the current operation is the first operation in a new transaction.
+     * The first operation is responsible for checking whether the underlying editor is dirty
+     * and for saving the file when the transaction is complete.
+     */
+    releaseTransaction?: MutexInterface.Releaser | undefined;
+    /**
+     * A lock on the queue for single operations.
+     */
+    releaseChange: MutexInterface.Releaser;
+}
 
 @injectable()
 export abstract class AbstractResourcePreferenceProvider extends PreferenceProvider {
@@ -37,10 +53,14 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi
     protected model: MonacoEditorModel | undefined;
     protected readonly loading = new Deferred();
     protected modelInitialized = false;
+    protected readonly singleChangeLock = new Mutex();
+    protected readonly transactionLock = new Mutex();
+    protected pendingTransaction = new Deferred<boolean>();
 
     @inject(MessageService) protected readonly messageService: MessageService;
     @inject(PreferenceSchemaProvider) protected readonly schemaProvider: PreferenceSchemaProvider;
     @inject(FileService) protected readonly fileService: FileService;
+    @inject(EditorManager) protected readonly editorManager: EditorManager;
 
     @inject(PreferenceConfigurations)
     protected readonly configurations: PreferenceConfigurations;
@@ -53,6 +73,7 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi
 
     @postConstruct()
     protected async init(): Promise<void> {
+        this.pendingTransaction.resolve(true);
         const uri = this.getUri();
         this.toDispose.push(Disposable.create(() => this.loading.reject(new Error(`preference provider for '${uri}' was disposed`))));
         await this.readPreferencesFromFile();
@@ -71,8 +92,8 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi
         this.toDispose.push(reference);
         this.toDispose.push(Disposable.create(() => this.model = undefined));
 
-        this.toDispose.push(this.model.onDidChangeContent(() => this.readPreferences()));
-        this.toDispose.push(this.model.onDirtyChanged(() => this.readPreferences()));
+        this.toDispose.push(this.model.onDidChangeContent(() => !this.transactionLock.isLocked() && this.readPreferences()));
+        this.toDispose.push(this.model.onDirtyChanged(() => !this.transactionLock.isLocked() && this.readPreferences()));
         this.toDispose.push(this.model.onDidChangeValid(() => this.readPreferences()));
 
         this.toDispose.push(Disposable.create(() => this.reset()));
@@ -111,56 +132,118 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi
     }
 
     async setPreference(key: string, value: any, resourceUri?: string): Promise<boolean> {
-        await this.loading.promise;
-        if (!this.model) {
-            return false;
-        }
-        if (!this.contains(resourceUri)) {
-            return false;
-        }
-        const path = this.getPath(key);
-        if (!path) {
-            return false;
-        }
+        const locks = await this.acquireLocks();
+        let shouldSave = Boolean(locks?.releaseTransaction);
         try {
-            const content = this.model.getText().trim();
-            if (!content && value === undefined) {
-                return true;
+            await this.loading.promise;
+            let path: string[] | undefined;
+            if (!this.model || !(path = this.getPath(key)) || !this.contains(resourceUri)) {
+                return false;
             }
-            const textModel = this.model.textEditorModel;
-            const editOperations: monaco.editor.IIdentifiedSingleEditOperation[] = [];
-            if (path.length || value !== undefined) {
-                const { insertSpaces, tabSize, defaultEOL } = textModel.getOptions();
-                for (const edit of jsoncparser.modify(content, path, value, {
-                    formattingOptions: {
-                        insertSpaces,
-                        tabSize,
-                        eol: defaultEOL === monaco.editor.DefaultEndOfLine.LF ? '\n' : '\r\n'
-                    }
-                })) {
-                    const start = textModel.getPositionAt(edit.offset);
-                    const end = textModel.getPositionAt(edit.offset + edit.length);
-                    editOperations.push({
-                        range: monaco.Range.fromPositions(start, end),
-                        text: edit.content || null,
-                        forceMoveMarkers: false
-                    });
+            if (!locks) {
+                throw new CancellationError();
+            }
+            if (shouldSave) {
+                if (this.model.dirty) {
+                    shouldSave = await this.handleDirtyEditor();
                 }
-            } else {
-                editOperations.push({
-                    range: textModel.getFullModelRange(),
-                    text: null,
-                    forceMoveMarkers: false
-                });
+                if (!shouldSave) {
+                    throw new CancellationError();
+                }
+            }
+            const editOperations = this.getEditOperations(path, value);
+            if (editOperations.length > 0) {
+                await this.workspace.applyBackgroundEdit(this.model, editOperations, false);
             }
-            await this.workspace.applyBackgroundEdit(this.model, editOperations);
-            return await this.pendingChanges;
+            return this.pendingTransaction.promise;
         } catch (e) {
+            if (e instanceof CancellationError) {
+                throw e;
+            }
             const message = `Failed to update the value of '${key}' in '${this.getUri()}'.`;
             this.messageService.error(`${message} Please check if it is corrupted.`);
             console.error(`${message}`, e);
             return false;
+        } finally {
+            this.releaseLocks(locks, shouldSave);
+        }
+    }
+
+    /**
+     * @returns `undefined` if the queue has been cleared by a user action.
+     */
+    protected async acquireLocks(): Promise<FilePreferenceProviderLocks | undefined> {
+        // Request locks immediately
+        const releaseTransactionPromise = this.transactionLock.isLocked() ? undefined : this.transactionLock.acquire();
+        const releaseChangePromise = this.singleChangeLock.acquire().catch(() => {
+            releaseTransactionPromise?.then(release => release());
+            return undefined;
+        });
+        if (releaseTransactionPromise) {
+            await this.pendingTransaction.promise; // Ensure previous transaction complete before starting a new one.
+            this.pendingTransaction = new Deferred();
         }
+        // Wait to acquire locks
+        const [releaseTransaction, releaseChange] = await Promise.all([releaseTransactionPromise, releaseChangePromise]);
+        return releaseChange && { releaseTransaction, releaseChange };
+    }
+
+    protected releaseLocks(locks: FilePreferenceProviderLocks | undefined, shouldSave: boolean): void {
+        if (locks?.releaseTransaction) {
+            if (shouldSave) {
+                this.singleChangeLock.waitForUnlock().then(() => this.singleChangeLock.runExclusive(async () => {
+                    locks.releaseTransaction!(); // Release lock so that no new changes join this transaction.
+                    let success = false;
+                    try {
+                        await this.model?.save();
+                        success = true;
+                    } finally {
+                        this.readPreferences();
+                        await this.fireDidPreferencesChanged(); // Ensure all consumers of the event have received it.
+                        this.pendingTransaction.resolve(success);
+                    }
+                }));
+            } else { // User canceled the operation.
+                this.singleChangeLock.cancel();
+                locks.releaseTransaction!();
+                this.pendingTransaction.resolve(false);
+            }
+        }
+        locks?.releaseChange();
+    }
+
+    protected getEditOperations(path: string[], value: any): monaco.editor.IIdentifiedSingleEditOperation[] {
+        const textModel = this.model!.textEditorModel;
+        const content = this.model!.getText().trim();
+        // Everything is already undefined - no need for changes.
+        if (!content && value === undefined) {
+            return [];
+        }
+        // Delete the entire document.
+        if (!path.length && value === undefined) {
+            return [{
+                range: textModel.getFullModelRange(),
+                text: null,
+                forceMoveMarkers: false
+            }];
+        }
+        const { insertSpaces, tabSize, defaultEOL } = textModel.getOptions();
+        const jsonCOptions = {
+            formattingOptions: {
+                insertSpaces,
+                tabSize,
+                eol: defaultEOL === monaco.editor.DefaultEndOfLine.LF ? '\n' : '\r\n'
+            }
+        };
+        return jsoncparser.modify(content, path, value, jsonCOptions).map(edit => {
+            const start = textModel.getPositionAt(edit.offset);
+            const end = textModel.getPositionAt(edit.offset + edit.length);
+            return {
+                range: monaco.Range.fromPositions(start, end),
+                text: edit.content || null,
+                forceMoveMarkers: false
+            };
+        });
     }
 
     protected getPath(preferenceName: string): string[] | undefined {
@@ -234,7 +317,7 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi
             }
         }
 
-        if (prefChanges.length > 0) { // do not emit the change event if the pref value is not changed
+        if (prefChanges.length > 0) {
             this.emitPreferencesChangedEvent(prefChanges);
         }
     }
@@ -256,4 +339,27 @@ export abstract class AbstractResourcePreferenceProvider extends PreferenceProvi
         }
     }
 
+    /**
+     * @returns whether the setting operation in progress, and any others started in the meantime, should continue.
+     */
+    protected async handleDirtyEditor(): Promise<boolean> {
+        const saveAndRetry = nls.localizeByDefault('Save and Retry');
+        const open = nls.localizeByDefault('Open File');
+        const msg = await this.messageService.error(
+            nls.localizeByDefault('Unable to write into {0} settings because the file has unsaved changes. Please save the {0} settings file first and then try again.',
+                nls.localizeByDefault(PreferenceScope[this.getScope()].toLocaleLowerCase())
+            ),
+            saveAndRetry, open);
+
+        if (this.model) {
+            if (msg === open) {
+                this.editorManager.open(new URI(this.model.uri));
+                return false;
+            } else if (msg === saveAndRetry) {
+                await this.model.save();
+                return true;
+            }
+        }
+        return false;
+    }
 }

packages/preferences/src/browser/views/components/preference-node-renderer.ts

@@ -403,7 +403,8 @@ export abstract class PreferenceLeafNodeRenderer<ValueType extends JSONValue, In
     protected setPreferenceWithDebounce = debounce(this.setPreferenceImmediately.bind(this), 500, { leading: false, trailing: true });
 
     protected setPreferenceImmediately(value: ValueType | undefined): Promise<void> {
-        return this.preferenceService.set(this.id, value, this.scopeTracker.currentScope.scope, this.scopeTracker.currentScope.uri);
+        return this.preferenceService.set(this.id, value, this.scopeTracker.currentScope.scope, this.scopeTracker.currentScope.uri)
+            .catch(() => this.handleValueChange());
     }
 
     handleSearchChange(isFiltered = this.model.isFiltered): void {
@@ -420,7 +421,18 @@ export abstract class PreferenceLeafNodeRenderer<ValueType extends JSONValue, In
         this.updateHeadline();
     }
 
+    /**
+     * Should create an HTML element that the user can interact with to change the value of the preference.
+     * @param container the parent element for the interactable. This method is responsible for adding the new element to its parent.
+     */
     protected abstract createInteractable(container: HTMLElement): void;
+    /**
+     * @returns a fallback default value for a preference of the type implemented by a concrete leaf renderer
+     * This function is only called if the default value for a given preference is not specified in its schema.
+     */
     protected abstract getFallbackValue(): ValueType;
+    /**
+     * This function is responsible for reconciling the display of the preference value with the value reported by the PreferenceService.
+     */
     protected abstract doHandleValueChange(): void;
 }