fix: fix windows exceptions on loading and deleting spaces

Loading a space or trying to remove it if the process fails was causing
an EPERM or an ENOTEMPTY exception due to issues with Windows and how
it does not release file handles properly. We are now handling this
using wrappers around the functions so that they are tried multiple
times or wait until a file access is finished.

closes #159

README.md

@@ -109,3 +109,11 @@ see the issue for details on the typos fixed
 
 fixes #12
 ```
+
+## Logs
+
+Following the `electron-log` defaults, logs are written to the following locations:
+
+- Linux: `~/.config/<app name>/log.log`
+- macOS: `~/Library/Logs/<app name>/log.log`
+- Windows: `%USERPROFILE%\AppData\Roaming\<app name>\log.log`

package.json

@@ -28,20 +28,22 @@
     "build": "env-cmd -f ./.env react-scripts build",
     "start": "concurrently \"env-cmd -f ./.env.local react-scripts start\" \"wait-on http://localhost:3000 && env-cmd -f ./.env.local electron .\"",
     "pack": "electron-builder --dir",
-    "postinstall": "electron-builder install-app-deps",
     "prestart": "env-cmd -f ./.env.local node scripts/setup.js",
     "prebuild": "env-cmd -f ./.env node scripts/setup.js",
-    "setup": "node scripts/setup.js",
     "predist": "yarn build",
+    "predist:windows": "yarn build",
+    "prerelease": "yarn test:once && yarn build",
+    "setup": "node scripts/setup.js",
     "lint": "eslint .",
     "prettier:check": "prettier --check '{src,public}/**/*.js'",
     "prettier:write": "prettier --write '{src,public}/**/*.js'",
     "test": "react-scripts test",
     "dist": "env-cmd -f ./.env electron-builder",
-    "prerelease": "yarn test:once && yarn build",
+    "dist:windows": "env-cmd -f ./.env build -w",
     "release": "git add CHANGELOG.md && standard-version -a && env-cmd -f ./.env build -mwl",
     "hooks:uninstall": "node node_modules/husky/husky.js uninstall",
     "hooks:install": "node node_modules/husky/husky.js install",
+    "postinstall": "electron-builder install-app-deps",
     "postrelease": "git push --follow-tags origin master",
     "test:once": "cross-env CI=true env-cmd -f ./.env.test react-scripts test --env=jsdom",
     "test:coverage": "cross-env CI=true env-cmd -f ./.env.test react-scripts test --env=jsdom --coverage",

public/app/config/config.js

@@ -1,6 +1,6 @@
 // eslint-disable-next-line import/no-extraneous-dependencies
 const { app } = require('electron');
-const process = require('process');
+const isWindows = require('../utils/isWindows');
 
 // types that we support downloading
 const DOWNLOADABLE_MIME_TYPES = [
@@ -29,7 +29,7 @@ const DOWNLOADABLE_MIME_TYPES = [
 
 // resolve path for windows '\'
 const escapeEscapeCharacter = str => {
-  return process.platform === 'win32' ? str.replace(/\\/g, '\\\\') : str;
+  return isWindows() ? str.replace(/\\/g, '\\\\') : str;
 };
 
 // categories

public/app/listeners/loadSpace.js

@@ -1,81 +1,89 @@
 const extract = require('extract-zip');
-const rimraf = require('rimraf');
+const { promisify } = require('util');
 const fs = require('fs');
-const { VAR_FOLDER, TMP_FOLDER } = require('../config/config');
+const ObjectId = require('bson-objectid');
+const { VAR_FOLDER } = require('../config/config');
 const { LOADED_SPACE_CHANNEL } = require('../config/channels');
 const {
   ERROR_SPACE_ALREADY_AVAILABLE,
   ERROR_GENERAL,
   ERROR_ZIP_CORRUPTED,
 } = require('../config/errors');
 const logger = require('../logger');
-const { isFileAvailable } = require('../utilities');
+const {
+  performFileSystemOperation,
+  isFileAvailable,
+  clean,
+} = require('../utilities');
 const { SPACES_COLLECTION } = require('../db');
 
 // use promisified fs
 const fsPromises = fs.promises;
 
 const loadSpace = (mainWindow, db) => async (event, { fileLocation }) => {
-  const extractPath = `${VAR_FOLDER}/${TMP_FOLDER}`;
+  const tmpId = ObjectId().str;
+
+  // make temporary folder hidden
+  const extractPath = `${VAR_FOLDER}/.${tmpId}`;
   try {
-    extract(fileLocation, { dir: extractPath }, async extractError => {
-      if (extractError) {
-        logger.error(extractError);
-        return mainWindow.webContents.send(LOADED_SPACE_CHANNEL, ERROR_GENERAL);
-      }
-      // get basic information from manifest
-      const manifestPath = `${extractPath}/manifest.json`;
-      // abort if there is no manifest
-      const hasManifest = await isFileAvailable(manifestPath);
-      if (!hasManifest) {
-        rimraf.sync(extractPath);
-        return mainWindow.webContents.send(
-          LOADED_SPACE_CHANNEL,
-          ERROR_ZIP_CORRUPTED
-        );
-      }
-      const manifestString = await fsPromises.readFile(manifestPath);
-      const manifest = JSON.parse(manifestString);
-      const { id } = manifest;
-      const spacePath = `${extractPath}/${id}.json`;
+    await promisify(extract)(fileLocation, { dir: extractPath });
+
+    // get basic information from manifest
+    const manifestPath = `${extractPath}/manifest.json`;
+    // abort if there is no manifest
+    const hasManifest = await isFileAvailable(manifestPath);
+    if (!hasManifest) {
+      mainWindow.webContents.send(LOADED_SPACE_CHANNEL, ERROR_ZIP_CORRUPTED);
+      return clean(extractPath);
+    }
+    const manifestString = await fsPromises.readFile(manifestPath);
+    const manifest = JSON.parse(manifestString);
+    const { id } = manifest;
+    const spacePath = `${extractPath}/${id}.json`;
+
+    // get handle to spaces collection
+    const spaces = db.get(SPACES_COLLECTION);
+    const existingSpace = spaces.find({ id }).value();
+
+    // abort if there is already a space with that id
+    if (existingSpace) {
+      mainWindow.webContents.send(
+        LOADED_SPACE_CHANNEL,
+        ERROR_SPACE_ALREADY_AVAILABLE
+      );
+      return clean(extractPath);
+    }
+
+    // abort if there is no space
+    const hasSpace = await isFileAvailable(spacePath);
+    if (!hasSpace) {
+      mainWindow.webContents.send(LOADED_SPACE_CHANNEL, ERROR_ZIP_CORRUPTED);
+      return clean(extractPath);
+    }
 
-      // get handle to spaces collection
-      const spaces = db.get(SPACES_COLLECTION);
-      const existingSpace = spaces.find({ id }).value();
+    const spaceString = await fsPromises.readFile(spacePath);
+    const space = JSON.parse(spaceString);
+    const finalPath = `${VAR_FOLDER}/${id}`;
 
-      // abort if there is already a space with that id
-      if (existingSpace) {
-        rimraf.sync(extractPath);
-        return mainWindow.webContents.send(
-          LOADED_SPACE_CHANNEL,
-          ERROR_SPACE_ALREADY_AVAILABLE
-        );
-      }
+    // we need to wrap this operation to avoid errors in windows
+    performFileSystemOperation(fs.renameSync)(extractPath, finalPath);
 
-      // abort if there is no space
-      const hasSpace = await isFileAvailable(spacePath);
-      if (!hasSpace) {
-        rimraf.sync(extractPath);
-        return mainWindow.webContents.send(
-          LOADED_SPACE_CHANNEL,
-          ERROR_ZIP_CORRUPTED
-        );
-      }
+    const wasRenamed = await isFileAvailable(finalPath);
 
-      const spaceString = await fsPromises.readFile(spacePath);
-      const space = JSON.parse(spaceString);
-      const finalPath = `${VAR_FOLDER}/${id}`;
-      await fsPromises.rename(extractPath, finalPath);
+    if (!wasRenamed) {
+      logger.error('unable to rename extract path');
+      mainWindow.webContents.send(LOADED_SPACE_CHANNEL, ERROR_GENERAL);
+      return clean(extractPath);
+    }
 
-      // write to database
-      spaces.push(space).write();
+    // write to database
+    spaces.push(space).write();
 
-      return mainWindow.webContents.send(LOADED_SPACE_CHANNEL);
-    });
+    return mainWindow.webContents.send(LOADED_SPACE_CHANNEL);
   } catch (err) {
     logger.error(err);
     mainWindow.webContents.send(LOADED_SPACE_CHANNEL, ERROR_GENERAL);
-    rimraf.sync(extractPath);
+    return clean(extractPath);
   }
 };
 

public/app/utilities.js

@@ -1,8 +1,11 @@
 const mkdirp = require('mkdirp');
+const rimraf = require('rimraf');
+const { promisify } = require('util');
 const mime = require('mime-types');
 const md5 = require('md5');
 const fs = require('fs');
 const logger = require('./logger');
+const isWindows = require('./utils/isWindows');
 const {
   DOWNLOADABLE_MIME_TYPES,
   APPLICATION,
@@ -11,6 +14,9 @@ const {
   TMP_FOLDER,
 } = require('./config/config');
 
+// use promisified fs
+const fsPromises = fs.promises;
+
 const isFileAvailable = filePath =>
   new Promise(resolve =>
     fs.access(filePath, fs.constants.F_OK, err => resolve(!err))
@@ -72,7 +78,49 @@ const createSpaceDirectory = ({ id, tmp }) => {
   }
 };
 
+// wraps file system operation so that it can be retried
+// many times for windows operating systems
+const performFileSystemOperation = functionToWrap => (...args) => {
+  let tryOperation = true;
+  // windows operations require silly amounts of retries
+  // because it will not release handles promptly (#159)
+  const retries = isWindows() ? 100 : 1;
+  let i = 0;
+  do {
+    let threw = true;
+    try {
+      functionToWrap(...args);
+      threw = false;
+    } finally {
+      i += 1;
+      if (i >= retries || !threw) {
+        tryOperation = false;
+        // eslint-disable-next-line no-unsafe-finally
+        break;
+      } else {
+        logger.error(`failed operation ${functionToWrap.name} on try ${i}`);
+        // eslint-disable-next-line no-continue, no-unsafe-finally
+        continue;
+      }
+    }
+  } while (tryOperation);
+};
+
+// waits until directory is accessed before removing it
+// and thus allowing windows to release the file handle
+const clean = async dir => {
+  try {
+    await fsPromises.access(dir);
+  } catch (err) {
+    // does not exist so all good
+    return true;
+  }
+  return promisify(rimraf)(dir);
+};
+
 module.exports = {
+  clean,
+  performFileSystemOperation,
   getExtension,
   isDownloadable,
   generateHash,

public/app/utils/isMac.js

@@ -0,0 +1,3 @@
+const isMac = () => process.platform === 'darwin';
+
+module.exports = isMac;

public/app/utils/isWindows.js

@@ -0,0 +1,3 @@
+const isWindows = () => process.platform === 'win32';
+
+module.exports = isWindows;

public/electron.js

@@ -69,6 +69,7 @@ const {
   getGeolocationEnabled,
   setGeolocationEnabled,
 } = require('./app/listeners');
+const isMac = require('./app/utils/isMac');
 
 // add keys to process
 Object.keys(env).forEach(key => {
@@ -194,12 +195,11 @@ const learnMoreLink =
 const fileIssueLink = 'https://github.com/react-epfl/graasp-desktop/issues';
 
 const generateMenu = () => {
-  const isMac = process.platform === 'darwin';
   const template = [
-    ...(isMac ? macAppMenu : standardAppMenu),
+    ...(isMac() ? macAppMenu : standardAppMenu),
     {
       label: 'File',
-      submenu: [...(isMac ? macFileSubmenu : standardFileSubmenu)],
+      submenu: [...(isMac() ? macFileSubmenu : standardFileSubmenu)],
     },
     { type: 'separator' },
     {
@@ -233,7 +233,7 @@ const generateMenu = () => {
       submenu: [
         { role: 'minimize' },
         { role: 'zoom' },
-        ...(isMac
+        ...(isMac()
           ? [
               { type: 'separator' },
               { role: 'front' },
@@ -264,8 +264,14 @@ const generateMenu = () => {
     },
   ];
 
-  Menu.setApplicationMenu(null);
-  mainWindow.setMenu(Menu.buildFromTemplate(template));
+  if (isMac()) {
+    Menu.setApplicationMenu(Menu.buildFromTemplate(template));
+  } else {
+    // this causes the menu to change on mac after first use
+    // and it's no longer possible to use the mac defaults
+    Menu.setApplicationMenu(null);
+    mainWindow.setMenu(Menu.buildFromTemplate(template));
+  }
 };
 
 app.on('ready', async () => {