Prevent multiple concurrent requests for packages

Fixes #38
Fixes #41

server/PackageCache.js

@@ -0,0 +1,85 @@
+require('isomorphic-fetch')
+const fs = require('fs')
+const path = require('path')
+const tmpdir = require('os-tmpdir')
+const gunzip = require('gunzip-maybe')
+const mkdirp = require('mkdirp')
+const tar = require('tar-fs')
+const createMutex = require('./createMutex')
+
+function createTempPath(name, version) {
+  return path.join(tmpdir(), `unpkg-${name}-${version}`)
+}
+
+function normalizeTarHeader(header) {
+  // 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
+  // prefix other than "package/". e.g. the firebase package uses the
+  // "firebase_npm/" prefix. So we just strip the first dir name.
+  header.name = header.name.replace(/^[^\/]+\//, '')
+  return header
+}
+
+function extractResponse(response, outputDir) {
+  return new Promise(function (resolve, reject) {
+    const extract = tar.extract(outputDir, {
+      dmode: 0o666, // All dirs should be writable
+      fmode: 0o444, // All files should be readable
+      map: normalizeTarHeader
+    })
+
+    response.body
+      .pipe(gunzip())
+      .pipe(extract)
+      .on('finish', resolve)
+      .on('error', reject)
+  })
+}
+
+function fetchAndExtract(tarballURL, outputDir) {
+  console.log(`Fetching ${tarballURL} and extracting to ${outputDir}`)
+
+  return fetch(tarballURL).then(function (response) {
+    return extractResponse(response, outputDir)
+  })
+}
+
+const fetchMutex = createMutex(function (payload, callback) {
+  const { tarballURL, outputDir } = payload
+
+  fs.access(outputDir, function (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, function (error) {
+          if (error) {
+            callback(error)
+          } else {
+            fetchAndExtract(tarballURL, outputDir).then(function () {
+              callback()
+            }, callback)
+          }
+        })
+      } else {
+        callback(error)
+      }
+    } else {
+      // Best case: we already have this package cached on disk!
+      callback()
+    }
+  })
+})
+
+function getPackage(packageConfig, callback) {
+  const tarballURL = packageConfig.dist.tarball
+  const outputDir = createTempPath(packageConfig.name, packageConfig.version)
+
+  fetchMutex(tarballURL, { tarballURL, outputDir }, function (error) {
+    callback(error, outputDir)
+  })
+}
+
+module.exports = {
+  get: getPackage
+}

server/PackageInfo.js

@@ -58,7 +58,7 @@ function getPackageInfo(packageName, callback) {
     } else if (value) {
       callback(null, value === PackageNotFound ? null : value)
     } else {
-      fetchMutex(packageName, callback)
+      fetchMutex(packageName, packageName, callback)
     }
   })
 }

server/createMutex.js

