Refactor startup/shutdown for tests

* `src/node/server.js` can now be run as a script (for normal
    operation) or imported as a module (for tests).
  * Move shutdown actions to `src/node/server.js` to be close to the
    startup actions.
  * Put startup and shutdown in functions so that tests can call them.
  * Use `await` instead of callbacks.
  * Block until the HTTP server is listening to avoid races during
    test startup.
  * Add a new `shutdown` hook.
  * Use the `shutdown` hook to:
      * close the HTTP server
      * call `end()` on the stats collection to cancel its timers
      * call `terminate()` on the Threads.Pool to stop the workers
  * Exit with exit code 0 (instead of 1) on SIGTERM.
  * Export the HTTP server so that tests can get the HTTP server's
    port via `server.address().port` when `settings.port` is 0.

doc/api/hooks_server-side.md

@@ -10,6 +10,28 @@ Things in context:
 
 Use this hook to receive the global settings in your plugin.
 
+## shutdown
+Called from: src/node/server.js
+
+Things in context: None
+
+This hook runs before shutdown. Use it to stop timers, close sockets and files,
+flush buffers, etc. The database is not available while this hook is running.
+The shutdown function must not block for long because there is a short timeout
+before the process is forcibly terminated.
+
+The shutdown function must return a Promise, which must resolve to `undefined`.
+Returning `callback(value)` will return a Promise that is resolved to `value`.
+
+Example:
+
+```
+// using an async function
+exports.shutdown = async (hookName, context) => {
+  await flushBuffers();
+};
+```
+
 ## pluginUninstall
 Called from: src/static/js/pluginfw/installer.js
 

src/ep.json

@@ -1,10 +1,14 @@
 {
   "parts": [
+    { "name": "DB", "hooks": { "shutdown": "ep_etherpad-lite/node/db/DB" } },
+    { "name": "Minify", "hooks": { "shutdown": "ep_etherpad-lite/node/utils/Minify" } },
     { "name": "express", "hooks": {
-      "createServer": "ep_etherpad-lite/node/hooks/express:createServer",
+      "createServer": "ep_etherpad-lite/node/hooks/express",
-      "restartServer": "ep_etherpad-lite/node/hooks/express:restartServer"
+      "restartServer": "ep_etherpad-lite/node/hooks/express",
+      "shutdown": "ep_etherpad-lite/node/hooks/express"
       } },
     { "name": "static", "hooks": { "expressCreateServer": "ep_etherpad-lite/node/hooks/express/static:expressCreateServer" } },
+    { "name": "stats", "hooks": { "shutdown": "ep_etherpad-lite/node/stats" } },
     { "name": "i18n", "hooks": { "expressCreateServer": "ep_etherpad-lite/node/hooks/i18n:expressCreateServer" } },
     { "name": "specialpages", "hooks": { "expressCreateServer": "ep_etherpad-lite/node/hooks/express/specialpages:expressCreateServer" } },
     { "name": "padurlsanitize", "hooks": { "expressCreateServer": "ep_etherpad-lite/node/hooks/express/padurlsanitize:expressCreateServer" } },

src/node/db/DB.js

@@ -71,3 +71,8 @@ exports.init = function() {
     });
   });
 }
+
+exports.shutdown = async (hookName, context) => {
+  await exports.doShutdown();
+  console.log('Database closed');
+};

src/node/hooks/express.js

@@ -5,18 +5,20 @@ var fs = require('fs');
 var path = require('path');
 var npm = require("npm/lib/npm.js");
 var  _ = require("underscore");
+const util = require('util');
 
-var server;
 var serverName;
 
