Skip to content

Commit

Permalink
fix(git): ensure stream failures are reported
Browse files Browse the repository at this point in the history
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
  • Loading branch information
lddubeau authored and isaacs committed Jul 16, 2019
1 parent fcf6de3 commit 7f07b5d
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 10 deletions.
21 changes: 11 additions & 10 deletions extract.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
}
41 changes: 41 additions & 0 deletions test/git.extract.js
Original file line number Diff line number Diff line change
@@ -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')
})
})

0 comments on commit 7f07b5d

Please sign in to comment.