@@ -1,7 +1,7 @@
 function createMutex(doWork) {
   const mutex = {}
 
-  return function (key, callback) {
+  return function (key, payload, callback) {
     if (mutex[key]) {
       mutex[key].push(callback)
     } else {
@@ -9,7 +9,7 @@ function createMutex(doWork) {
         delete mutex[key]
       }, callback ]
 
-      doWork(key, function (error, value) {
+      doWork(payload, function (error, value) {
         mutex[key].forEach(function (callback) {
           callback(error, value)
         })

server/middleware/RegistryUtils.js

@@ -1,47 +0,0 @@
-require('isomorphic-fetch')
-const gunzip = require('gunzip-maybe')
-const mkdirp = require('mkdirp')
-const tar = require('tar-fs')
-
-function normalizeTarHeader(header) {
-  // 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
-  // prefix other than "package/". e.g. the firebase package uses the
-  // "firebase_npm/" prefix. So we just strip the first dir name.
-  header.name = header.name.replace(/^[^\/]+\//, '')
-  return header
-}
-
-function getPackage(tarballURL, outputDir, 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 = {
-  getPackage
-}

server/middleware/fetchPackage.js

@@ -1,34 +1,13 @@
-const fs = require('fs')
-const path = require('path')
-const tmpdir = require('os-tmpdir')
 const { maxSatisfying: maxSatisfyingVersion } = require('semver')
+const PackageCache = require('../PackageCache')
 const PackageInfo = require('../PackageInfo')
 const { createPackageURL } = require('./PackageUtils')
-const { getPackage } = require('./RegistryUtils')
-
-function checkLocalCache(dir, callback) {
-  fs.stat(path.join(dir, 'package.json'), function (error, stats) {
-    callback(stats && stats.isFile())
-  })
-}
-
-function createTempPath(name) {
-  return path.join(tmpdir(), `unpkg-${name}`)
-}
 
 /**
  * Fetch the package from the registry and store a local copy on disk.
  * Redirect if the URL does not specify an exact req.packageVersion number.
  */
-function fetchPackage() {
-  return function (req, res, next) {
-    req.packageDir = createTempPath(req.packageSpec)
-
-    // TODO: fix race condition! (see #38)
-    checkLocalCache(req.packageDir, function (isCached) {
-      if (isCached)
-        return next() // Best case: we already have this package on disk.
-
+function fetchPackage(req, res, next) {
   PackageInfo.get(req.packageName, function (error, packageInfo) {
     if (error) {
       console.error(error)
@@ -38,17 +17,20 @@ function fetchPackage() {
     if (packageInfo == null || packageInfo.versions == null)
       return res.status(404).send(`Cannot find package "${req.packageName}"`)
 
-        const { versions, 'dist-tags': tags } = packageInfo
+    req.packageInfo = packageInfo
+
+    const { versions, 'dist-tags': tags } = req.packageInfo
 
     if (req.packageVersion in versions) {
       // A valid request for a package we haven't downloaded yet.
-          const packageConfig = versions[req.packageVersion]
-          const tarballURL = packageConfig.dist.tarball
+      req.packageConfig = versions[req.packageVersion]
 
-          getPackage(tarballURL, req.packageDir, function (error) {
+      PackageCache.get(req.packageConfig, function (error, outputDir) {
         if (error) {
-              res.status(500).send(error.message || error)
+          console.error(error)
+          res.status(500).send(`Cannot fetch package ${req.packageSpec}`)
         } else {
+          req.packageDir = outputDir
           next()
         }
       })
@@ -64,8 +46,6 @@ function fetchPackage() {
       }
     }
   })
-    })
-  }
 }
 
 module.exports = fetchPackage

server/middleware/findFile.js

@@ -41,8 +41,7 @@ function resolveFile(base, useIndex, callback) {
  * Determine which file we're going to serve and get its stats.
  * Redirect if the request targets a directory with no trailing slash.
  */
-function findFile() {
-  return function (req, res, next) {
+function findFile(req, res, next) {
   if (req.filename) {
     const base = path.join(req.packageDir, req.filename)
 
@@ -114,7 +113,6 @@ function findFile() {
       })
     })
   }
-  }
 }
 
 module.exports = findFile

server/middleware/index.js

@@ -33,10 +33,10 @@ function createRequestHandler(options = {}) {
   const app = express.Router()
 
   app.use(
-    parseURL(),
+    parseURL,
     checkBlacklist(blacklist),
-    fetchPackage(),
-    findFile(),
+    fetchPackage,
+    findFile,
     serveFile(autoIndex, maximumDepth)
   )
 

server/middleware/parseURL.js

@@ -3,8 +3,7 @@ const { parsePackageURL } = require('./PackageUtils')
 /**
  * Parse and validate the URL.
  */
-function parseURL() {
-  return function (req, res, next) {
+function parseURL(req, res, next) {
   let url
   try {
     url = parsePackageURL(req.url)
@@ -24,7 +23,6 @@ function parseURL() {
   req.query = url.query
 
   next()
-  }
 }
 
 module.exports = parseURL

server/middleware/serveFile.js

@@ -1,5 +1,4 @@
 const path = require('path')
-const PackageInfo = require('../PackageInfo')
 const { generateMetadata } = require('./MetadataUtils')
 const { generateDirectoryIndexHTML } = require('./IndexUtils')
 const { sendFile } = require('./ResponseUtils')
@@ -22,19 +21,13 @@ function serveFile(autoIndex, maximumDepth) {
       // TODO: use res.sendFile instead of our own custom function?
       sendFile(res, path.join(req.packageDir, req.file), req.stats, 31536000)
     } else if (autoIndex && req.stats.isDirectory()) {
-      PackageInfo.get(req.packageName, function (error, packageInfo) {
-        if (error) {
-          res.status(500).send(`Cannot generate index page for ${req.packageSpec}${req.filename}`)
-        } else {
-          generateDirectoryIndexHTML(packageInfo, req.packageVersion, req.packageDir, req.file, function (error, html) {
+      generateDirectoryIndexHTML(req.packageInfo, req.packageVersion, req.packageDir, req.file, function (error, html) {
         if (html) {
           res.send(html)
         } else {
           res.status(500).send(`Cannot generate index page for ${req.packageSpec}${req.filename}`)
         }
       })
-        }
-      })
     } else {
       res.status(403).send(`Cannot serve ${req.packageSpec}${req.filename}; it's not a req.file`)
     }