-exports.createServer = function () {
+exports.server = null;
+
+exports.createServer = async () => {
   console.log("Report bugs at https://github.com/ether/etherpad-lite/issues")
 
   serverName = `Etherpad ${settings.getGitCommit()} (https://etherpad.org)`;
 
   console.log(`Your Etherpad version is ${settings.getEpVersion()} (${settings.getGitCommit()})`);
 
-  exports.restartServer();
+  await exports.restartServer();
 
   if (settings.ip === "") {
     // using Unix socket for connectivity
@@ -38,10 +40,10 @@ exports.createServer = function () {
   }
 }
 
-exports.restartServer = function () {
+exports.restartServer = async () => {
-  if (server) {
+  if (exports.server) {
     console.log("Restarting express server");
-    server.close();
+    await util.promisify(exports.server.close).bind(exports.server)();
   }
 
   var app = express(); // New syntax for express v3
@@ -65,10 +67,10 @@ exports.restartServer = function () {
     }
 
     var https = require('https');
-    server = https.createServer(options, app);
+    exports.server = https.createServer(options, app);
   } else {
     var http = require('http');
-    server = http.createServer(app);
+    exports.server = http.createServer(app);
   }
 
   app.use(function(req, res, next) {
@@ -110,7 +112,12 @@ exports.restartServer = function () {
   }
 
   hooks.callAll("expressConfigure", {"app": app});
-  hooks.callAll("expressCreateServer", {"app": app, "server": server});
+  hooks.callAll('expressCreateServer', {app, server: exports.server});
 
-  server.listen(settings.port, settings.ip);
+  await util.promisify(exports.server.listen).bind(exports.server)(settings.port, settings.ip);
-}
+};
+
+exports.shutdown = async (hookName, context) => {
+  if (!exports.server) return;
+  await util.promisify(exports.server.close).bind(exports.server)();
+};

src/node/hooks/express/adminsettings.js

@@ -46,11 +46,10 @@ exports.socketio = function (hook_name, args, cb) {
       });
     });
 
-    socket.on("restartServer", function () {
+    socket.on('restartServer', async () => {
       console.log("Admin request to restart server through a socket on /admin/settings");
       settings.reloadSettings();
-      hooks.aCallAll("restartServer", {}, function () {});
+      await hooks.aCallAll('restartServer');
-
     });
 
   });

src/node/hooks/express/errorhandling.js

@@ -1,39 +1,5 @@
-var os = require("os");
-var db = require('../../db/DB');
 var stats = require('ep_etherpad-lite/node/stats')
 
-
-exports.onShutdown = false;
-exports.gracefulShutdown = function(err) {
-  if(err && err.stack) {
-    console.error(err.stack);
-  } else if(err) {
-    console.error(err);
-  }
-
-  // ensure there is only one graceful shutdown running
-  if (exports.onShutdown) {
-    return;
-  }
-
-  exports.onShutdown = true;
-
-  console.log("graceful shutdown...");
-
-  // do the db shutdown
-  db.doShutdown().then(function() {
-    console.log("db sucessfully closed.");
-
-    process.exit(0);
-  });
-
-  setTimeout(function() {
-    process.exit(1);
-  }, 3000);
-}
-
-process.on('uncaughtException', exports.gracefulShutdown);
-
 exports.expressCreateServer = function (hook_name, args, cb) {
   exports.app = args.app;
 
@@ -46,28 +12,4 @@ exports.expressCreateServer = function (hook_name, args, cb) {
     console.error(err.stack? err.stack : err.toString());
     stats.meter('http500').mark()
   });
-
-  /*
-   * Connect graceful shutdown with sigint and uncaught exception
-   *
-   * Until Etherpad 1.7.5, process.on('SIGTERM') and process.on('SIGINT') were
-   * not hooked up under Windows, because old nodejs versions did not support
-   * them.
-   *
-   * According to nodejs 6.x documentation, it is now safe to do so. This
-   * allows to gracefully close the DB connection when hitting CTRL+C under
-   * Windows, for example.
-   *
-   * Source: https://nodejs.org/docs/latest-v6.x/api/process.html#process_signal_events
-   *
-   *   - SIGTERM is not supported on Windows, it can be listened on.
-   *   - SIGINT from the terminal is supported on all platforms, and can usually
-   *     be generated with <Ctrl>+C (though this may be configurable). It is not
-   *     generated when terminal raw mode is enabled.
-   */
-  process.on('SIGINT', exports.gracefulShutdown);
-
-  // when running as PID1 (e.g. in docker container)
-  // allow graceful shutdown on SIGTERM c.f. #3265
-  process.on('SIGTERM', exports.gracefulShutdown);
 }

src/node/server.js

@@ -21,65 +21,112 @@
  * limitations under the License.
  */
 
-const log4js = require('log4js')
+const log4js = require('log4js');
-  , NodeVersion = require('./utils/NodeVersion')
-  , UpdateCheck = require('./utils/UpdateCheck')
-  ;
-
 log4js.replaceConsole();
 
 /*
  * early check for version compatibility before calling
  * any modules that require newer versions of NodeJS
  */
+const NodeVersion = require('./utils/NodeVersion');
 NodeVersion.enforceMinNodeVersion('10.13.0');
-
-/*
- * Etherpad 1.8.3 will require at least nodejs 10.13.0.
- */
 NodeVersion.checkDeprecationStatus('10.13.0', '1.8.3');
 
