Fix extraction race condition

This is a fix for a subtle bug that caused unpkg to incorrectly send a
404 response for valid filenames on the initial request for a package.
It also prevents the same package from being downloaded from the
registry by multiple concurrent requests.

Fixes #38
Closes #39

server/middleware/RegistryUtils.js

@@ -4,7 +4,7 @@ const mkdirp = require('mkdirp')
 const tar = require('tar-fs')
 const RegistryCache = require('./RegistryCache')
 
-const getPackageInfoFromRegistry = (registryURL, packageName) => {
+const fetchPackageInfoFromRegistry = (registryURL, packageName) => {
   let encodedPackageName
   if (packageName.charAt(0) === '@') {
     encodedPackageName = `@${encodeURIComponent(packageName.substring(1))}`
@@ -23,34 +23,35 @@ const getPackageInfoFromRegistry = (registryURL, packageName) => {
 
 const PackageNotFound = 'PackageNotFound'
 
-const getPackageInfo = (registryURL, packageName, callback) => {
+const fetchPackageInfo = (registryURL, packageName) =>
-  RegistryCache.get(packageName, (error, value) => {
+  new Promise((resolve, reject) => {
-    if (error) {
+    RegistryCache.get(packageName, (error, value) => {
-      callback(error)
+      if (error) {
-    } else if (value) {
+        reject(error)
-      callback(null, value === PackageNotFound ? null : value)
+      } else if (value) {
-    } else {
+        resolve(value === PackageNotFound ? null : value)
-      getPackageInfoFromRegistry(registryURL, packageName).then(value => {
+      } else {
-        if (value == null) {
+        fetchPackageInfoFromRegistry(registryURL, packageName).then(value => {
-          // Keep 404s in the cache for 5 minutes. This prevents us
+          if (value == null) {
-          // from making unnecessary requests to the registry for
+            // Keep 404s in the cache for 5 minutes. This prevents us
-          // bad package names. In the worst case, a brand new
+            // from making unnecessary requests to the registry for
-          // package's info will be available within 5 minutes.
+            // bad package names. In the worst case, a brand new
-          RegistryCache.set(packageName, PackageNotFound, 300)
+            // package's info will be available within 5 minutes.
-        } else {
+            RegistryCache.set(packageName, PackageNotFound, 300)
-          // Keep package.json in the cache for a minute.
+          } else {
-          RegistryCache.set(packageName, value, 60)
+            // Keep package.json in the cache for a minute.
-        }
+            RegistryCache.set(packageName, value, 60)
+          }
 
-        callback(null, value)
+          resolve(value)
-      }, error => {
+        }, error => {
-        // Do not cache errors.
+          // Do not cache errors.
-        RegistryCache.del(packageName)
+          RegistryCache.del(packageName)
-        callback(error)
+          reject(error)
-      })
+        })
-    }
+      }
+    })
   })
-}
 
 const normalizeTarHeader = (header) => {
   // Most packages have header names that look like "package/index.js"
@@ -61,34 +62,60 @@ const normalizeTarHeader = (header) => {
   return header
 }
 
-const getPackage = (tarballURL, outputDir, callback) => {
+const fetchAndExtractPackage = (tarballURL, outputDir) =>
-  mkdirp(outputDir, (error) => {
+  new Promise((resolve, reject) => {
-    if (error) {
+    mkdirp(outputDir, (error) => {
-      callback(error)
+      if (error) {
-    } else {
+        reject(error)
-      let callbackWasCalled = false
+      } else {
-
+        fetch(tarballURL).then(response => {
-      fetch(tarballURL).then(response => {
+          response.body
-        response.body
+            .pipe(gunzip())
-          .pipe(gunzip())
+            .pipe(
-          .pipe(
+              tar.extract(outputDir, {
-            tar.extract(outputDir, {
+                dmode: 0o666, // All dirs should be writable
-              dmode: 0o666, // All dirs should be writable
+                fmode: 0o444, // All files should be readable
-              fmode: 0o444, // All files should be readable
+                map: normalizeTarHeader
-              map: normalizeTarHeader
+              })
-            })
+            )
-          )
+            .on('finish', resolve)
-          .on('finish', callback)
+            .on('error', reject)
-          .on('error', (error) => {
+        }, reject)
-            if (callbackWasCalled) // LOL node streams
+      }
-              return
+    })
-
-            callbackWasCalled = true
-            callback(error)
-          })
-      })
-    }
   })
+
+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)
 }
 
 module.exports = {

server/middleware/index.js

@@ -22,14 +22,11 @@ const oneMinute = 60
 const oneDay = oneMinute * 60 * 24
 const oneYear = oneDay * 365
 
-const checkLocalCache = (dir, callback) =>
+const checkLocalCache = (dir, filename, callback) =>
-  statFile(joinPaths(dir, 'package.json'), (error, stats) => {
+  statFile(joinPaths(dir, filename || 'package.json'), (error, stats) => {
     callback(stats && stats.isFile())
   })
 
-const createTempPath = (name) =>
-  joinPaths(tmpdir(), `unpkg-${name}`)
-
 const ResolveExtensions = [ '', '.js', '.json' ]
 
 /**
@@ -110,9 +107,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 = createTempPath(displayName)
+      const packageDir = joinPaths(tmpdir(), `unpkg-${displayName}`)
 
-      checkLocalCache(packageDir, (isCached) => {
+      checkLocalCache(packageDir, filename, (isCached) => {
         if (isCached)
           return next(packageDir) // Best case: we already have this package on disk.