From 70ed9988380baf5bf22b7db6ea569cdd593435d6 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Mon, 22 Feb 2021 14:09:39 +0000 Subject: [PATCH] Replaced db.ready event with direct call to urlservice refs https://github.com/ErisDS/Ghost/commit/dd715b33dc21da721e6c3312a713b5e77fccf453 - this is the last event that is used to trigger part of the standard boot process - events make the code harder to read/reason about - the urlservice is one of the most core and critical components in Ghost, possibly the biggest consumer of time and memory - we want to have the work it is doing front and center so that we can improve it --- core/boot.js | 15 +++-- core/frontend/services/url/Resources.js | 17 +----- test/regression/site/UrlService_spec.js | 9 ++- test/regression/site/site_spec.js | 80 ++++++++++++------------- test/utils/db-utils.js | 2 + test/utils/index.js | 6 +- test/utils/old-integration-utils.js | 6 ++ test/utils/url-service-utils.js | 12 ++-- 8 files changed, 74 insertions(+), 73 deletions(-) diff --git a/core/boot.js b/core/boot.js index 55fe75eb5f..6f430dd5ef 100644 --- a/core/boot.js +++ b/core/boot.js @@ -41,7 +41,7 @@ async function initCore({ghostServer}) { const settings = require('./server/services/settings'); const jobService = require('./server/services/jobs'); const models = require('./server/models'); - const {events, i18n} = require('./server/lib/common'); + const {i18n} = require('./server/lib/common'); ghostServer.registerCleanupTask(async () => { await jobService.shutdown(); @@ -56,13 +56,16 @@ async function initCore({ghostServer}) { await settings.init(); - // @TODO: fix this - has to happen before db.ready is emitted + // The URLService is a core part of Ghost, which depends on models + // It needs moving from the frontend to make this clear debug('Begin: Url Service'); - require('./frontend/services/url'); + const urlService = require('./frontend/services/url'); + // Note: there is no await here, we do not wait for the url service to finish + // We can return, but the site will remain in (the shared, not global) maintenance mode until this finishes + // This is managed on request: https://github.com/TryGhost/Ghost/blob/main/core/server/web/shared/middlewares/maintenance.js#L13 + urlService.init(); debug('End: Url Service'); - // @TODO: fix this location - events.emit('db.ready'); debug('End: initCore'); } @@ -239,7 +242,7 @@ async function bootGhost() { require('./shared/sentry'); debug('End: Load sentry'); - // Start server with minimal app in maintenance mode + // Start server with minimal app in global maintenance mode debug('Begin: load server + minimal app'); const rootApp = require('./app'); const GhostServer = require('./server/ghost-server'); diff --git a/core/frontend/services/url/Resources.js b/core/frontend/services/url/Resources.js index 2ed81fbd44..b625447ed8 100644 --- a/core/frontend/services/url/Resources.js +++ b/core/frontend/services/url/Resources.js @@ -21,7 +21,6 @@ class Resources { this.data = {}; this.listeners = []; - this._listeners(); } /** @@ -41,16 +40,6 @@ class Resources { events.on(eventName, listener); } - /** - * @description Little helper which get's called on class instantiation. It will subscribe to the - * database ready event to start fetching the data as early as possible. - * - * @private - */ - _listeners() { - this._listenOn('db.ready', this.fetchResources.bind(this)); - } - /** * @description Initialise the resource config. We currently fetch the data straight via the the model layer, * but because Ghost supports multiple API versions, we have to ensure we load the correct data. @@ -441,12 +430,8 @@ class Resources { * * @param {Object} options */ - reset(options = {ignoreDBReady: false}) { + reset() { _.each(this.listeners, (obj) => { - if (obj.eventName === 'db.ready' && options.ignoreDBReady) { - return; - } - events.removeListener(obj.eventName, obj.listener); }); diff --git a/test/regression/site/UrlService_spec.js b/test/regression/site/UrlService_spec.js index fb08a3706e..7beb0da034 100644 --- a/test/regression/site/UrlService_spec.js +++ b/test/regression/site/UrlService_spec.js @@ -114,7 +114,8 @@ describe('Integration: services/url/UrlService', function () { events.emit('router.created', router3); events.emit('router.created', router4); - events.emit('db.ready'); + // We can't use our url service utils here, because this is a local copy of the urlService, not the singletone + urlService.init(); let timeout; (function retry() { @@ -318,7 +319,8 @@ describe('Integration: services/url/UrlService', function () { events.emit('router.created', router4); events.emit('router.created', router5); - events.emit('db.ready'); + // We can't use our url service utils here, because this is a local copy of the urlService, not the singletone + urlService.init(); let timeout; (function retry() { @@ -515,7 +517,8 @@ describe('Integration: services/url/UrlService', function () { events.emit('router.created', router4); events.emit('router.created', router5); - events.emit('db.ready'); + // We can't use our url service utils here, because this is a local copy of the urlService, not the singletone + urlService.init(); let timeout; (function retry() { diff --git a/test/regression/site/site_spec.js b/test/regression/site/site_spec.js index bfd7c07d74..6993488c2a 100644 --- a/test/regression/site/site_spec.js +++ b/test/regression/site/site_spec.js @@ -33,7 +33,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }) .then(() => { return appService.init(); @@ -402,7 +402,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -519,7 +519,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -580,7 +580,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -682,7 +682,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -768,7 +768,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -926,7 +926,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -1043,7 +1043,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -1113,7 +1113,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -1172,7 +1172,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -1358,7 +1358,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(10); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -1608,7 +1608,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -1731,7 +1731,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }) .then(() => { return appService.init(); @@ -2100,7 +2100,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -2219,7 +2219,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -2280,7 +2280,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -2382,7 +2382,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -2468,7 +2468,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -2626,7 +2626,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -2743,7 +2743,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -2813,7 +2813,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -2872,7 +2872,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -3058,7 +3058,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(10); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -3308,7 +3308,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -3431,7 +3431,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }) .then(() => { return appService.init(); @@ -3802,7 +3802,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -3919,7 +3919,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -3980,7 +3980,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -4082,7 +4082,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -4168,7 +4168,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -4326,7 +4326,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -4443,7 +4443,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -4513,7 +4513,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -4572,7 +4572,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -4758,7 +4758,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(10); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -5009,7 +5009,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }); }); @@ -5133,7 +5133,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }) .then(() => { return appService.init(); @@ -5252,7 +5252,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }) .then(() => { return appService.init(); @@ -5414,7 +5414,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }) .then(() => { return appService.init(); @@ -5461,7 +5461,7 @@ describe('Integration - Web - Site', function () { sinon.stub(themeService.getActive(), 'config').withArgs('posts_per_page').returns(2); app = siteApp({start: true}); - return testUtils.integrationTesting.urlService.isFinished(); + return testUtils.integrationTesting.urlServiceInitAndWait(); }) .then(() => { return appService.init(); diff --git a/test/utils/db-utils.js b/test/utils/db-utils.js index a3b2d5ee8c..afa642cb24 100644 --- a/test/utils/db-utils.js +++ b/test/utils/db-utils.js @@ -17,6 +17,8 @@ const urlServiceUtils = require('./url-service-utils'); module.exports.initData = async () => { await knexMigrator.init(); + await urlServiceUtils.reset(); + await urlServiceUtils.init(); await urlServiceUtils.isFinished(); }; diff --git a/test/utils/index.js b/test/utils/index.js index 8642874fcb..ffca924d71 100644 --- a/test/utils/index.js +++ b/test/utils/index.js @@ -198,6 +198,7 @@ const restartModeGhostStart = async () => { // Reload the URL service & wait for it to be ready again // @TODO: Prob B: why/how is this different to urlService.resetGenerators? urlServiceUtils.reset(); + urlServiceUtils.init(); await urlServiceUtils.isFinished(); // @TODO: why does this happen _after_ URL service web.shared.middlewares.customRedirects.reload(); @@ -246,13 +247,14 @@ const freshModeGhostStart = async (options) => { // Reset the URL service generators // @TODO: Prob B: why/how is this different to urlService.reset? + // @TODO: why would we do this on a fresh boot?! urlService.resetGenerators(); // Actually boot Ghost await bootGhost(options); - // Wait for the URL service to be ready, which happens after boot, but don't re-trigger db.ready - await urlServiceUtils.isFinished({disableDbReadyEvent: true}); + // Wait for the URL service to be ready, which happens after boot + await urlServiceUtils.isFinished(); }; const startGhost = async (options) => { diff --git a/test/utils/old-integration-utils.js b/test/utils/old-integration-utils.js index 768f4b76c4..1f16972ee6 100644 --- a/test/utils/old-integration-utils.js +++ b/test/utils/old-integration-utils.js @@ -56,5 +56,11 @@ module.exports = { } }, + // Temporary function just for test/regression/site/site_spec.js + async urlServiceInitAndWait() { + urlServiceUtils.init(); + return await urlServiceUtils.isFinished(); + }, + urlService: urlServiceUtils }; diff --git a/test/utils/url-service-utils.js b/test/utils/url-service-utils.js index 5ca01f36d7..19e920cea5 100644 --- a/test/utils/url-service-utils.js +++ b/test/utils/url-service-utils.js @@ -1,13 +1,8 @@ const urlService = require('../../core/frontend/services/url'); -const events = require('../../core/server/lib/common/events'); module.exports.isFinished = async (options = {disableDbReadyEvent: false}) => { let timeout; - if (options.disableDbReadyEvent === false) { - events.emit('db.ready'); - } - return new Promise(function (resolve) { (function retry() { clearTimeout(timeout); @@ -21,11 +16,16 @@ module.exports.isFinished = async (options = {disableDbReadyEvent: false}) => { }); }; +// @TODO: unify all the reset/softTeset helpers so they either work how the main code works or the reasons why they are different are clear +module.exports.init = () => { + urlService.init(); +}; + module.exports.reset = () => { urlService.softReset(); }, module.exports.resetGenerators = () => { urlService.resetGenerators(); - urlService.resources.reset({ignoreDBReady: true}); + urlService.resources.reset(); };