From fce275e1d4b7864b3d64ee238aeb7b4d26d3af3e Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Fri, 3 Jan 2020 10:06:19 +0000 Subject: [PATCH 1/3] 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') { From 6a679023d30bc13cbc9fb18a1ba96f7547363cd3 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Sat, 4 Jan 2020 17:15:21 +0000 Subject: [PATCH 2/3] Fix order of .status().send() --- services/filestore/app.js | 2 +- services/filestore/app/js/ExceptionHandler.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/services/filestore/app.js b/services/filestore/app.js index d80514738c..1e84440eb6 100644 --- a/services/filestore/app.js +++ b/services/filestore/app.js @@ -143,7 +143,7 @@ app.get('/status', function(req, res) { res.send('filestore sharelatex up') } else { logger.log('app is not ok - shutting down') - res.send('server is being shut down').status(500) + res.status(500).send('server is being shut down') } }) diff --git a/services/filestore/app/js/ExceptionHandler.js b/services/filestore/app/js/ExceptionHandler.js index 122e78805c..d52c00bb2c 100644 --- a/services/filestore/app/js/ExceptionHandler.js +++ b/services/filestore/app/js/ExceptionHandler.js @@ -57,7 +57,7 @@ class ExceptionHandler { // request a shutdown to prevent memory leaks this.beginShutdown() if (!res.headerSent) { - res.send('uncaught exception').status(500) + res.status(500).send('uncaught exception') } req = { body: req.body, From 85d3c0a852d1ebc6b0118dc7cc477b87bb0fc13d Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Mon, 6 Jan 2020 15:43:24 +0000 Subject: [PATCH 3/3] Remove old exception-handling and shutdown-related mechanisms --- services/filestore/app.js | 36 +------ services/filestore/app/js/ExceptionHandler.js | 97 ------------------- .../test/acceptance/js/FilestoreApp.js | 1 - 3 files changed, 2 insertions(+), 132 deletions(-) delete mode 100644 services/filestore/app/js/ExceptionHandler.js diff --git a/services/filestore/app.js b/services/filestore/app.js index 1e84440eb6..232c5b24bc 100644 --- a/services/filestore/app.js +++ b/services/filestore/app.js @@ -12,11 +12,8 @@ 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 ExceptionHandler = require('./app/js/ExceptionHandler') -const exceptionHandler = new ExceptionHandler() const app = express() -app.exceptionHandler = exceptionHandler if (settings.sentry && settings.sentry.dsn) { logger.initializeErrorReporting(settings.sentry.dsn) @@ -34,8 +31,6 @@ app.use(function(req, res, next) { next() }) -exceptionHandler.addMiddleware(app) - Metrics.injectMetricsRoute(app) app.head( @@ -133,51 +128,24 @@ app.get('/heapdump', (req, res, next) => ) ) -app.post('/shutdown', function(req, res) { - exceptionHandler.setNotOk() - res.sendStatus(200) -}) - app.get('/status', function(req, res) { - if (exceptionHandler.appIsOk()) { - res.send('filestore sharelatex up') - } else { - logger.log('app is not ok - shutting down') - res.status(500).send('server is being shut down') - } + res.send('filestore sharelatex up') }) app.get('/health_check', healthCheckController.check) -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 - const server = app.listen(port, host, error => { + 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') - exceptionHandler.beginShutdown() -}) - -if (global.gc) { - const oneMinute = 60 * 1000 - const gcTimer = setInterval(function() { - 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 deleted file mode 100644 index d52c00bb2c..0000000000 --- a/services/filestore/app/js/ExceptionHandler.js +++ /dev/null @@ -1,97 +0,0 @@ -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.status(500).send('uncaught exception') - } - 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 32ec7a5adf..718d53bcf8 100644 --- a/services/filestore/test/acceptance/js/FilestoreApp.js +++ b/services/filestore/test/acceptance/js/FilestoreApp.js @@ -44,7 +44,6 @@ class FilestoreApp { resolve() } ) - this.app.exceptionHandler.server = this.server }) if (Settings.filestore.backend === 's3') {