From 77afd334e47c3504b0e7b43e5d3e9c9d7560b13c Mon Sep 17 00:00:00 2001 From: Juan Carlos Farah Date: Thu, 8 Aug 2019 18:48:58 +0200 Subject: [PATCH] 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 | 8 +++ package.json | 8 ++- public/app/config/config.js | 4 +- public/app/listeners/loadSpace.js | 114 ++++++++++++++++-------------- public/app/utilities.js | 48 +++++++++++++ public/app/utils/isMac.js | 3 + public/app/utils/isWindows.js | 3 + public/electron.js | 18 +++-- 8 files changed, 142 insertions(+), 64 deletions(-) create mode 100644 public/app/utils/isMac.js create mode 100644 public/app/utils/isWindows.js diff --git a/README.md b/README.md index 4d5850dd..7a9a8b56 100644 --- a/README.md +++ b/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//log.log` +- macOS: `~/Library/Logs//log.log` +- Windows: `%USERPROFILE%\AppData\Roaming\\log.log` diff --git a/package.json b/package.json index d7044cf6..b2cd3eaa 100644 --- a/package.json +++ b/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", diff --git a/public/app/config/config.js b/public/app/config/config.js index 37dd92ef..9bd1ac9d 100644 --- a/public/app/config/config.js +++ b/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 diff --git a/public/app/listeners/loadSpace.js b/public/app/listeners/loadSpace.js index 6d99f87f..30d20dd0 100644 --- a/public/app/listeners/loadSpace.js +++ b/public/app/listeners/loadSpace.js @@ -1,7 +1,8 @@ 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, @@ -9,73 +10,80 @@ const { 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); } }; diff --git a/public/app/utilities.js b/public/app/utilities.js index 40ac2e42..5a72cef2 100644 --- a/public/app/utilities.js +++ b/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, diff --git a/public/app/utils/isMac.js b/public/app/utils/isMac.js new file mode 100644 index 00000000..8320f8d9 --- /dev/null +++ b/public/app/utils/isMac.js @@ -0,0 +1,3 @@ +const isMac = () => process.platform === 'darwin'; + +module.exports = isMac; diff --git a/public/app/utils/isWindows.js b/public/app/utils/isWindows.js new file mode 100644 index 00000000..0d952c8f --- /dev/null +++ b/public/app/utils/isWindows.js @@ -0,0 +1,3 @@ +const isWindows = () => process.platform === 'win32'; + +module.exports = isWindows; diff --git a/public/electron.js b/public/electron.js index 5cc2c693..5053b83d 100644 --- a/public/electron.js +++ b/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 () => {