From 796a9dc21c8046ae5b99fd7d417f8a842ac408af Mon Sep 17 00:00:00 2001 From: Sergio Costas Rodriguez Date: Mon, 17 Feb 2025 12:49:33 +0100 Subject: [PATCH 1/2] Add asyncMux to avoid race conditions in async functions When several async functions share access to common elements, it's possible to have race condition problems. For example, when an extension is enabled, it is deleted from the list of disabled extensions and added to the list of enabled extensions. This can cause a race condition because both cases call the same asynchronous function, resulting in the creation of two instances of each extension, and the `enable` method of each one being called twice. This is possible because the method `onEnabledExtensionsChanged` can be called both from a signal of a change in the `disabled-extensions` property, or from a signal of a change in the `enabled-extensions` property. Since this method is async and does several `await` calls, two very close changes to those properties can trigger it the second time when the first time is in the middle of an `await`, and, thus, in an incomplete state. This patch defines an `AsyncMutex` object that allows to ensure that, if an async function is called again while already processing a call, won't execute until any previous call has ended and completed their operations. This is, it works like a mutex semaphore, but designed for Javascript async functions/methods. GLib mutexes can't be used in this case because they are tied to the C language and presume threading/ multiprocess, something that doesn't exist in javascript's asynchronous programming. To work with it, an `AsyncMutex` object is created for each critical section. Then, the `hold()` method must be awaited when entering the critical section, and the `release()` method must be called just after exiting it. The first worker will run as usual, but if it does an `await` inside the critical section and a new worker tries to enter it, the `await asyncMutex.hold()` call will wait until the first worker calls `asyncMutex.release()`. The `AsyncMutex` class implements a waiting queue, thus if several workers are awaiting, when the worker that owns the mutex releases it, the ownership will be passed to the first worker that waited when doing the `hold()` call. Each worker will receive ownership of the mutex in the same order than they requested it when calling the `hold()` method. --- js/js-resources.gresource.xml | 1 + js/misc/asyncMutex.js | 29 +++++ tests/meson.build | 1 + tests/unit/asyncMutex.js | 223 ++++++++++++++++++++++++++++++++++ 4 files changed, 254 insertions(+) create mode 100644 js/misc/asyncMutex.js create mode 100755 tests/unit/asyncMutex.js diff --git a/js/js-resources.gresource.xml b/js/js-resources.gresource.xml index e5e6167f15..85a53c8f6b 100644 --- a/js/js-resources.gresource.xml +++ b/js/js-resources.gresource.xml @@ -14,6 +14,7 @@ extensions/extension.js extensions/sharedInternals.js + misc/asyncMutex.js misc/animationUtils.js misc/breakManager.js misc/brightnessManager.js diff --git a/js/misc/asyncMutex.js b/js/misc/asyncMutex.js new file mode 100644 index 0000000000..0ae52dad0e --- /dev/null +++ b/js/misc/asyncMutex.js @@ -0,0 +1,29 @@ +export class AsyncMutex { + #isHold = false; + #pendingResolvers = []; + + hold() { + if (this.#isHold) { + const {promise, resolve} = Promise.withResolvers(); + this.#pendingResolvers.push(resolve); + return promise; + } + this.#isHold = true; + return Promise.resolve(); + } + + release() { + if (!this.#isHold) + return; + if (this.#pendingResolvers.length === 0) { + this.#isHold = false; + return; + } + const resolve = this.#pendingResolvers.shift(); + resolve(); + } + + get pendingResolves() { + return this.#pendingResolvers.length; + } +} diff --git a/tests/meson.build b/tests/meson.build index b9b4f31126..dcd4351e1f 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -29,6 +29,7 @@ unit_testenv.append('GI_TYPELIB_PATH', shell_typelib_path, separator: ':') unit_testenv.append('GI_TYPELIB_PATH', st_typelib_path, separator: ':') unit_tests = { + 'asyncMutex': {}, 'breakManager': {}, 'errorUtils': {}, 'extensionUtils': {}, diff --git a/tests/unit/asyncMutex.js b/tests/unit/asyncMutex.js new file mode 100755 index 0000000000..18e93194c0 --- /dev/null +++ b/tests/unit/asyncMutex.js @@ -0,0 +1,223 @@ +import GLib from 'gi://GLib'; + +import * as Mutex from 'resource:///org/gnome/shell/misc/asyncMutex.js'; + +/** + * A helper that asynchronously waits an specified amount + * of time in milliseconds. + * + * @param {Int} ms Time, in milliseconds, to wait before resolving the promise. + * @returns A promise that will be resolved after the specified time. + */ +function waitMs(ms) { + return new Promise((resolve, _reject) => { + setTimeout(resolve, ms); + }); +} + +const STATE_UNINITIALIZED = -1; +const STATE_HOLD = 0; +const STATE_RUN = 1; +const STATE_RELEASE = 2; +const STATE_DONE = 3; + +class baseTest { + #counterWithMutex = 0; + #mutex; + #loop; + #promiseList = []; + #messages = []; + #workercounter = 0; + + constructor() { + this.#mutex = new Mutex.AsyncMutex(); + this.#loop = new GLib.MainLoop(null, false); + } + + _disableMutex() { + this.#mutex = null; + } + + async _workerWithMutex(index) { + this.#messages.push({'state': STATE_HOLD, 'id': index}); + if (this.#mutex) + await this.#mutex.hold(); + this.#messages.push({'state': STATE_RUN, 'id': index}); + const value = this.#counterWithMutex; + await waitMs(200); + this.#messages.push({'state': STATE_RELEASE, 'id': index}); + this.#counterWithMutex = value + 1; + if (this.#mutex) + this.#mutex.release(); + this.#messages.push({'state': STATE_DONE, 'id': index}); + } + + _appendWorker() { + this.#promiseList.push(this._workerWithMutex(this.#workercounter++)); + } + + _runLoop() { + Promise.all(this.#promiseList).then(() => { + this.#loop.quit(); + }); + this.#loop.run(); + } + + get counter() { + return this.#counterWithMutex; + } + + #checkWorkerCompletedFine(workerId) { + // Ensures that the specified worker passes in order by all the states. + let lastState = STATE_UNINITIALIZED; + for (const msg of this.#messages) { + if (msg.id !== workerId) + continue; + if (msg.state !== (lastState + 1)) + return false; + lastState = msg.state; + } + return lastState === STATE_DONE; + } + + checkWorkersCompletedFine() { + // Ensures that all the workers pass in order by all the states. + for (let i = 0; i < this.#workercounter; i++) { + if (!this.#checkWorkerCompletedFine(i)) + return false; + } + return true; + } + + checkNoRaceConditions() { + // Ensures that no worker has STATE_RUN or STATE_RELEASE between the STATE_HOLD and the + // STATE_RELEASE of other worker, thus ensuring that the mutex works as expected. + let currentWorker = -1; + const onHold = []; + for (const msg of this.#messages) { + switch (msg.state) { + case STATE_HOLD: + if (currentWorker === -1) + currentWorker = msg.id; + else + onHold.push(msg.id); + break; + case STATE_RUN: + if (currentWorker !== -1) { + // Ensure that the worker that enters RUN state is the one that owns the mutex + expect(msg.id).toBe(currentWorker); + } else { + // the mutex is released and a new worker has taken ownership of it + currentWorker = msg.id; + + // Ensure that the worker did a HOLD before + const idx = onHold.indexOf(msg.id); + expect(idx).not.toBe(-1); + + // remove the id from the onHold list + onHold.splice(onHold.indexOf(msg.id), 1); + } + break; + case STATE_RELEASE: + // Ensure that the worker that releases the mutex is the one that owns it + expect(currentWorker).toBe(msg.id); + currentWorker = -1; + break; + } + } + return true; + } +} + + +/* This test confirms that the race condition happens */ +class mutexTestRaceCondition extends baseTest { + run() { + this._disableMutex(); + + this._appendWorker(); + this._appendWorker(); + this._appendWorker(); + this._appendWorker(); + + this._runLoop(); + + expect(this.checkWorkersCompletedFine()).toBe(true); + expect(this.counter).not.toBe(4); + } +} + + +/* This test confirms that the asyncMutex works when called once */ +class mutexTestAsyncMutex extends baseTest { + run() { + this._appendWorker(); + this._runLoop(); + + expect(this.checkWorkersCompletedFine()).toBe(true); + expect(this.counter).toBe(1); + } +} + +/* This test confirms that the asyncMutex fixes the race condition */ +class mutexTestAsyncMutexFix extends baseTest { + run() { + this._appendWorker(); + this._appendWorker(); + this._appendWorker(); + this._appendWorker(); + + this._runLoop(); + + expect(this.checkWorkersCompletedFine()).toBe(true); + expect(this.counter).toBe(4); + expect(this.checkNoRaceConditions()).toBe(true); + } +} + +/* This test confirms that several asyncMutex can be used + symultaneously */ +class mutexTestMultipleUse { + run() { + const mutexA = new Mutex.AsyncMutex(); + const mutexB = new Mutex.AsyncMutex(); + const mutexC = new Mutex.AsyncMutex(); + + mutexA.hold(); + expect(mutexA.pendingResolves).toBe(0); + + mutexB.hold(); + expect(mutexB.pendingResolves).toBe(0); + + mutexA.hold(); + expect(mutexA.pendingResolves).toBe(1); + + mutexB.hold(); + expect(mutexB.pendingResolves).toBe(1); + + mutexC.hold(); + expect(mutexC.pendingResolves).toBe(0); + + mutexC.hold(); + expect(mutexC.pendingResolves).toBe(1); + } +} + +describe('AsyncMutex Test', () => { + it('Confirm that the race condition happens', () => { + const test = new mutexTestRaceCondition(); + test.run(); + }); + it('Test that AsyncMutex works with one worker', () => { + const test = new mutexTestAsyncMutex(); + test.run(); + }); + it('Test that AsyncMutex works with several concurrent workers', () => { + const test = new mutexTestAsyncMutexFix(); + test.run(); + }); + it('Test that several AsyncMutex can be used at the same time without interference', () => { + const test = new mutexTestMultipleUse(); + test.run(); + }); +}); -- GitLab From 14aefb4f32d458eb66ef4d8fea0803945aff8842 Mon Sep 17 00:00:00 2001 From: Sergio Costas Rodriguez Date: Tue, 26 Aug 2025 13:16:12 +0200 Subject: [PATCH 2/2] Fix race condition when enabling extensions When an extension is enabled, it is deleted from the list of disabled extensions and added to the list of enabled extensions. This can cause a race condition, because both cases call the same asynchronous function, resulting in the creation of two instances of each extension, and the `enable` method of each one being called twice. This is possible because the method `onEnabledExtensionsChanged` can be called both from a signal of a change in the `disabled-extensions` property, or from a signal of a change in the `enabled-extensions` property. Since this method is async and does several `await` calls, two very close changes to those properties can trigger it the second time when the first time is in the middle of an `await`, and, thus, in an incomplete state. This patch fixes the bug itself by using the previously defined `asyncMutex` object. --- js/ui/extensionSystem.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/js/ui/extensionSystem.js b/js/ui/extensionSystem.js index 1dc94b40b6..986c6e12df 100644 --- a/js/ui/extensionSystem.js +++ b/js/ui/extensionSystem.js @@ -5,6 +5,7 @@ import St from 'gi://St'; import Shell from 'gi://Shell'; import * as Signals from '../misc/signals.js'; +import * as AsyncMutex from '../misc/asyncMutex.js'; import * as Config from '../misc/config.js'; import * as ExtensionDownloader from './extensionDownloader.js'; import {formatError} from '../misc/errorUtils.js'; @@ -22,6 +23,8 @@ const EXTENSION_DISABLE_VERSION_CHECK_KEY = 'disable-extension-version-validatio const UPDATE_CHECK_TIMEOUT = 24 * 60 * 60; // 1 day in seconds +const enableExtensionMutex = new AsyncMutex.AsyncMutex(); + function stateToString(state) { return Object.keys(ExtensionState).find(k => ExtensionState[k] === state); } @@ -562,6 +565,8 @@ export class ExtensionManager extends Signals.EventEmitter { } async _onEnabledExtensionsChanged() { + await enableExtensionMutex.hold(); + const newEnabledExtensions = this._getEnabledExtensions(); for (const extension of this._extensions.values()) { @@ -595,6 +600,8 @@ export class ExtensionManager extends Signals.EventEmitter { } this._enabledExtensions = newEnabledExtensions; + + enableExtensionMutex.release(); } _onSettingsWritableChanged() { -- GitLab