-// Check if Etherpad version is up-to-date
+const UpdateCheck = require('./utils/UpdateCheck');
-UpdateCheck.check();
+const db = require('./db/DB');
+const express = require('./hooks/express');
+const hooks = require('ep_etherpad-lite/static/js/pluginfw/hooks');
+const npm = require('npm/lib/npm.js');
+const plugins = require('ep_etherpad-lite/static/js/pluginfw/plugins');
+const settings = require('./utils/Settings');
+const util = require('util');
 
-/*
+let started = false;
- * start up stats counting system
+let stopped = false;
- */
-var stats = require('./stats');
-stats.gauge('memoryUsage', function() {
-  return process.memoryUsage().rss;
-});
 
-/*
+exports.start = async () => {
- * no use of let or await here because it would cause startup
+  if (started) return;
- * to fail completely on very early versions of NodeJS
+  started = true;
- */
+  if (stopped) throw new Error('restart not supported');
-var npm = require("npm/lib/npm.js");
 
-npm.load({}, function() {
+  // Check if Etherpad version is up-to-date
-  var settings = require('./utils/Settings');
+  UpdateCheck.check();
-  var db = require('./db/DB');
-  var plugins = require("ep_etherpad-lite/static/js/pluginfw/plugins");
-  var hooks = require("ep_etherpad-lite/static/js/pluginfw/hooks");
 
-  db.init()
+  // start up stats counting system
-    .then(plugins.update)
+  const stats = require('./stats');
-    .then(function() {
+  stats.gauge('memoryUsage', () => process.memoryUsage().rss);
-      console.info("Installed plugins: " + plugins.formatPluginsWithVersion());
-      console.debug("Installed parts:\n" + plugins.formatParts());
-      console.debug("Installed hooks:\n" + plugins.formatHooks());
 
-      // Call loadSettings hook
+  await util.promisify(npm.load)();
-      hooks.aCallAll("loadSettings", { settings: settings });
 
-      // initalize the http server
+  try {
-      hooks.callAll("createServer", {});
+    await db.init();
-    })
+    await plugins.update();
-    .catch(function(e) {
+    console.info('Installed plugins: ' + plugins.formatPluginsWithVersion());
-      console.error("exception thrown: " + e.message);
+    console.debug('Installed parts:\n' + plugins.formatParts());
-      if (e.stack) {
+    console.debug('Installed hooks:\n' + plugins.formatHooks());
-        console.log(e.stack);
+    await hooks.aCallAll('loadSettings', {settings});
-      }
+    await hooks.aCallAll('createServer');
-      process.exit(1);
+  } catch (e) {
-    });
+    console.error('exception thrown: ' + e.message);
-});
+    if (e.stack) console.log(e.stack);
+    process.exit(1);
+  }
+
+  process.on('uncaughtException', exports.exit);
+
+  /*
+   * Connect graceful shutdown with sigint and uncaught exception
+   *
+   * Until Etherpad 1.7.5, process.on('SIGTERM') and process.on('SIGINT') were
+   * not hooked up under Windows, because old nodejs versions did not support
+   * them.
+   *
+   * According to nodejs 6.x documentation, it is now safe to do so. This
+   * allows to gracefully close the DB connection when hitting CTRL+C under
+   * Windows, for example.
+   *
+   * Source: https://nodejs.org/docs/latest-v6.x/api/process.html#process_signal_events
+   *
+   *   - SIGTERM is not supported on Windows, it can be listened on.
+   *   - SIGINT from the terminal is supported on all platforms, and can usually
+   *     be generated with <Ctrl>+C (though this may be configurable). It is not
+   *     generated when terminal raw mode is enabled.
+   */
+  process.on('SIGINT', exports.exit);
+
+  // When running as PID1 (e.g. in docker container) allow graceful shutdown on SIGTERM c.f. #3265.
+  // Pass undefined to exports.exit because this is not an abnormal termination.
+  process.on('SIGTERM', () => exports.exit());
+
+  // Return the HTTP server to make it easier to write tests.
+  return express.server;
+};
+
+exports.stop = async () => {
+  if (stopped) return;
+  stopped = true;
+  console.log('Stopping Etherpad...');
+  await new Promise(async (resolve, reject) => {
+    const id = setTimeout(() => reject(new Error('Timed out waiting for shutdown tasks')), 3000);
+    await hooks.aCallAll('shutdown');
+    clearTimeout(id);
+    resolve();
+  });
+};
+
+exports.exit = async (err) => {
+  let exitCode = 0;
+  if (err) {
+    exitCode = 1;
+    console.error(err.stack ? err.stack : err);
+  }
+  try {
+    await exports.stop();
+  } catch (err) {
+    exitCode = 1;
+    console.error(err.stack ? err.stack : err);
+  }
+  process.exit(exitCode);
+};
+
+if (require.main === module) exports.start();

