From a977a30f7c7984ee8c5f8f1fce02148077ed9e6a Mon Sep 17 00:00:00 2001 From: Louis-Dominique Dubeau Date: Tue, 2 Jul 2019 15:37:55 -0400 Subject: [PATCH 1/2] test(git): check that a git failure in an extract call is reported Without the code fix that will come in the next commit, the new test fails to complete because of the bug in the codebase. --- test/git.extract.js | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 test/git.extract.js diff --git a/test/git.extract.js b/test/git.extract.js new file mode 100644 index 0000000..8bad851 --- /dev/null +++ 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') + }) +}) From a65cf2e0a9b7424816bdb28fcbdff7c1410813d4 Mon Sep 17 00:00:00 2001 From: Louis-Dominique Dubeau Date: Tue, 2 Jul 2019 16:09:36 -0400 Subject: [PATCH 2/2] fix(git): ensure stream failures are reported up the promise chain 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. --- extract.js | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/extract.js b/extract.js index d2ab47d..f49a544 100644 --- 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 }) }