diff --git a/lib/content/write.js b/lib/content/write.js index a71e81ad..b0aa18c1 100644 --- a/lib/content/write.js +++ b/lib/content/write.js @@ -13,34 +13,37 @@ const path = require('path') const rimraf = util.promisify(require('rimraf')) const ssri = require('ssri') const uniqueFilename = require('unique-filename') -const { disposer } = require('./../util/disposer') const fsm = require('fs-minipass') const writeFile = util.promisify(fs.writeFile) module.exports = write -function write (cache, data, opts = {}) { +async function write (cache, data, opts = {}) { const { algorithms, size, integrity } = opts if (algorithms && algorithms.length > 1) { throw new Error('opts.algorithms only supports a single algorithm for now') } if (typeof size === 'number' && data.length !== size) { - return Promise.reject(sizeError(size, data.length)) + throw sizeError(size, data.length) } const sri = ssri.fromData(data, algorithms ? { algorithms } : {}) if (integrity && !ssri.checkData(data, integrity, opts)) { - return Promise.reject(checksumError(integrity, sri)) + throw checksumError(integrity, sri) } - return disposer(makeTmp(cache, opts), makeTmpDisposer, - (tmp) => { - return writeFile(tmp.target, data, { flag: 'wx' }) - .then(() => moveToDestination(tmp, cache, sri, opts)) - }) - .then(() => ({ integrity: sri, size: data.length })) + const tmp = await makeTmp(cache, opts) + try { + await writeFile(tmp.target, data, { flag: 'wx' }) + await moveToDestination(tmp, cache, sri, opts) + return { integrity: sri, size: data.length } + } finally { + if (!tmp.moved) { + await rimraf(tmp.target) + } + } } module.exports.stream = writeStream @@ -94,18 +97,22 @@ function writeStream (cache, opts = {}) { return new CacacheWriteStream(cache, opts) } -function handleContent (inputStream, cache, opts) { - return disposer(makeTmp(cache, opts), makeTmpDisposer, (tmp) => { - return pipeToTmp(inputStream, cache, tmp.target, opts) - .then((res) => { - return moveToDestination( - tmp, - cache, - res.integrity, - opts - ).then(() => res) - }) - }) +async function handleContent (inputStream, cache, opts) { + const tmp = await makeTmp(cache, opts) + try { + const res = await pipeToTmp(inputStream, cache, tmp.target, opts) + await moveToDestination( + tmp, + cache, + res.integrity, + opts + ) + return res + } finally { + if (!tmp.moved) { + await rimraf(tmp.target) + } + } } function pipeToTmp (inputStream, cache, tmpTarget, opts) { @@ -136,11 +143,7 @@ function pipeToTmp (inputStream, cache, tmpTarget, opts) { outStream ) - return pipeline.promise() - .then(() => ({ integrity, size })) - .catch(er => rimraf(tmpTarget).then(() => { - throw er - })) + return pipeline.promise().then(() => ({ integrity, size })) } function makeTmp (cache, opts) { @@ -151,14 +154,6 @@ function makeTmp (cache, opts) { })) } -function makeTmpDisposer (tmp) { - if (tmp.moved) { - return Promise.resolve() - } - - return rimraf(tmp.target) -} - function moveToDestination (tmp, cache, sri, opts) { const destination = contentPath(cache, sri) const destDir = path.dirname(destination) diff --git a/lib/entry-index.js b/lib/entry-index.js index 426778b8..9d448562 100644 --- a/lib/entry-index.js +++ b/lib/entry-index.js @@ -8,7 +8,6 @@ const path = require('path') const ssri = require('ssri') const uniqueFilename = require('unique-filename') -const { disposer } = require('./util/disposer') const contentPath = require('./content/path') const fixOwner = require('./util/fix-owner') const hashToSegments = require('./util/hash-to-segments') @@ -102,7 +101,12 @@ async function compact (cache, key, matchFn, opts = {}) { } // write the file atomically - await disposer(setup(), teardown, write) + const tmp = await setup() + try { + await write(tmp) + } finally { + await teardown(tmp) + } // we reverse the list we generated such that the newest // entries come first in order to make looping through them easier diff --git a/lib/util/disposer.js b/lib/util/disposer.js deleted file mode 100644 index 52d7d3ed..00000000 --- a/lib/util/disposer.js +++ /dev/null @@ -1,31 +0,0 @@ -'use strict' - -module.exports.disposer = disposer - -function disposer (creatorFn, disposerFn, fn) { - const runDisposer = (resource, result, shouldThrow = false) => { - return disposerFn(resource) - .then( - // disposer resolved, do something with original fn's promise - () => { - if (shouldThrow) { - throw result - } - - return result - }, - // Disposer fn failed, crash process - (err) => { - throw err - // Or process.exit? - }) - } - - return creatorFn - .then((resource) => { - // fn(resource) can throw, so wrap in a promise here - return Promise.resolve().then(() => fn(resource)) - .then((result) => runDisposer(resource, result)) - .catch((err) => runDisposer(resource, err, true)) - }) -} diff --git a/test/content.write.js b/test/content.write.js index f066bc3b..737714ea 100644 --- a/test/content.write.js +++ b/test/content.write.js @@ -209,7 +209,7 @@ t.test('cleans up tmp on successful completion', (t) => { })) }) -t.test('cleans up tmp on error', (t) => { +t.test('cleans up tmp on streaming error', (t) => { const CONTENT = 'foobarbaz' const CACHE = t.testdir() return t.rejects( @@ -233,6 +233,25 @@ t.test('cleans up tmp on error', (t) => { })) }) +t.test('cleans up tmp on non streaming error', (t) => { + // mock writefile and make it reject + const CONTENT = 'foobarbaz' + const CACHE = t.testdir({ 'content-v2': 'oh no a file' }) + return t.rejects(write(CACHE, CONTENT)) + .then(() => new Promise((resolve, reject) => { + const tmp = path.join(CACHE, 'tmp') + fs.readdir(tmp, function (err, files) { + if (!err || (err && err.code === 'ENOENT')) { + files = files || [] + t.same(files, [], 'nothing in the tmp dir!') + resolve() + } else { + reject(err) + } + }) + })) +}) + t.test('checks the size of stream data if opts.size provided', (t) => { const CONTENT = 'foobarbaz' let int1 = null @@ -270,12 +289,11 @@ t.test('checks the size of stream data if opts.size provided', (t) => { }) }) -t.test('only one algorithm for now', t => { +t.test('only one algorithm for now', async t => { const CACHE = t.testdir() - t.throws(() => write(CACHE, 'foo', { algorithms: [1, 2] }), { + await t.rejects(() => write(CACHE, 'foo', { algorithms: [1, 2] }), { message: 'opts.algorithms only supports a single algorithm for now', }) - t.end() }) t.test('writes to cache with default options', t => { diff --git a/test/disposer.js b/test/disposer.js deleted file mode 100644 index 9a1ecd09..00000000 --- a/test/disposer.js +++ /dev/null @@ -1,142 +0,0 @@ -'use strict' - -const { disposer } = require('./../lib/util/disposer') -const { test } = require('tap') - -test('disposerFn should run in resolve', (t) => { - let disposerRan = false - - const mockCreatorResource = '__creator_resource__' - const mockFunctionResult = '__function_result__' - const creatorFn = () => { - return Promise.resolve(mockCreatorResource) - } - - const disposerFn = () => { - disposerRan = true - return Promise.resolve('Disposer Resolve') - } - - return disposer( - creatorFn(), - disposerFn, - (data) => { - t.equal(disposerRan, false, 'disposerFn should not have been called') - t.equal( - data, - mockCreatorResource, - 'Disposer not returning the created resource to running function' - ) - return Promise.resolve(mockFunctionResult) - }) - .then((data) => { - t.equal(disposerRan, true, 'disposerFn should have been called') - t.equal( - data, - mockFunctionResult, - 'Disposer not returning the returned result of the function' - ) - }) -}) - -test('disposerFn should run in reject', (t) => { - let disposerRan = false - - const mockCreatorResource = '__creator_resource__' - const mockFunctionResult = '__function_result__' - const creatorFn = () => { - return Promise.resolve(mockCreatorResource) - } - - const disposerFn = () => { - disposerRan = true - return Promise.resolve('Disposer Resolve') - } - - return disposer( - creatorFn(), - disposerFn, - (data) => { - t.equal(disposerRan, false, 'disposerFn should not have been called') - t.equal( - data, - mockCreatorResource, - 'Disposer not returning the created resource to running function' - ) - return Promise.reject(mockFunctionResult) - }) - .then( - () => { - throw new Error('expected a failure') - }, - (data) => { - t.equal(disposerRan, true, 'disposerFn should have been called') - t.equal( - data, - mockFunctionResult, - 'Disposer not returning the returned result of the function' - ) - }) -}) - -test('disposer should reject on creatorFn reject', (t) => { - let disposerRan = false - - const mockCreatorFailure = '__creator_fn_failure__' - const creatorFn = () => { - return Promise.reject(mockCreatorFailure) - } - - const disposerFn = () => { - disposerRan = true - return Promise.resolve('Disposer Resolve') - } - - return disposer( - creatorFn(), - disposerFn, - (data) => { - throw new Error('expected a failure') - }) - .catch((data) => { - t.equal(disposerRan, false, 'disposerFn should have not have been called') - t.equal( - data, - mockCreatorFailure, - 'Disposer not passing along the failure from creator function' - ) - }) -}) - -test('disposer should reject on disposerFn reject', (t) => { - let disposerRan = false - - const mockCreatorResource = '__creator_resource__' - const mockDisposerReject = '__disposer_reject__' - const creatorFn = () => { - return Promise.resolve(mockCreatorResource) - } - - const disposerFn = () => { - disposerRan = true - return Promise.reject(mockDisposerReject) - } - - return disposer( - creatorFn(), - disposerFn, - (data) => { - return 'foo' - }) - .then(() => { - throw new Error('expected a failure') - }) - .catch((data) => { - t.equal(disposerRan, true, 'disposerFn should have been called') - t.equal( - data, - mockDisposerReject, - 'Disposer not passing along the failure from disposer function' - ) - }) -})