From 0f895bf04e5187f121a0d3aa06ee2e4d0a54204b Mon Sep 17 00:00:00 2001 From: Michael Jackson Date: Tue, 22 May 2018 22:06:55 -0400 Subject: [PATCH] Use lockfile to prevent inter-process races See #86 --- package.json | 1 + server/utils/fetchPackage.js | 38 ++------------------- server/utils/getPackage.js | 65 ++++++++++++++++++++++++++++++++---- yarn.lock | 11 ++++++ 4 files changed, 73 insertions(+), 42 deletions(-) diff --git a/package.json b/package.json index a230d6c..cc14828 100644 --- a/package.json +++ b/package.json @@ -31,6 +31,7 @@ "os-tmpdir": "^1.0.2", "pretty-bytes": "^3", "prop-types": "^15.5.8", + "proper-lockfile": "^3.0.2", "react": "^15.5.4", "react-dom": "^15.5.4", "react-motion": "^0.4.7", diff --git a/server/utils/fetchPackage.js b/server/utils/fetchPackage.js index ee89acd..5d240bc 100644 --- a/server/utils/fetchPackage.js +++ b/server/utils/fetchPackage.js @@ -1,11 +1,7 @@ require("isomorphic-fetch"); -const fs = require("fs"); -const mkdirp = require("mkdirp"); const gunzip = require("gunzip-maybe"); const tar = require("tar-fs"); -const createTempPath = require("./createTempPath"); - function stripNamePrefix(headers) { // Most packages have header names that look like "package/index.js" // so we shorten that to just "index.js" here. A few packages use a @@ -35,38 +31,10 @@ function extractResponse(response, outputDir) { }); } -function fetchPackage(packageConfig) { - return new Promise((resolve, reject) => { - const tarballURL = packageConfig.dist.tarball; - const outputDir = createTempPath(packageConfig.name, packageConfig.version); +function fetchPackage(tarballURL, outputDir) { + console.log(`info: Fetching ${tarballURL} and extracting to ${outputDir}`); - console.log(`info: Fetching ${tarballURL} and extracting to ${outputDir}`); - - fs.access(outputDir, error => { - if (error) { - if (error.code === "ENOENT" || error.code === "ENOTDIR") { - // ENOENT or ENOTDIR are to be expected when we haven't yet - // fetched a package for the first time. Carry on! - mkdirp(outputDir, error => { - if (error) { - reject(error); - } else { - resolve( - fetch(tarballURL) - .then(res => extractResponse(res, outputDir)) - .then(() => outputDir) - ); - } - }); - } else { - reject(error); - } - } else { - // Best case: we already have this package cached on disk! - resolve(outputDir); - } - }); - }); + return fetch(tarballURL).then(res => extractResponse(res, outputDir)); } module.exports = fetchPackage; diff --git a/server/utils/getPackage.js b/server/utils/getPackage.js index 46f7b21..4d60054 100644 --- a/server/utils/getPackage.js +++ b/server/utils/getPackage.js @@ -1,15 +1,66 @@ +const fs = require("fs"); +const mkdirp = require("mkdirp"); +const lockfile = require("proper-lockfile"); + const createMutex = require("./createMutex"); +const createTempPath = require("./createTempPath"); const fetchPackage = require("./fetchPackage"); const fetchMutex = createMutex((packageConfig, callback) => { - fetchPackage(packageConfig).then( - outputDir => { - callback(null, outputDir); - }, - error => { - callback(error); + const tarballURL = packageConfig.dist.tarball; + const outputDir = createTempPath(packageConfig.name, packageConfig.version); + + fs.access(outputDir, error => { + if (error) { + if (error.code === "ENOENT" || error.code === "ENOTDIR") { + // ENOENT or ENOTDIR are to be expected when we haven't yet + // fetched a package for the first time. Carry on! + mkdirp.sync(outputDir); + const release = lockfile.lockSync(outputDir); + + fetchPackage(tarballURL, outputDir).then( + () => { + release(); + callback(null, outputDir); + }, + error => { + release(); + callback(error); + } + ); + } else { + callback(error); + } + } else { + lockfile.check(outputDir).then(isLocked => { + if (isLocked) { + // Another process on this same machine has locked the + // directory. We need to wait for it to be unlocked + // before we callback. + const timer = setInterval(() => { + lockfile.check(outputDir).then( + isLocked => { + if (!isLocked) { + clearInterval(timer); + callback(null, outputDir); + } + }, + error => { + clearInterval(timer); + callback(error); + } + ); + }, 10); + + timer.unref(); + } else { + // Best case: we already have this package cached on disk + // and it's not locked! + callback(null, outputDir); + } + }, callback); } - ); + }); }, packageConfig => packageConfig.dist.tarball); function getPackage(packageConfig) { diff --git a/yarn.lock b/yarn.lock index f0481b2..794eb27 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5560,6 +5560,13 @@ prop-types@^15.5.10, prop-types@^15.5.4, prop-types@^15.5.8, prop-types@^15.6.0: loose-envify "^1.3.1" object-assign "^4.1.1" +proper-lockfile@^3.0.2: + version "3.0.2" + resolved "https://registry.yarnpkg.com/proper-lockfile/-/proper-lockfile-3.0.2.tgz#d30b3b83ecb157e08fe0d411f2393bc384b77ad1" + dependencies: + graceful-fs "^4.1.11" + retry "^0.10.1" + proxy-addr@~2.0.2: version "2.0.2" resolved "https://registry.yarnpkg.com/proxy-addr/-/proxy-addr-2.0.2.tgz#6571504f47bb988ec8180253f85dd7e14952bdec" @@ -6068,6 +6075,10 @@ ret@~0.1.10: version "0.1.15" resolved "https://registry.yarnpkg.com/ret/-/ret-0.1.15.tgz#b8a4825d5bdb1fc3f6f53c2bc33f81388681c7bc" +retry@^0.10.1: + version "0.10.1" + resolved "https://registry.yarnpkg.com/retry/-/retry-0.10.1.tgz#e76388d217992c252750241d3d3956fed98d8ff4" + right-align@^0.1.1: version "0.1.3" resolved "https://registry.yarnpkg.com/right-align/-/right-align-0.1.3.tgz#61339b722fe6a3515689210d24e14c96148613ef"