From fce275e1d4b7864b3d64ee238aeb7b4d26d3af3e Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Fri, 3 Jan 2020 10:06:19 +0000 Subject: [PATCH] Post-decaf cleanup of app.js --- services/filestore/app.js | 125 ++++-------------- services/filestore/app/js/ExceptionHandler.js | 97 ++++++++++++++ .../test/acceptance/js/FilestoreApp.js | 1 + 3 files changed, 127 insertions(+), 96 deletions(-) create mode 100644 services/filestore/app/js/ExceptionHandler.js diff --git a/services/filestore/app.js b/services/filestore/app.js index 9e76107ea6..d80514738c 100644 --- a/services/filestore/app.js +++ b/services/filestore/app.js @@ -1,89 +1,40 @@ -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -/* eslint-disable node/no-deprecated-api */ const Metrics = require('metrics-sharelatex') +const logger = require('logger-sharelatex') + Metrics.initialize('filestore') +logger.initialize('filestore') + +const settings = require('settings-sharelatex') const express = require('express') const bodyParser = require('body-parser') -let logger = require('logger-sharelatex') -logger.initialize('filestore') -const settings = require('settings-sharelatex') + const fileController = require('./app/js/FileController') const bucketController = require('./app/js/BucketController') const keyBuilder = require('./app/js/KeyBuilder') const healthCheckController = require('./app/js/HealthCheckController') -const domain = require('domain') -let appIsOk = true -const app = express() +const ExceptionHandler = require('./app/js/ExceptionHandler') +const exceptionHandler = new ExceptionHandler() -if ((settings.sentry != null ? settings.sentry.dsn : undefined) != null) { +const app = express() +app.exceptionHandler = exceptionHandler + +if (settings.sentry && settings.sentry.dsn) { logger.initializeErrorReporting(settings.sentry.dsn) } Metrics.open_sockets.monitor(logger) -if (Metrics.event_loop != null) { +Metrics.memory.monitor(logger) +if (Metrics.event_loop) { Metrics.event_loop.monitor(logger) } -Metrics.memory.monitor(logger) app.use(Metrics.http.monitor(logger)) - app.use(function(req, res, next) { Metrics.inc('http-request') - return next() + next() }) -app.use(function(req, res, next) { - const requestDomain = domain.create() - requestDomain.add(req) - requestDomain.add(res) - requestDomain.on('error', function(err) { - try { - // request a shutdown to prevent memory leaks - beginShutdown() - if (!res.headerSent) { - res.send(500, 'uncaught exception') - } - logger = require('logger-sharelatex') - req = { - body: req.body, - headers: req.headers, - url: req.url, - key: req.key, - statusCode: req.statusCode - } - err = { - message: err.message, - stack: err.stack, - name: err.name, - type: err.type, - arguments: err.arguments - } - return logger.err( - { err, req, res }, - 'uncaught exception thrown on request' - ) - } catch (exception) { - return logger.err( - { err: exception }, - 'exception in request domain handler' - ) - } - }) - return requestDomain.run(next) -}) - -app.use(function(req, res, next) { - if (!appIsOk) { - // when shutting down, close any HTTP keep-alive connections - res.set('Connection', 'close') - } - return next() -}) +exceptionHandler.addMiddleware(app) Metrics.injectMetricsRoute(app) @@ -108,7 +59,7 @@ app.put( bodyParser.json(), fileController.copyFile ) -app.del( +app.delete( '/project/:project_id/file/:file_id', keyBuilder.userFileKeyMiddleware, fileController.deleteFile @@ -156,7 +107,7 @@ app.put( bodyParser.json(), fileController.copyFile ) -app.del( +app.delete( '/project/:project_id/public/:public_file_id', keyBuilder.publicFileKeyMiddleware, fileController.deleteFile @@ -183,68 +134,50 @@ app.get('/heapdump', (req, res, next) => ) app.post('/shutdown', function(req, res) { - appIsOk = false - return res.send() + exceptionHandler.setNotOk() + res.sendStatus(200) }) app.get('/status', function(req, res) { - if (appIsOk) { - return res.send('filestore sharelatex up') + if (exceptionHandler.appIsOk()) { + res.send('filestore sharelatex up') } else { logger.log('app is not ok - shutting down') - return res.send('server is being shut down', 500) + res.send('server is being shut down').status(500) } }) app.get('/health_check', healthCheckController.check) -app.get('*', (req, res) => res.send(404)) - -var beginShutdown = function() { - if (appIsOk) { - appIsOk = false - // hard-terminate this process if graceful shutdown fails - const killTimer = setTimeout(() => process.exit(1), 120 * 1000) - if (typeof killTimer.unref === 'function') { - killTimer.unref() - } // prevent timer from keeping process alive - server.close(function() { - logger.log('closed all connections') - Metrics.close() - return typeof process.disconnect === 'function' - ? process.disconnect() - : undefined - }) - return logger.log('server will stop accepting connections') - } -} +app.get('*', (req, res) => res.sendStatus(404)) const port = settings.internal.filestore.port || 3009 const host = '0.0.0.0' if (!module.parent) { // Called directly - var server = app.listen(port, host, error => { + const server = app.listen(port, host, error => { if (error) { logger.error('Error starting Filestore', error) throw error } logger.info(`Filestore starting up, listening on ${host}:${port}`) }) + exceptionHandler.server = server } module.exports = app process.on('SIGTERM', function() { logger.log('filestore got SIGTERM, shutting down gracefully') - return beginShutdown() + exceptionHandler.beginShutdown() }) -if (global.gc != null) { +if (global.gc) { const oneMinute = 60 * 1000 const gcTimer = setInterval(function() { global.gc() - return logger.log(process.memoryUsage(), 'global.gc') + logger.log(process.memoryUsage(), 'global.gc') }, 3 * oneMinute) gcTimer.unref() } diff --git a/services/filestore/app/js/ExceptionHandler.js b/services/filestore/app/js/ExceptionHandler.js new file mode 100644 index 0000000000..122e78805c --- /dev/null +++ b/services/filestore/app/js/ExceptionHandler.js @@ -0,0 +1,97 @@ +const Metrics = require('metrics-sharelatex') +const logger = require('logger-sharelatex') + +// TODO: domain has been deprecated for some time - do we need it and is there a better way? + +// eslint-disable-next-line node/no-deprecated-api +const domain = require('domain') + +const TWO_MINUTES = 120 * 1000 + +class ExceptionHandler { + constructor() { + this._appIsOk = true + } + + beginShutdown() { + if (this._appIsOk) { + this._appIsOk = false + + // hard-terminate this process if graceful shutdown fails + const killTimer = setTimeout(() => process.exit(1), TWO_MINUTES) + + if (typeof killTimer.unref === 'function') { + killTimer.unref() + } // prevent timer from keeping process alive + + this.server.close(function() { + logger.log('closed all connections') + Metrics.close() + if (typeof process.disconnect === 'function') { + process.disconnect() + } + }) + logger.log('server will stop accepting connections') + } + } + + addMiddleware(app) { + app.use(this.middleware.bind(this)) + } + + appIsOk() { + return this._appIsOk + } + + setNotOk() { + this._appIsOk = false + } + + middleware(req, res, next) { + const rescueLogger = require('logger-sharelatex') + const requestDomain = domain.create() + requestDomain.add(req) + requestDomain.add(res) + requestDomain.on('error', err => { + try { + // request a shutdown to prevent memory leaks + this.beginShutdown() + if (!res.headerSent) { + res.send('uncaught exception').status(500) + } + req = { + body: req.body, + headers: req.headers, + url: req.url, + key: req.key, + statusCode: req.statusCode + } + err = { + message: err.message, + stack: err.stack, + name: err.name, + type: err.type, + arguments: err.arguments + } + rescueLogger.err( + { err, req, res }, + 'uncaught exception thrown on request' + ) + } catch (exception) { + rescueLogger.err( + { err: exception }, + 'exception in request domain handler' + ) + } + }) + + if (!this._appIsOk) { + // when shutting down, close any HTTP keep-alive connections + res.set('Connection', 'close') + } + + requestDomain.run(next) + } +} + +module.exports = ExceptionHandler diff --git a/services/filestore/test/acceptance/js/FilestoreApp.js b/services/filestore/test/acceptance/js/FilestoreApp.js index 718d53bcf8..32ec7a5adf 100644 --- a/services/filestore/test/acceptance/js/FilestoreApp.js +++ b/services/filestore/test/acceptance/js/FilestoreApp.js @@ -44,6 +44,7 @@ class FilestoreApp { resolve() } ) + this.app.exceptionHandler.server = this.server }) if (Settings.filestore.backend === 's3') {