Skip to content

Commit

Permalink
Removed experimental URL cache
Browse files Browse the repository at this point in the history
fix https://linear.app/ghost/issue/ENG-1803/remove-url-cache-code

- this feature was intended to write a static cache of our URLs from the
  URL service, which should help boot become faster because we don't
  have to fetch them from the DB
- however, we added this a long time ago and never gave it the love to
  get it going correctly, so it's been sitting there unused for years
  and not providing any real benefit
- little bit of spring cleaning, we can clean up the code here and
  potentially revisit it in the future
  • Loading branch information
daniellockyer committed Feb 10, 2025
1 parent c7b631b commit 6b2f4de
Show file tree
Hide file tree
Showing 16 changed files with 25 additions and 2,111 deletions.
8 changes: 3 additions & 5 deletions ghost/core/core/boot.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,8 @@ async function initDatabase({config}) {
* @param {object} options.ghostServer
* @param {object} options.config
* @param {object} options.bootLogger
* @param {boolean} options.frontend
*/
async function initCore({ghostServer, config, bootLogger, frontend}) {
async function initCore({ghostServer, config, bootLogger}) {
debug('Begin: initCore');

// URL Utils is a bit slow, put it here so the timing is visible separate from models
Expand Down Expand Up @@ -118,8 +117,7 @@ async function initCore({ghostServer, config, bootLogger, frontend}) {
onFinished: () => {
bootLogger.metric('url-service', urlServiceStart);
bootLogger.log('URL Service Ready');
},
urlCache: !frontend // hacky parameter to make the cache initialization kick in as we can't initialize labs before the boot
}
});
debug('End: Url Service');

Expand Down Expand Up @@ -563,7 +561,7 @@ async function bootGhost({backend = true, frontend = true, server = true} = {})

// Step 4 - Load Ghost with all its services
debug('Begin: Load Ghost Services & Apps');
await initCore({ghostServer, config, bootLogger, frontend});
await initCore({ghostServer, config, bootLogger});

// Instrument the knex instance and connection pool if prometheus is enabled
// Needs to be after initCore because the pool is destroyed and recreated in initCore, which removes the event listeners
Expand Down
75 changes: 0 additions & 75 deletions ghost/core/core/server/services/url/LocalFileCache.js

This file was deleted.

52 changes: 10 additions & 42 deletions ghost/core/core/server/services/url/UrlService.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
const debug = require('@tryghost/debug')('services:url:service');
const _ = require('lodash');
const errors = require('@tryghost/errors');
const labs = require('../../../shared/labs');
const UrlGenerator = require('./UrlGenerator');
const Queue = require('./Queue');
const Urls = require('./Urls');
Expand All @@ -15,16 +14,8 @@ const resourcesConfig = require('./config');
* It will tell you if the url generation is in progress or not.
*/
class UrlService {
/**
*
* @param {Object} [options]
* @param {Object} [options.cache] - cache handler instance
* @param {Function} [options.cache.read] - read cache by type
* @param {Function} [options.cache.write] - write into cache by type
*/
constructor({cache} = {}) {
constructor() {
this.utils = urlUtils;
this.cache = cache;
this.onFinished = null;
this.finished = false;
this.urlGenerators = [];
Expand Down Expand Up @@ -304,44 +295,21 @@ class UrlService {
* @description Initializes components needed for the URL Service to function
* @param {Object} options
* @param {Function} [options.onFinished] - callback when url generation is finished
* @param {Boolean} [options.urlCache] - whether to init using url cache or not
*/
async init({onFinished, urlCache} = {}) {
async init({onFinished} = {}) {
this.onFinished = onFinished;

let persistedUrls;
let persistedResources;

if (this.cache && (labs.isSet('urlCache') || urlCache)) {
persistedUrls = await this.cache.read('urls');
persistedResources = await this.cache.read('resources');
}

if (persistedUrls && persistedResources) {
this.urls.urls = persistedUrls;
this.resources.data = persistedResources;
this.resources.initEventListeners();

this._onQueueEnded('init');
} else {
this.resources.initEventListeners();
await this.resources.fetchResources();
// CASE: all resources are fetched, start the queue
this.queue.start({
event: 'init',
tolerance: 100,
requiredSubscriberCount: 1
});
}
this.resources.initEventListeners();
await this.resources.fetchResources();
// CASE: all resources are fetched, start the queue
this.queue.start({
event: 'init',
tolerance: 100,
requiredSubscriberCount: 1
});
}

async shutdown() {
if (!labs.isSet('urlCache')) {
return null;
}

await this.cache.write('urls', this.urls.urls);
await this.cache.write('resources', this.resources.getAll());
}

/**
Expand Down
22 changes: 1 addition & 21 deletions ghost/core/core/server/services/url/index.js
Original file line number Diff line number Diff line change
@@ -1,26 +1,6 @@
const config = require('../../../shared/config');
const LocalFileCache = require('./LocalFileCache');
const UrlService = require('./UrlService');

// NOTE: instead of a path we could give UrlService a "data-resolver" of some sort
// so it doesn't have to contain the logic to read data at all. This would be
// a possible improvement in the future
let writeDisabled = false;
let storagePath = config.getContentPath('data');

// TODO: remove this hack in favor of loading from the content path when it's possible to do so
// by mocking content folders in pre-boot phase
if (process.env.NODE_ENV.startsWith('test')){
storagePath = config.get('paths').urlCache;

// NOTE: prevents test suites from overwriting cache fixtures.
// A better solution would be injecting a different implementation of the
// cache based on the environment, this approach should do the trick for now
writeDisabled = true;
}

const cache = new LocalFileCache({storagePath, writeDisabled});
const urlService = new UrlService({cache});
const urlService = new UrlService();

// Singleton
module.exports = urlService;
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@
"useMinFiles": false,
"paths": {
"fixtures": "test/utils/fixtures/fixtures",
"defaultSettings": "test/utils/fixtures/default-settings-browser.json",
"urlCache": "test/utils/fixtures/urls"
"defaultSettings": "test/utils/fixtures/default-settings-browser.json"
},
"enableDeveloperExperiments": true
}
3 changes: 1 addition & 2 deletions ghost/core/core/shared/config/env/config.testing-mysql.json
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@
"useMinFiles": false,
"paths": {
"fixtures": "test/utils/fixtures/fixtures",
"defaultSettings": "test/utils/fixtures/default-settings.json",
"urlCache": "test/utils/fixtures/urls"
"defaultSettings": "test/utils/fixtures/default-settings.json"
}
}
3 changes: 1 addition & 2 deletions ghost/core/core/shared/config/env/config.testing.json
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@
"useMinFiles": false,
"paths": {
"fixtures": "test/utils/fixtures/fixtures",
"defaultSettings": "test/utils/fixtures/default-settings.json",
"urlCache": "test/utils/fixtures/urls"
"defaultSettings": "test/utils/fixtures/default-settings.json"
}
}
1 change: 0 additions & 1 deletion ghost/core/core/shared/labs.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ const BETA_FEATURES = [

const ALPHA_FEATURES = [
'NestPlayground',
'urlCache',
'emailCustomization',
'mailEvents',
'collectionsCard',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ Object {
"postsX": true,
"stripeAutomaticTax": true,
"themeErrorsNotification": true,
"urlCache": true,
"webmentions": true,
},
"mail": "",
Expand Down
88 changes: 0 additions & 88 deletions ghost/core/test/unit/server/services/url/LocalFileCache.test.js

This file was deleted.

1 change: 0 additions & 1 deletion ghost/core/test/unit/shared/config/loader.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@ describe('Config Loader', function () {
'fixtures',
'defaultSettings',
'assetSrc',
'urlCache',
'appRoot',
'corePath',
'adminAssets',
Expand Down
10 changes: 5 additions & 5 deletions ghost/core/test/unit/shared/labs.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,26 +33,26 @@ describe('Labs Service', function () {
sinon.stub(process.env, 'NODE_ENV').value('production');
sinon.stub(settingsCache, 'get');
settingsCache.get.withArgs('labs').returns({
urlCache: true
NestPlayground: true
});

// NOTE: this test should be rewritten to test the alpha flag independently of the internal ALPHA_FEATURES list
// otherwise we end up in the endless maintenance loop and need to update it every time a feature graduates from alpha
assert.deepEqual(labs.getAll(), expectedLabsObject({
urlCache: true,
NestPlayground: true,
members: true
}));

assert.equal(labs.isSet('members'), true);
assert.equal(labs.isSet('urlCache'), true);
assert.equal(labs.isSet('NestPlayground'), true);
});

it('returns a falsy alpha flag when dev experiments in NOT toggled', function () {
configUtils.set('enableDeveloperExperiments', false);
sinon.stub(process.env, 'NODE_ENV').value('production');
sinon.stub(settingsCache, 'get');
settingsCache.get.withArgs('labs').returns({
urlCache: true
NestPlayground: true
});

// NOTE: this test should be rewritten to test the alpha flag independently of the internal ALPHA_FEATURES list
Expand All @@ -62,7 +62,7 @@ describe('Labs Service', function () {
}));

assert.equal(labs.isSet('members'), true);
assert.equal(labs.isSet('urlCache'), false);
assert.equal(labs.isSet('NestPlayground'), false);
});

it('respects the value in config over settings', function () {
Expand Down
Loading

0 comments on commit 6b2f4de

Please sign in to comment.