From 8859d6298cfdd81e97fe320766e820003cb3e436 Mon Sep 17 00:00:00 2001 From: Daniel Lockyer Date: Thu, 4 Aug 2022 12:29:58 +0200 Subject: [PATCH] Added guard for `activateTheme` being called in parallel causing sync bugs (#15053) refs https://github.com/TryGhost/Team/issues/1666 - it seems like we may have a situation where `.activateTheme()` can be called simultaneously resulting in unexpected behaviour in the sync such as duplicate theme setting records - adjusted behaviour to keep track of the currently running activation within the service and if `.activateTheme()` is called again whilst it's in progress it will wait for completion of the first sync before exiting early or continuing with a new activation **Note:** There is a known edge-case if there are _more_ than 2 parallel `.activateTheme()` calls. We don't believe that will be an issue but calling it out in case we do still see duplicated custom setting records being created. Co-authored-by: Kevin Ansfield --- .../lib/service.js | 52 ++++++++++-- .../test/service.test.js | 85 +++++++++++++++++++ 2 files changed, 129 insertions(+), 8 deletions(-) diff --git a/ghost/custom-theme-settings-service/lib/service.js b/ghost/custom-theme-settings-service/lib/service.js index a2a2b839e7..e02d1e4bb5 100644 --- a/ghost/custom-theme-settings-service/lib/service.js +++ b/ghost/custom-theme-settings-service/lib/service.js @@ -20,6 +20,10 @@ module.exports = class CustomThemeSettingsService { this.activeThemeName = null; /** @private */ + this._activatingPromise = null; + this._activatingName = null; + this._activatingSettings = null; + this._repository = new BREAD({model}); this._valueCache = cache; this._activeThemeSettings = {}; @@ -27,21 +31,53 @@ module.exports = class CustomThemeSettingsService { /** * The service only deals with one theme at a time, - * that theme is changed by calling this method with the output from gscan + * that theme is changed by calling this method with the output from gscan. + * + * To avoid syncing issues with activateTheme being called in quick succession, + * any previous/still-running activation promise is awaited before re-starting + * if necessary. * * @param {string} name - the name of the theme (Ghost has different names to themes with duplicate package.json names) * @param {Object} theme - checked theme output from gscan */ async activateTheme(name, theme) { - this.activeThemeName = name; + const activate = async () => { + this.activeThemeName = name; - // add/remove/edit key/value records in the respository to match theme settings - const settings = await this._syncRepositoryWithTheme(name, theme); + // add/remove/edit key/value records in the respository to match theme settings + const settings = await this._syncRepositoryWithTheme(name, theme); - // populate the shared cache with all key/value pairs for this theme - this._populateValueCacheForTheme(theme, settings); - // populate the cache used for exposing full setting details for editing - this._populateInternalCacheForTheme(theme, settings); + // populate the shared cache with all key/value pairs for this theme + this._populateValueCacheForTheme(theme, settings); + // populate the cache used for exposing full setting details for editing + this._populateInternalCacheForTheme(theme, settings); + }; + + if (this._activatingPromise) { + // NOTE: must be calculated before awaiting promise as the promise finishing will clear the properties + const isSameName = name === this._activatingName; + const isSameSettings = JSON.stringify(theme.customSettings) === this._activatingSettings; + + // wait for previous activation to finish + await this._activatingPromise; + + // skip sync if we're re-activating exactly the same theme settings + if (isSameName && isSameSettings) { + return; + } + } + + try { + this._activatingName = name; + this._activatingSettings = JSON.stringify(theme.customSettings); + this._activatingPromise = activate(); + + await this._activatingPromise; + } finally { + this._activatingPromise = null; + this._activatingName = null; + this._activatingSettings = null; + } } /** diff --git a/ghost/custom-theme-settings-service/test/service.test.js b/ghost/custom-theme-settings-service/test/service.test.js index 3c2b8a0e3a..56df5c8a7c 100644 --- a/ghost/custom-theme-settings-service/test/service.test.js +++ b/ghost/custom-theme-settings-service/test/service.test.js @@ -394,6 +394,91 @@ describe('Service', function () { should.exist(model.findAll.getCall(1).firstArg.filter); should.doesNotThrow(() => nql.parse(model.findAll.getCall(1).firstArg.filter)); }); + + it('does not allow simultaneous calls for same theme', async function () { + service.activateTheme('test', { + name: 'test', + customSettings: { + // no change + one: { + type: 'select', + options: ['1', '2'], + default: '2' + }, + // no change + two: { + type: 'select', + options: ['1', '2'], + default: '1' + }, + // new setting + three: { + type: 'select', + options: ['uno', 'dos', 'tres'], + default: 'tres' + } + } + }); + + await service.activateTheme('test', { + name: 'test', + customSettings: { + // no change + one: { + type: 'select', + options: ['1', '2'], + default: '2' + }, + // no change + two: { + type: 'select', + options: ['1', '2'], + default: '1' + }, + // new setting + three: { + type: 'select', + options: ['uno', 'dos', 'tres'], + default: 'tres' + } + } + }); + + // model methods are only called enough times for one .activate call despite being called twice + model.findAll.callCount.should.equal(2); + model.add.callCount.should.equal(1); + + // internal cache is correct + service.listSettings().should.deepEqual([{ + id: 1, + key: 'one', + type: 'select', + options: ['1', '2'], + default: '2', + value: '1' + }, { + id: 2, + key: 'two', + type: 'select', + options: ['1', '2'], + default: '1', + value: '2' + }, { + id: 3, + key: 'three', + type: 'select', + options: ['uno', 'dos', 'tres'], + default: 'tres', + value: 'tres' + }]); + + // external cache is correct + cache.getAll().should.deepEqual({ + one: '1', + two: '2', + three: 'tres' + }); + }); }); describe('listSettings()', function () {