From 71f02d25e9da2bbae727d5c3ead5c6d9b0de02ef Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Sun, 9 Aug 2020 16:40:47 +0100 Subject: [PATCH] Refactored server announce functions to be clearer - the announce functions exist for the purpose of communicating with Ghost CLI - the functions were called announceServerStart and announceServerStopped, which implies they tell Ghost CLI when the server starts and stops - however, the true intention / purpose of these functions is to: - either tell Ghost CLI when Ghost has successfully booted (e.g. is ready to serve requests) - or tell Ghost CLI when the server failed to boot, and report the error so that Ghost CLI can communicate it to the user - therefore, I've refactored the old functions into 1 function to make it clearer they do the same job, but with 2 different states - also added some tests :D --- core/index.js | 2 +- core/server/ghost-server.js | 80 ++++++++------------- core/server/index.js | 4 +- test/unit/server/ghost-server_spec.js | 100 ++++++++++++++++++++++++++ test/utils/index.js | 2 +- 5 files changed, 133 insertions(+), 55 deletions(-) create mode 100644 test/unit/server/ghost-server_spec.js diff --git a/core/index.js b/core/index.js index 1815dff4b5..9b2fe143c4 100644 --- a/core/index.js +++ b/core/index.js @@ -16,7 +16,7 @@ function makeGhost(options) { err = new errors.GhostError({message: err.message, err: err}); } - return GhostServer.announceServerStopped(err) + return GhostServer.announceServerReadiness(err) .finally(() => { throw err; }); diff --git a/core/server/ghost-server.js b/core/server/ghost-server.js index f903f451e6..274ecc9a81 100644 --- a/core/server/ghost-server.js +++ b/core/server/ghost-server.js @@ -29,13 +29,6 @@ function GhostServer(rootApp) { this.config = config; } -const debugInfo = { - versions: process.versions, - platform: process.platform, - arch: process.arch, - release: process.release -}; - /** * ## Public API methods * @@ -109,7 +102,7 @@ GhostServer.prototype.start = function (externalApp) { debug('...Started'); self.logStartMessages(); - return GhostServer.announceServerStart() + return GhostServer.announceServerReadiness() .finally(() => { resolve(self); }); @@ -249,65 +242,50 @@ GhostServer.prototype.logShutdownMessages = function () { module.exports = GhostServer; /** - * @NOTE announceServerStartCalled: + * We call announce server readiness when the server is ready + * When the server is started, but not ready, it is only able to serve 503s * - * - backwards compatible logic, because people complained that not all themes were loaded when using Ghost as NPM module - * - we told them to call `announceServerStart`, which is not required anymore, because we restructured the code + * If the server isn't able to reach readiness, announceServerReadiness is called with an error + * A status message, any error, and debug info are all passed to managing processes via IPC and the bootstrap socket */ -let announceServerStartCalled = false; -module.exports.announceServerStart = function announceServerStart() { - if (announceServerStartCalled || config.get('maintenance:enabled')) { +let announceServerReadinessCalled = false; + +const debugInfo = { + versions: process.versions, + platform: process.platform, + arch: process.arch, + release: process.release +}; + +module.exports.announceServerReadiness = function (error = null) { + // If we already announced readiness, we should not do it again + if (announceServerReadinessCalled) { return Promise.resolve(); } - announceServerStartCalled = true; - let socketAddress = config.get('bootstrap-socket'); + // Mark this function as called + announceServerReadinessCalled = true; + + // Build our message + // - if there's no error then the server is ready let message = { started: true, debug: debugInfo }; - events.emit('server.start'); + // - if there's an error then the server is not ready, include the errors + if (error) { + message.started = false; + message.error = error; + } - // CASE: IPC communication to the CLI via child process. + // CASE: IPC communication to the CLI for local process manager if (process.send) { process.send(message); } - // CASE: Ghost extension - bootstrap sockets - if (socketAddress) { - return bootstrapSocket.connectAndSend(socketAddress, logging, message); - } - - return Promise.resolve(); -}; - -/** - * @NOTE announceServerStopCalled: - * - * - backwards compatible logic, because people complained that not all themes were loaded when using Ghost as NPM module - * - we told them to call `announceServerStart`, which is not required anymore, because we restructured code - */ -let announceServerStopCalled = false; -module.exports.announceServerStopped = function announceServerStopped(error) { - if (announceServerStopCalled) { - return Promise.resolve(); - } - announceServerStopCalled = true; - + // CASE: use bootstrap socket to communicate with CLI for systemd let socketAddress = config.get('bootstrap-socket'); - let message = { - started: false, - error: error, - debug: debugInfo - }; - - // CASE: IPC communication to the CLI via child process. - if (process.send) { - process.send(message); - } - - // CASE: Ghost extension - bootstrap sockets if (socketAddress) { return bootstrapSocket.connectAndSend(socketAddress, logging, message); } diff --git a/core/server/index.js b/core/server/index.js index c4349b9ed9..b7f390a383 100644 --- a/core/server/index.js +++ b/core/server/index.js @@ -138,10 +138,10 @@ const minimalRequiredSetupToStartGhost = (dbState) => { .then(() => { config.set('maintenance:enabled', false); logging.info('Blog is out of maintenance mode.'); - return GhostServer.announceServerStart(); + return GhostServer.announceServerReadiness(); }) .catch((err) => { - return GhostServer.announceServerStopped(err) + return GhostServer.announceServerReadiness(err) .finally(() => { logging.error(err); setTimeout(() => { diff --git a/test/unit/server/ghost-server_spec.js b/test/unit/server/ghost-server_spec.js new file mode 100644 index 0000000000..8829876ae9 --- /dev/null +++ b/test/unit/server/ghost-server_spec.js @@ -0,0 +1,100 @@ +const should = require('should'); +const sinon = require('sinon'); + +const configUtils = require('../../utils/configUtils'); + +const bootstrapSocket = require('@tryghost/bootstrap-socket'); + +describe('GhostServer', function () { + describe('announceServerReadiness', function () { + let GhostServer; + let socketStub; + + beforeEach(function () { + // Have to re-require each time to clear the internal flag + delete require.cache[require.resolve('../../../core/server/ghost-server')]; + GhostServer = require('../../../core/server/ghost-server'); + + // process.send isn't set for tests, we can safely override; + process.send = sinon.stub(); + + // stub socket connectAndSend method + socketStub = sinon.stub(bootstrapSocket, 'connectAndSend'); + }); + + afterEach(function () { + process.send = undefined; + configUtils.restore(); + socketStub.restore(); + }); + + it('it resolves a promise', function () { + GhostServer.announceServerReadiness().should.be.fulfilled(); + }); + + it('it communicates with IPC correctly on success', function () { + GhostServer.announceServerReadiness(); + + process.send.calledOnce.should.be.true(); + + let message = process.send.firstCall.args[0]; + message.should.be.an.Object().with.properties('started', 'debug'); + message.should.not.have.property('error'); + message.started.should.be.true(); + }); + + it('communicates with IPC correctly on failure', function () { + GhostServer.announceServerReadiness(new Error('something went wrong')); + + process.send.calledOnce.should.be.true(); + + let message = process.send.firstCall.args[0]; + message.should.be.an.Object().with.properties('started', 'debug', 'error'); + message.started.should.be.false(); + message.error.should.be.an.Object().with.properties('message'); + message.error.message.should.eql('something went wrong'); + }); + + it('communicates via bootstrap socket correctly on success', function () { + configUtils.set('bootstrap-socket', 'testing'); + + GhostServer.announceServerReadiness(); + + socketStub.calledOnce.should.be.true(); + socketStub.firstCall.args[0].should.eql('testing'); + socketStub.firstCall.args[1].should.be.an.Object().with.properties('info', 'warn'); + + let message = socketStub.firstCall.args[2]; + message.should.be.an.Object().with.properties('started', 'debug'); + message.should.not.have.property('error'); + message.started.should.be.true(); + }); + + it('communicates via bootstrap socket correctly on failure', function () { + configUtils.set('bootstrap-socket', 'testing'); + + GhostServer.announceServerReadiness(new Error('something went wrong')); + + socketStub.calledOnce.should.be.true(); + socketStub.firstCall.args[0].should.eql('testing'); + socketStub.firstCall.args[1].should.be.an.Object().with.properties('info', 'warn'); + + let message = socketStub.firstCall.args[2]; + message.should.be.an.Object().with.properties('started', 'debug', 'error'); + message.started.should.be.false(); + message.error.should.be.an.Object().with.properties('message'); + message.error.message.should.eql('something went wrong'); + }); + + it('can be called multiple times, but only communicates once', function () { + configUtils.set('bootstrap-socket', 'testing'); + + GhostServer.announceServerReadiness(); + GhostServer.announceServerReadiness(new Error('something went wrong')); + GhostServer.announceServerReadiness(); + + process.send.calledOnce.should.be.true(); + socketStub.calledOnce.should.be.true(); + }); + }); +}); diff --git a/test/utils/index.js b/test/utils/index.js index 49b355a025..a972798383 100644 --- a/test/utils/index.js +++ b/test/utils/index.js @@ -946,7 +946,7 @@ startGhost = function startGhost(options) { .then(function () { let timeout; - GhostServer.announceServerStart(); + GhostServer.announceServerReadiness(); return new Promise(function (resolve) { (function retry() {