src/node/stats.js

@@ -1,3 +1,7 @@
 var measured = require('measured-core')
 
 module.exports = measured.createCollection();
+
+module.exports.shutdown = async (hookName, context) => {
+  module.exports.end();
+};

src/node/utils/Minify.js

@@ -418,3 +418,7 @@ exports.minify = minify;
 
 exports.requestURI = requestURI;
 exports.requestURIs = requestURIs;
+
+exports.shutdown = async (hookName, context) => {
+  await threadsPool.terminate();
+};

src/package.json

@@ -92,7 +92,7 @@
     "url": "https://github.com/ether/etherpad-lite.git"
   },
   "scripts": {
-    "test": "nyc wtfnode node_modules/.bin/_mocha --timeout 5000 --recursive ../tests/backend/specs ../node_modules/ep_*/static/tests/backend/specs",
+    "test": "nyc wtfnode node_modules/.bin/_mocha --timeout 30000 --recursive ../tests/backend/specs ../node_modules/ep_*/static/tests/backend/specs",
     "test-container": "nyc mocha --timeout 5000 ../tests/container/specs/api"
   },
   "version": "1.8.6",

tests/backend/specs/socketio.js

@@ -1,51 +1,30 @@
 function m(mod) { return __dirname + '/../../../src/' + mod; }
 
 const assert = require('assert').strict;
-const db = require(m('node/db/DB'));
+const io = require(m('node_modules/socket.io-client'));
-const express = require(m('node_modules/express'));
-const http = require('http');
 const log4js = require(m('node_modules/log4js'));
-let padManager;
+const padManager = require(m('node/db/PadManager'));
 const plugins = require(m('static/js/pluginfw/plugin_defs'));
+const server = require(m('node/server'));
 const setCookieParser = require(m('node_modules/set-cookie-parser'));
 const settings = require(m('node/utils/Settings'));
-const io = require(m('node_modules/socket.io-client'));
-const stats = require(m('node/stats'));
 const supertest = require(m('node_modules/supertest'));
-const util = require('util');
 
 const logger = log4js.getLogger('test');
-const app = express();
-const server = http.createServer(app);
 let client;
 let baseUrl;
 
 before(async () => {
-  await util.promisify(server.listen).bind(server)(0, 'localhost');
+  settings.port = 0;
-  baseUrl = `http://localhost:${server.address().port}`;
+  settings.ip = 'localhost';
+  const httpServer = await server.start();
+  baseUrl = `http://localhost:${httpServer.address().port}`;
   logger.debug(`HTTP server at ${baseUrl}`);
   client = supertest(baseUrl);
-  const npm = require(m('node_modules/npm/lib/npm.js'));
-  await util.promisify(npm.load)();
-  settings.users = {
-    admin: {password: 'admin-password', is_admin: true},
-    user: {password: 'user-password'},
-  };
-  await db.init();
-  padManager = require(m('node/db/PadManager'));
-  const webaccess = require(m('node/hooks/express/webaccess'));
-  webaccess.expressConfigure('expressConfigure', {app});
-  const socketio = require(m('node/hooks/express/socketio'));
-  socketio.expressCreateServer('expressCreateServer', {app, server});
-  app.get(/./, (req, res) => { res.status(200).send('OK'); });
 });
 
 after(async () => {
-  stats.end();
+  await server.stop();
-  await Promise.all([
-    db.doShutdown(),
-    util.promisify(server.close).bind(server)(),
-  ]);
 });
 
 // Waits for and returns the next named socket.io event. Rejects if there is any error while waiting
@@ -129,16 +108,23 @@ const handshake = async (socket, padID) => {
 };
 
 describe('socket.io access checks', () => {
+  const settingsBackup = {};
   let socket;
   beforeEach(async () => {
+    Object.assign(settingsBackup, settings);
     assert(socket == null);
     settings.requireAuthentication = false;
     settings.requireAuthorization = false;
+    settings.users = {
+      admin: {password: 'admin-password', is_admin: true},
+      user: {password: 'user-password'},
+    };
     Promise.all(['pad', 'other-pad'].map(async (pad) => {
       if (await padManager.doesPadExist(pad)) (await padManager.getPad(pad)).remove();
     }));
   });
   afterEach(async () => {
+    Object.assign(settings, settingsBackup);
     if (socket) socket.close();
     socket = null;
   });