fix: #8120, bubble errors from static hooks

src/plugins/hooks.js

@@ -148,40 +148,40 @@ module.exports = function (Plugins) {
 		if (!Array.isArray(hookList) || !hookList.length) {
 			return;
 		}
+		// don't bubble errors from these hooks, so bad plugins don't stop startup
+		const noErrorHooks = ['static:app.load', 'static:assets.prepare', 'static:app.preload'];
 		await async.each(hookList, function (hookObj, next) {
-			if (typeof hookObj.method === 'function') {
+			if (typeof hookObj.method !== 'function') {
-				let timedOut = false;
+				return next();
-				const timeoutId = setTimeout(function () {
+			}
-					winston.warn('[plugins] Callback timed out, hook \'' + hook + '\' in plugin \'' + hookObj.id + '\'');
+
-					timedOut = true;
+			let timedOut = false;
-					next();
+			const timeoutId = setTimeout(function () {
-				}, 5000);
+				winston.warn('[plugins] Callback timed out, hook \'' + hook + '\' in plugin \'' + hookObj.id + '\'');
-
+				timedOut = true;
-				const onError = (err) => {
+				next();
+			}, 5000);
+
+			const callback = (err) => {
+				clearTimeout(timeoutId);
+				if (err) {
 					winston.error('[plugins] Error executing \'' + hook + '\' in plugin \'' + hookObj.id + '\'');
-					winston.error(err);
+					winston.error(err.stack);
-					clearTimeout(timeoutId);
-					next();
-				};
-				const callback = (...args) => {
-					clearTimeout(timeoutId);
-					if (!timedOut) {
-						next(...args);
-					}
-				};
-				try {
-					const returned = hookObj.method(params, callback);
-					if (utils.isPromise(returned)) {
-						returned.then(
-							payload => setImmediate(callback, null, payload),
-							err => setImmediate(onError, err)
-						);
-					}
-				} catch (err) {
-					onError(err);
 				}
-			} else {
+				if (!timedOut) {
-				next();
+					next(noErrorHooks.includes(hook) ? null : err);
+				}
+			};
+			try {
+				const returned = hookObj.method(params, callback);
+				if (utils.isPromise(returned)) {
+					returned.then(
+						payload => setImmediate(callback, null, payload),
+						err => setImmediate(callback, err)
+					);
+				}
+			} catch (err) {
+				callback(err);
 			}
 		});
 	}

test/plugins.js

@@ -120,7 +120,7 @@ describe('Plugins', function () {
 		});
 	});
 
-	it('should register and fire a static hook returning a promise that gets rejected with a warning only', function (done) {
+	it('should register and fire a static hook returning a promise that gets rejected with a error', function (done) {
 		function method(data) {
 			assert.equal(data.bar, 'test');
 			return new Promise(function (resolve, reject) {
@@ -129,7 +129,8 @@ describe('Plugins', function () {
 		}
 		plugins.registerHook('test-plugin', { hook: 'static:test.hook', method: method });
 		plugins.fireHook('static:test.hook', { bar: 'test' }, function (err) {
-			assert.ifError(err);
+			assert.strictEqual(err.message, 'just because');
+			plugins.unregisterHook('test-plugin', 'static:test.hook', method);
 			done();
 		});
 	});
@@ -144,6 +145,7 @@ describe('Plugins', function () {
 		plugins.registerHook('test-plugin', { hook: 'static:test.hook', method: method });
 		plugins.fireHook('static:test.hook', { bar: 'test' }, function (err) {
 			assert.ifError(err);
+			plugins.unregisterHook('test-plugin', 'static:test.hook', method);
 			done();
 		});
 	});