import/export: Improve error handling, promisify

src/node/handler/ExportHandler.js

@@ -33,24 +33,12 @@ const util = require('util');
 const fsp_writeFile = util.promisify(fs.writeFile);
 const fsp_unlink = util.promisify(fs.unlink);
 
-let convertor = null;
-
-// load abiword only if it is enabled
-if (settings.abiword != null) {
-  convertor = require('../utils/Abiword');
-}
-
-// Use LibreOffice if an executable has been defined in the settings
-if (settings.soffice != null) {
-  convertor = require('../utils/LibreOffice');
-}
-
 const tempDirectory = os.tmpdir();
 
 /**
  * do a requested export
  */
-const doExport = async (req, res, padId, readOnlyId, type) => {
+exports.doExport = async (req, res, padId, readOnlyId, type) => {
   // avoid naming the read-only file as the original pad's id
   let fileName = readOnlyId ? readOnlyId : padId;
 
@@ -85,7 +73,7 @@ const doExport = async (req, res, padId, readOnlyId, type) => {
       const newHTML = await hooks.aCallFirst('exportHTMLSend', html);
       if (newHTML.length) html = newHTML;
       res.send(html);
-      throw 'stop';
+      return;
     }
 
     // else write the html export to a file
@@ -98,20 +86,19 @@ const doExport = async (req, res, padId, readOnlyId, type) => {
     html = null;
     await TidyHtml.tidy(srcFile);
 
-    // send the convert job to the convertor (abiword, libreoffice, ..)
+    // send the convert job to the converter (abiword, libreoffice, ..)
     const destFile = `${tempDirectory}/etherpad_export_${randNum}.${type}`;
 
     // Allow plugins to overwrite the convert in export process
     const result = await hooks.aCallAll('exportConvert', {srcFile, destFile, req, res});
     if (result.length > 0) {
       // console.log("export handled by plugin", destFile);
     } else {
-      // @TODO no Promise interface for convertors (yet)
-      await new Promise((resolve, reject) => {
-        convertor.convertFile(srcFile, destFile, type, (err) => {
-          err ? reject('convertFailed') : resolve();
-        });
-      });
+      const converter =
+          settings.soffice != null ? require('../utils/LibreOffice')
+          : settings.abiword != null ? require('../utils/Abiword')
+          : null;
+      await converter.convertFile(srcFile, destFile, type);
     }
 
     // send the file
@@ -128,11 +115,3 @@ const doExport = async (req, res, padId, readOnlyId, type) => {
     await fsp_unlink(destFile);
   }
 };
-
-exports.doExport = (req, res, padId, readOnlyId, type) => {
-  doExport(req, res, padId, readOnlyId, type).catch((err) => {
-    if (err !== 'stop') {
-      throw err;
-    }
-  });
-};

src/node/handler/ImportHandler.js

@@ -55,17 +55,17 @@ const rm = async (path) => {
   }
 };
 
-let convertor = null;
+let converter = null;
 let exportExtension = 'htm';
 
 // load abiword only if it is enabled and if soffice is disabled
 if (settings.abiword != null && settings.soffice == null) {
-  convertor = require('../utils/Abiword');
+  converter = require('../utils/Abiword');
 }
 
 // load soffice only if it is enabled
 if (settings.soffice != null) {
-  convertor = require('../utils/LibreOffice');
+  converter = require('../utils/LibreOffice');
   exportExtension = 'html';
 }
 
@@ -80,8 +80,8 @@ const doImport = async (req, res, padId) => {
   // set html in the pad
   const randNum = Math.floor(Math.random() * 0xFFFFFFFF);
 
-  // setting flag for whether to use convertor or not
-  let useConvertor = (convertor != null);
+  // setting flag for whether to use converter or not
+  let useConverter = (converter != null);
 
   const form = new formidable.IncomingForm();
   form.keepExtensions = true;
@@ -170,30 +170,25 @@ const doImport = async (req, res, padId) => {
   // convert file to html if necessary
   if (!importHandledByPlugin && !directDatabaseAccess) {
     if (fileIsTXT) {
-      // Don't use convertor for text files
-      useConvertor = false;
+      // Don't use converter for text files
+      useConverter = false;
     }
 
     // See https://github.com/ether/etherpad-lite/issues/2572
-    if (fileIsHTML || !useConvertor) {
-      // if no convertor only rename
+    if (fileIsHTML || !useConverter) {
+      // if no converter only rename
       await fs.rename(srcFile, destFile);
     } else {
-      // @TODO - no Promise interface for convertors (yet)
-      await new Promise((resolve, reject) => {
-        convertor.convertFile(srcFile, destFile, exportExtension, (err) => {
-          // catch convert errors
-          if (err) {
-            logger.warn(`Converting Error: ${err.stack || err}`);
-            return reject(new ImportError('convertFailed'));
-          }
-          resolve();
-        });
-      });
+      try {
+        await converter.convertFile(srcFile, destFile, exportExtension);
+      } catch (err) {
+        logger.warn(`Converting Error: ${err.stack || err}`);
+        throw new ImportError('convertFailed');
+      }
     }
   }
 
-  if (!useConvertor && !directDatabaseAccess) {
+  if (!useConverter && !directDatabaseAccess) {
     // Read the file with no encoding for raw buffer access.
     const buf = await fs.readFile(destFile);
 
@@ -224,7 +219,7 @@ const doImport = async (req, res, padId) => {
 
   // change text of the pad and broadcast the changeset
   if (!directDatabaseAccess) {
-    if (importHandledByPlugin || useConvertor || fileIsHTML) {
+    if (importHandledByPlugin || useConverter || fileIsHTML) {
       try {
         await importHtml.setPadHTML(pad, text);
       } catch (err) {

src/node/hooks/express/importexport.js

@@ -60,7 +60,7 @@ exports.expressCreateServer = (hookName, args, cb) => {
         }
 
         console.log(`Exporting pad "${req.params.pad}" in ${req.params.type} format`);
-        exportHandler.doExport(req, res, padId, readOnlyId, req.params.type);
+        await exportHandler.doExport(req, res, padId, readOnlyId, req.params.type);
       }
     })().catch((err) => next(err || new Error(err)));
   });

src/node/utils/Abiword.js

@@ -24,111 +24,64 @@ const async = require('async');
 const settings = require('./Settings');
 const os = require('os');
 
-let doConvertTask;
-
 // on windows we have to spawn a process for each convertion,
 // cause the plugin abicommand doesn't exist on this platform
 if (os.type().indexOf('Windows') > -1) {
-  let stdoutBuffer = '';
-
-  doConvertTask = (task, callback) => {
-    // span an abiword process to perform the conversion
-    const abiword = spawn(settings.abiword, [`--to=${task.destFile}`, task.srcFile]);
-
-    // delegate the processing of stdout to another function
-    abiword.stdout.on('data', (data) => {
-      // add data to buffer
-      stdoutBuffer += data.toString();
-    });
-
-    // append error messages to the buffer
-    abiword.stderr.on('data', (data) => {
-      stdoutBuffer += data.toString();
-    });
-
-    // throw exceptions if abiword is dieing
-    abiword.on('exit', (code) => {
-      if (code !== 0) {
-        return callback(`Abiword died with exit code ${code}`);
-      }
-
-      if (stdoutBuffer !== '') {
-        console.log(stdoutBuffer);
-      }
-
-      callback();
+  exports.convertFile = async (srcFile, destFile, type) => {
+    const abiword = spawn(settings.abiword, [`--to=${destFile}`, srcFile]);
+    let stdoutBuffer = '';
+    abiword.stdout.on('data', (data) => { stdoutBuffer += data.toString(); });
+    abiword.stderr.on('data', (data) => { stdoutBuffer += data.toString(); });
+    await new Promise((resolve, reject) => {
+      abiword.on('exit', (code) => {
+        if (code !== 0) return reject(new Error(`Abiword died with exit code ${code}`));
+        if (stdoutBuffer !== '') {
+          console.log(stdoutBuffer);
+        }
+        resolve();
+      });
     });
   };
-
-  exports.convertFile = (srcFile, destFile, type, callback) => {
-    doConvertTask({srcFile, destFile, type}, callback);
-  };
   // on unix operating systems, we can start abiword with abicommand and
   // communicate with it via stdin/stdout
   // thats much faster, about factor 10
 } else {
-  // spawn the abiword process
   let abiword;
   let stdoutCallback = null;
   const spawnAbiword = () => {
     abiword = spawn(settings.abiword, ['--plugin', 'AbiCommand']);
     let stdoutBuffer = '';
     let firstPrompt = true;
-
-    // append error messages to the buffer
-    abiword.stderr.on('data', (data) => {
-      stdoutBuffer += data.toString();
-    });
-
-    // abiword died, let's restart abiword and return an error with the callback
+    abiword.stderr.on('data', (data) => { stdoutBuffer += data.toString(); });
     abiword.on('exit', (code) => {
       spawnAbiword();
-      stdoutCallback(`Abiword died with exit code ${code}`);
+      if (stdoutCallback != null) stdoutCallback(new Error(`Abiword died with exit code ${code}`));
     });
-
-    // delegate the processing of stdout to a other function
     abiword.stdout.on('data', (data) => {
-      // add data to buffer
       stdoutBuffer += data.toString();
-
       // we're searching for the prompt, cause this means everything we need is in the buffer
       if (stdoutBuffer.search('AbiWord:>') !== -1) {
-        // filter the feedback message
-        const err = stdoutBuffer.search('OK') !== -1 ? null : stdoutBuffer;
-
-        // reset the buffer
+        const err = stdoutBuffer.search('OK') !== -1 ? null : new Error(stdoutBuffer);
         stdoutBuffer = '';
-
-        // call the callback with the error message
-        // skip the first prompt
         if (stdoutCallback != null && !firstPrompt) {
           stdoutCallback(err);
           stdoutCallback = null;
         }
-
         firstPrompt = false;
       }
     });
   };
   spawnAbiword();
 
-  doConvertTask = (task, callback) => {
+  const queue = async.queue((task, callback) => {
     abiword.stdin.write(`convert ${task.srcFile} ${task.destFile} ${task.type}\n`);
-    // create a callback that calls the task callback and the caller callback
     stdoutCallback = (err) => {
-      callback();
-      console.log('queue continue');
-      try {
-        task.callback(err);
-      } catch (e) {
-        console.error('Abiword File failed to convert', e);
-      }
+      if (err != null) console.error('Abiword File failed to convert', err);
+      callback(err);
     };
-  };
+  }, 1);
 
-  // Queue with the converts we have to do
-  const queue = async.queue(doConvertTask, 1);
-  exports.convertFile = (srcFile, destFile, type, callback) => {
-    queue.push({srcFile, destFile, type, callback});
+  exports.convertFile = async (srcFile, destFile, type) => {
+    await queue.pushAsync({srcFile, destFile, type});
   };
 }