fix(git): ensure stream failures are reported

lddubeau · isaacs · commit 7f07b5d6d74a · 2019-07-16T16:21:15.000-07:00
Prior to the change, race conditions could cause errors on the stream to be lost. The symptom for npm users would be the infamous "cb() never called" error. PR-URL: #1 Close: #1 Reviewed-by: @isaacs
diff --git a/extract.js b/extract.js
@@ -50,21 +50,22 @@ function extract (spec, dest, opts) {
 function tryExtract (spec, tarStream, dest, opts) {
   return new BB((resolve, reject) => {
     tarStream.on('error', reject)
-    setImmediate(resolve)
+
+    rimraf(dest)
+      .then(() => mkdirp(dest))
+      .then(() => {
+        const xtractor = extractStream(spec, dest, opts)
+        xtractor.on('error', reject)
+        xtractor.on('close', resolve)
+        tarStream.pipe(xtractor)
+      })
+      .catch(reject)
   })
-    .then(() => rimraf(dest))
-    .then(() => mkdirp(dest))
-    .then(() => new BB((resolve, reject) => {
-      const xtractor = extractStream(spec, dest, opts)
-      tarStream.on('error', reject)
-      xtractor.on('error', reject)
-      xtractor.on('close', resolve)
-      tarStream.pipe(xtractor)
-    }))
     .catch(err => {
       if (err.code === 'EINTEGRITY') {
         err.message = `Verification failed while extracting ${spec}:\n${err.message}`
       }
+
       throw err
     })
 }
diff --git a/test/git.extract.js b/test/git.extract.js
@@ -0,0 +1,41 @@
+'use strict'
+
+const BB = require('bluebird')
+const fs = BB.promisifyAll(require('fs'))
+const mkdirp = BB.promisify(require('mkdirp'))
+const npmlog = require('npmlog')
+const path = require('path')
+const test = require('tap').test
+
+const testDir = require('./util/test-dir')(__filename)
+
+const extract = require('../extract.js')
+
+npmlog.level = process.env.LOGLEVEL || 'silent'
+const OPTS = {
+  git: 'git',
+  cache: path.join(testDir, 'cache'),
+  log: npmlog,
+  registry: 'https://my.mock.registry/',
+  retry: false
+}
+
+test('extracting a git target reports failures', t => {
+  const oldPath = process.env.PATH
+  process.env.PATH = ''
+  const dest = path.join(testDir, 'foo')
+  return mkdirp(dest)
+    .then(() => fs.writeFileAsync(path.join(dest, 'q'), 'foo'))
+    .then(() => extract('github:zkat/pacote', dest,
+      Object.assign({}, OPTS)))
+    .finally(() => {
+      process.env.PATH = oldPath
+    })
+    .then(() => {
+      t.fail('the promise should not have resolved')
+    }, (err) => {
+      // We're not testing the specific text of the error message. We just check
+      // that it is an execution error.
+      t.equal(err.code, 'ENOENT')
+    })
+})
