From 141e440b94c031326e6862111b69e82c2c490c1d Mon Sep 17 00:00:00 2001 From: MICHAEL JACKSON Date: Wed, 21 Jun 2017 11:22:35 +1000 Subject: [PATCH] Revert "Fix extraction race condition" This reverts commit 4aba460a6309ece485603bab3017742b1443e5b7. --- server/middleware/RegistryUtils.js | 135 ++++++++++++----------------- server/middleware/index.js | 11 ++- 2 files changed, 61 insertions(+), 85 deletions(-) diff --git a/server/middleware/RegistryUtils.js b/server/middleware/RegistryUtils.js index e422d95..a5aa752 100644 --- a/server/middleware/RegistryUtils.js +++ b/server/middleware/RegistryUtils.js @@ -4,7 +4,7 @@ const mkdirp = require('mkdirp') const tar = require('tar-fs') const RegistryCache = require('./RegistryCache') -const fetchPackageInfoFromRegistry = (registryURL, packageName) => { +const getPackageInfoFromRegistry = (registryURL, packageName) => { let encodedPackageName if (packageName.charAt(0) === '@') { encodedPackageName = `@${encodeURIComponent(packageName.substring(1))}` @@ -23,35 +23,34 @@ const fetchPackageInfoFromRegistry = (registryURL, packageName) => { const PackageNotFound = 'PackageNotFound' -const fetchPackageInfo = (registryURL, packageName) => - new Promise((resolve, reject) => { - RegistryCache.get(packageName, (error, value) => { - if (error) { - reject(error) - } else if (value) { - resolve(value === PackageNotFound ? null : value) - } else { - fetchPackageInfoFromRegistry(registryURL, packageName).then(value => { - if (value == null) { - // Keep 404s in the cache for 5 minutes. This prevents us - // from making unnecessary requests to the registry for - // bad package names. In the worst case, a brand new - // package's info will be available within 5 minutes. - RegistryCache.set(packageName, PackageNotFound, 300) - } else { - // Keep package.json in the cache for a minute. - RegistryCache.set(packageName, value, 60) - } +const getPackageInfo = (registryURL, packageName, callback) => { + RegistryCache.get(packageName, (error, value) => { + if (error) { + callback(error) + } else if (value) { + callback(null, value === PackageNotFound ? null : value) + } else { + getPackageInfoFromRegistry(registryURL, packageName).then(value => { + if (value == null) { + // Keep 404s in the cache for 5 minutes. This prevents us + // from making unnecessary requests to the registry for + // bad package names. In the worst case, a brand new + // package's info will be available within 5 minutes. + RegistryCache.set(packageName, PackageNotFound, 300) + } else { + // Keep package.json in the cache for a minute. + RegistryCache.set(packageName, value, 60) + } - resolve(value) - }, error => { - // Do not cache errors. - RegistryCache.del(packageName) - reject(error) - }) - } - }) + callback(null, value) + }, error => { + // Do not cache errors. + RegistryCache.del(packageName) + callback(error) + }) + } }) +} const normalizeTarHeader = (header) => { // Most packages have header names that look like "package/index.js" @@ -62,60 +61,34 @@ const normalizeTarHeader = (header) => { return header } -const fetchAndExtractPackage = (tarballURL, outputDir) => - new Promise((resolve, reject) => { - mkdirp(outputDir, (error) => { - if (error) { - reject(error) - } else { - fetch(tarballURL).then(response => { - response.body - .pipe(gunzip()) - .pipe( - tar.extract(outputDir, { - dmode: 0o666, // All dirs should be writable - fmode: 0o444, // All files should be readable - map: normalizeTarHeader - }) - ) - .on('finish', resolve) - .on('error', reject) - }, reject) - } - }) - }) - -const runCache = {} - -// A helper that prevents running multiple async operations -// identified by the same key concurrently. Instead, the operation -// is performed only once the first time it is requested and all -// subsequent calls get that same result until it is completed. -const runOnce = (key, perform) => { - let promise = runCache[key] - - if (!promise) { - promise = runCache[key] = perform() - - // Clear the cache when we're done. - promise.then(() => { - delete runCache[key] - }, () => { - delete runCache[key] - }) - } - - return promise -} - -const getPackageInfo = (registryURL, packageName, callback) => { - runOnce(registryURL + packageName, () => fetchPackageInfo(registryURL, packageName)) - .then(info => callback(null, info), callback) -} - const getPackage = (tarballURL, outputDir, callback) => { - runOnce(tarballURL + outputDir, () => fetchAndExtractPackage(tarballURL, outputDir)) - .then(() => callback(null), callback) + mkdirp(outputDir, (error) => { + if (error) { + callback(error) + } else { + let callbackWasCalled = false + + fetch(tarballURL).then(response => { + response.body + .pipe(gunzip()) + .pipe( + tar.extract(outputDir, { + dmode: 0o666, // All dirs should be writable + fmode: 0o444, // All files should be readable + map: normalizeTarHeader + }) + ) + .on('finish', callback) + .on('error', (error) => { + if (callbackWasCalled) // LOL node streams + return + + callbackWasCalled = true + callback(error) + }) + }) + } + }) } module.exports = { diff --git a/server/middleware/index.js b/server/middleware/index.js index b62ac09..381de74 100644 --- a/server/middleware/index.js +++ b/server/middleware/index.js @@ -22,11 +22,14 @@ const oneMinute = 60 const oneDay = oneMinute * 60 * 24 const oneYear = oneDay * 365 -const checkLocalCache = (dir, filename, callback) => - statFile(joinPaths(dir, filename || 'package.json'), (error, stats) => { +const checkLocalCache = (dir, callback) => + statFile(joinPaths(dir, 'package.json'), (error, stats) => { callback(stats && stats.isFile()) }) +const createTempPath = (name) => + joinPaths(tmpdir(), `unpkg-${name}`) + const ResolveExtensions = [ '', '.js', '.json' ] /** @@ -107,9 +110,9 @@ const createRequestHandler = (options = {}) => { // Step 1: Fetch the package from the registry and store a local copy. // Redirect if the URL does not specify an exact version number. const fetchPackage = (next) => { - const packageDir = joinPaths(tmpdir(), `unpkg-${displayName}`) + const packageDir = createTempPath(displayName) - checkLocalCache(packageDir, filename, (isCached) => { + checkLocalCache(packageDir, (isCached) => { if (isCached) return next(packageDir) // Best case: we already have this package on disk.