fix(perf): Reduce use of synchronous IO calls (#404)

* remove synchronous call from consent controller * 🧹 remove HttpRequestFileCache() * uploadedFile.js :: deleteFile() * log slow requests to stderr instead of access.log (we don't need that file anymore, now that we use datadog) * 🧹 makeWhydPageFromFile() * fix: adapt 404 responses based on format * reunite page404 rendering code * 🧹 renderAsyncWhydPageFromTemplateFile()
openwhyd · Nov 22, 2020 · 2cb41b3 · 2cb41b3
1 parent 66294ce
commit 2cb41b3
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 149 deletions.
diff --git a/app.js b/app.js
@@ -124,15 +124,20 @@ function start() {
     port: params.port,
     appDir: __dirname,
     sessionMiddleware,
-    errorHandler: function (req, params, response, statusCode) {
+    errorHandler: function (req, params = {}, response, statusCode) {
       // to render 404 and 401 error pages from server/router
       console.log(
         `rendering server error page ${statusCode} for ${req.method} ${req.path}`
       );
       require('./app/templates/error.js').renderErrorResponse(
         { errorCode: statusCode },
         response,
-        (params || {}).format,
+        params.format ||
+          (req.accepts('html')
+            ? 'html'
+            : req.accepts('json')
+            ? 'json'
+            : 'text'),
         req.getUser()
       );
     },

diff --git a/app/controllers/consent.js b/app/controllers/consent.js
@@ -34,24 +34,27 @@ function renderMarkdownLine(mdLine) {
     .replace(/<\/(li|h1)><\/p>$/, '</$1>');
 }
 
-var templatePerLang = Object.keys(filePerLang).reduce(function (
-  templatePerLang,
-  langId
-) {
-  var html = fs
-    .readFileSync(filePerLang[langId], { encoding: 'utf8' })
-    .toString()
-    .split('\n')
-    .filter(removeEmptyLine)
-    .map(renderMarkdownLine)
-    .join('\n');
-  var langObj = {};
-  langObj[langId] = html;
-  return Object.assign({}, templatePerLang, langObj);
-},
-{});
+const promisedTemplatePerLang = Object.entries(filePerLang).reduce(
+  (acc, [langId, langTemplate]) => {
+    const promisedHtml = fs.promises
+      .readFile(langTemplate, { encoding: 'utf8' })
+      .then((res) =>
+        res
+          .toString()
+          .split('\n')
+          .filter(removeEmptyLine)
+          .map(renderMarkdownLine)
+          .join('\n')
+      );
+    return {
+      ...acc,
+      [langId]: promisedHtml,
+    };
+  },
+  {}
+);
 
-function renderPageContent(params) {
+async function renderPageContent(params) {
   var safeRedirect = snip.sanitizeJsStringInHtml(params.redirect || '/');
   // credits: flag icons by Freepik, https://www.flaticon.com/packs/countrys-flags
   return [
@@ -61,13 +64,13 @@ function renderPageContent(params) {
     '    <img alt="French / Français" id="lang-fr" src="/images/lang-fr.svg">',
     '  </div>',
     '  <form class="whitePanel lang-en" action="/consent" method="POST">',
-    templatePerLang.en,
+    await promisedTemplatePerLang.en,
     '    <input type="hidden" name="lang" value="en">',
     '    <input type="hidden" name="redirect" value="' + safeRedirect + '">',
     '    <input disabled class="consent-submit" type="submit">',
     '  </form>',
     '  <form class="whitePanel lang-fr" action="/consent" method="POST">',
-    templatePerLang.fr,
+    await promisedTemplatePerLang.fr,
     '    <input type="hidden" name="lang" value="fr">',
     '    <input type="hidden" name="redirect" value="' + safeRedirect + '">',
     '    <input disabled class="consent-submit" type="submit">',
@@ -93,7 +96,7 @@ function renderPageContent(params) {
   ].join('\n');
 }
 
-exports.controller = function (request, getParams, response) {
+exports.controller = async function (request, getParams, response) {
   var isPost = request.method.toLowerCase() === 'post';
   var p = (isPost ? request.body : getParams) || {};
   request.logToConsole('consent.controller ' + request.method, p);
@@ -142,7 +145,7 @@ exports.controller = function (request, getParams, response) {
   } else {
     (p.css = p.css || []).push('consent.css');
     p.bodyClass = 'pgConsent';
-    p.content = renderPageContent(p);
+    p.content = await renderPageContent(p);
     p.redirect = ''; // to avoid redirection loops
     render(p);
   }

diff --git a/app/controllers/uploadedFile.js b/app/controllers/uploadedFile.js
@@ -24,7 +24,7 @@ exports.config.uPlaylistPath =
 const NO_IMAGE_PATH = exports.config.whydPath + '/public/images/no_image.png';
 
 // create upload dirs
-var dirMode = 0755;
+var dirMode = 0755; // eslint-disable-line no-octal
 var dirsToCreate = [
   exports.config.uploadPath,
   exports.config.uAvatarImgPath,
@@ -69,13 +69,11 @@ exports.actualFilePath = function (filepath) {
 };
 
 exports.deleteFile = function (_filepath) {
-  try {
-    const filepath = exports.actualFilePath(_filepath);
-    console.log('deleting ' + filepath);
-    fs.unlinkSync(filepath);
-  } catch (e) {
-    console.log(e, e.stack);
-  }
+  const filepath = exports.actualFilePath(_filepath);
+  console.log('deleting ' + filepath);
+  return fs.promises
+    .unlink(filepath)
+    .catch((err) => console.log(err, err.stack));
 };
 
 exports.renameTo = function (filename, toFilename, callback) {

diff --git a/app/lib/my-http-wrapper/http/Application.js b/app/lib/my-http-wrapper/http/Application.js
@@ -53,18 +53,17 @@ const makeBodyParser = (uploadSettings) =>
     });
   };
 
-const makeStatsUpdater = ({ accessLogFile }) =>
+const makeStatsUpdater = () =>
   function statsUpdater(req, res, next) {
     const startDate = new Date();
     const userId = (req.session || {}).whydUid;
     const userAgent = req.headers['user-agent'];
 
     sessionTracker.notifyUserActivity({ startDate, userId, userAgent }); // maintain lastAccessPerUA
 
-    // whenever a request is slow to respond, append log entry to _accessLogFile
+    // log whenever a request is slow to respond
     res.on('finish', () => {
       appendSlowQueryToAccessLog({
-        accessLogFile,
         startDate,
         req,
         userId,
@@ -93,7 +92,6 @@ exports.Application = class Application {
     this._appDir = options.appDir + '/app';
     this._publicDir = options.appDir + '/public';
     this._routeFile = options.appDir + '/config/app.route';
-    this._accessLogFile = options.appDir + '/access.log';
     this._port = options.port;
     this._expressApp = null; // will be lazy-loaded by getExpressApp()
     this._uploadSettings = options.uploadSettings;
@@ -107,7 +105,7 @@ exports.Application = class Application {
     app.use(express.static(this._publicDir));
     app.use(makeBodyParser(this._uploadSettings)); // parse uploads and arrays from query params
     this._sessionMiddleware && app.use(this._sessionMiddleware);
-    app.use(makeStatsUpdater({ accessLogFile: this._accessLogFile }));
+    app.use(makeStatsUpdater());
     attachLegacyRoutesFromFile(app, this._appDir, this._routeFile);
     app.use(makeNotFound(this._errorHandler));
     return (this._expressApp = app);
@@ -181,13 +179,7 @@ function attachLegacyRoutesFromFile(expressApp, appDir, routeFile) {
   });
 }
 
-function appendSlowQueryToAccessLog({
-  accessLogFile,
-  startDate,
-  req,
-  userId,
-  userAgent,
-}) {
+function appendSlowQueryToAccessLog({ startDate, req, userId, userAgent }) {
   const duration = Date.now() - startDate;
   if (duration < LOG_THRESHOLD) return;
   const logLine = [
@@ -198,5 +190,5 @@ function appendSlowQueryToAccessLog({
   ];
   if (userId) logLine.push('uid=' + userId);
   if (userAgent) logLine.push('ua=' + sessionTracker.stripUserAgent(userAgent));
-  fs.appendFileSync(accessLogFile, logLine.join(' ') + '\n');
+  console.error('slow request:', logLine.join(' '));
 }
diff --git a/app/snip.js b/app/snip.js
@@ -690,62 +690,6 @@ exports.HttpRequestCache = function (cache) {
   };
 };
 
-exports.HttpRequestFileCache = function (fileName) {
-  exports.HttpRequestCache.apply(this);
-  try {
-    // try to restore from file, if it exists
-    this.cache.restore(
-      JSON.parse(fs.readFileSync(fileName, { encoding: 'utf8' }))
-    );
-  } catch (e) {
-    console.error(e);
-  }
-  this.save = function () {
-    fs.writeFileSync(fileName, JSON.stringify(this.cache.dump()), {
-      encoding: 'utf8',
-    });
-  };
-};
-/*
-// testing HttpRequestCache
-console.log("\n\n");
-var cache = new exports.HttpRequestCache();
-cache.httpRequest("http://google.com", null, function(err, data){
-	console.log(err || data.length);
-	cache.httpRequest("http://google.com", null, function(err, data){
-		console.log(err || data.length);
-		var dump = cache.cache.dump();
-		console.log("dump", dump);
-		cache = new exports.HttpRequestCache();
-		console.log("new cache dump", cache.cache.dump());
-		cache.cache.restore(dump);
-		console.log("new cache restored", cache.cache.dump());
-		cache.httpRequest("http://google.com", null, function(err, data){ console.log(err || data.length); });
-		console.log("\n\n");
-	});
-});
-*/
-/*
-// testing HttpRequestFileCache
-console.log("\n\n");
-var FILENAME = "./httpCache.json";
-try { fs.unlinkSync(FILENAME); } catch(e) { };
-var cache = new exports.HttpRequestFileCache(FILENAME);
-cache.httpRequest("http://google.com", null, function(err, data){
-	console.log(err || data.length);
-	cache.httpRequest("http://google.com", null, function(err, data){
-		console.log(err || data.length);
-		var dump = cache.cache.dump();
-		console.log("dump", dump);
-		cache.save();
-		cache = new exports.HttpRequestFileCache(FILENAME);
-		console.log("new cache restored", cache.cache.dump());
-		cache.httpRequest("http://google.com", null, function(err, data){ console.log(err || data.length); });
-		console.log("\n\n");
-	});
-});
-*/
-
 // =========================================================================
 /**
  * simple implementation of an async event emitter

diff --git a/app/templates/error.js b/app/templates/error.js
@@ -3,12 +3,17 @@
  * @author adrienjoly, whyd
  **/
 
+const fs = require('fs');
 var snip = require('../snip.js');
 var mainTemplate = require('../templates/mainTemplate.js');
 
-var renderPage404 = mainTemplate.makeWhydPageRendererFromFile(
-  'public/html/404.html'
-);
+const page404Html = fs.readFileSync('public/html/404.html', 'utf8');
+
+const renderPage404 = (params) =>
+  mainTemplate.renderWhydPage({
+    ...params,
+    content: page404Html,
+  });
 
 exports.ERRORCODE = {
   401: 'Unauthorized. Please login before accessing this page.',
@@ -42,28 +47,36 @@ exports.renderErrorCode = function (errorCode, loggedUser) {
 exports.renderErrorResponse = function (
   errorObj,
   response,
-  format,
+  format = 'html',
   loggedUser
 ) {
   var statusCode =
     errorObj && typeof errorObj.errorCode === 'number' && errorObj.errorCode;
   //var format = (querystring.parse(url.parse(request.url).query) || {}).format || "";
-  if ((format || '').toLowerCase() == 'json') {
+  if (format.toLowerCase() == 'json') {
     errorObj.error = errorObj.error || exports.ERRORCODE[errorObj.errorCode];
     response.renderJSON(errorObj, statusCode);
   } else if (
     errorObj.errorCode == 404 ||
     errorObj.errorCode == 'USER_NOT_FOUND'
-  )
-    //response.sendFile("public/html/404.html");
-    response.renderHTML(
-      renderPage404({
-        pageTitle: 'Oops...',
-        loggedUser: loggedUser,
-      }),
-      statusCode
-    );
-  else if (errorObj.errorCode)
+  ) {
+    response.status(404);
+    if (format === 'html') {
+      //response.sendFile("public/html/404.html");
+      response.renderHTML(
+        renderPage404({
+          pageTitle: 'Oops...',
+          loggedUser: loggedUser,
+        }),
+        statusCode
+      );
+      // TODO: response.render('404', { url: req.url });
+    } else if (format === 'json') {
+      response.send({ error: 'Not found' });
+    } else {
+      response.type('txt').send('Not found');
+    }
+  } else if (errorObj.errorCode)
     response.renderHTML(
       exports.renderErrorCode(errorObj.errorCode, loggedUser),
       statusCode

diff --git a/app/templates/mainTemplate.js b/app/templates/mainTemplate.js
@@ -8,7 +8,6 @@ var fs = require('fs');
 var util = require('util');
 var snip = require('../snip.js');
 var uiSnippets = snip;
-var templateLoader = require('../templates/templateLoader.js');
 var config = require('../models/config.js');
 var render = { urlPrefix: '' };
 
@@ -414,36 +413,3 @@ var params = {
 */
 
 // MINIMAL EXAMPLE OF USE: /admin/testMainTemplate.js
-
-exports.makeWhydPageFromFile = function (path, params) {
-  params = params || {};
-  params.content = fs.readFileSync(path, 'utf8');
-  return exports.renderWhydPage(params);
-};
-
-exports.makeWhydPageRendererFromFile = function (path) {
-  var content = fs.readFileSync(path, 'utf8');
-  return function (params) {
-    params = params || {};
-    params.content = content;
-    return exports.renderWhydPage(params);
-  };
-};
-
-exports.renderAsyncWhydPageFromTemplateFile = function (
-  templateFilePath,
-  templateParams,
-  whydPageParams,
-  cb,
-  forceReload
-) {
-  templateLoader.loadTemplate(
-    templateFilePath,
-    function (template) {
-      whydPageParams = whydPageParams || {};
-      whydPageParams.content = template.render(templateParams);
-      cb(exports.renderWhydPage(whydPageParams));
-    },
-    forceReload
-  );
-};