Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: move to async functions where possible #106

Merged
merged 1 commit into from
May 3, 2022
Merged

fix: move to async functions where possible #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .eslintrc.local.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
'use strict'

module.exports = {
rules: {
strict: 'error',
'no-shadow': 0, // XXX: fix this later
},
}
163 changes: 74 additions & 89 deletions lib/content/read.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,42 +1,35 @@
'use strict'

const util = require('util')

const fs = require('fs')
const fs = require('@npmcli/fs')
const fsm = require('fs-minipass')
const ssri = require('ssri')
const contentPath = require('./path')
const Pipeline = require('minipass-pipeline')

const lstat = util.promisify(fs.lstat)
const readFile = util.promisify(fs.readFile)
const copyFile = util.promisify(fs.copyFile)

module.exports = read

const MAX_SINGLE_READ_SIZE = 64 * 1024 * 1024
function read (cache, integrity, opts = {}) {
async function read (cache, integrity, opts = {}) {
const { size } = opts
return withContentSri(cache, integrity, (cpath, sri) => {
const { stat, cpath, sri } = await withContentSri(cache, integrity, async (cpath, sri) => {
// get size
return lstat(cpath).then(stat => ({ stat, cpath, sri }))
}).then(({ stat, cpath, sri }) => {
if (typeof size === 'number' && stat.size !== size) {
throw sizeError(size, stat.size)
}
const stat = await fs.lstat(cpath)
return { stat, cpath, sri }
})
if (typeof size === 'number' && stat.size !== size) {
throw sizeError(size, stat.size)
}

if (stat.size > MAX_SINGLE_READ_SIZE) {
return readPipeline(cpath, stat.size, sri, new Pipeline()).concat()
}
if (stat.size > MAX_SINGLE_READ_SIZE) {
return readPipeline(cpath, stat.size, sri, new Pipeline()).concat()
}

return readFile(cpath, null).then((data) => {
if (!ssri.checkData(data, sri)) {
throw integrityError(sri, cpath)
}
const data = await fs.readFile(cpath, { encoding: null })
if (!ssri.checkData(data, sri)) {
throw integrityError(sri, cpath)
}

return data
})
})
return data
}

const readPipeline = (cpath, size, sri, stream) => {
Expand All @@ -58,7 +51,7 @@ module.exports.sync = readSync
function readSync (cache, integrity, opts = {}) {
const { size } = opts
return withContentSriSync(cache, integrity, (cpath, sri) => {
const data = fs.readFileSync(cpath)
const data = fs.readFileSync(cpath, { encoding: null })
if (typeof size === 'number' && size !== data.length) {
throw sizeError(size, data.length)
}
Expand All @@ -77,16 +70,19 @@ module.exports.readStream = readStream
function readStream (cache, integrity, opts = {}) {
const { size } = opts
const stream = new Pipeline()
withContentSri(cache, integrity, (cpath, sri) => {
// just lstat to ensure it exists
return lstat(cpath).then((stat) => ({ stat, cpath, sri }))
}).then(({ stat, cpath, sri }) => {
// Set all this up to run on the stream and then just return the stream
Promise.resolve().then(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it took me a second to see what this was doing.. seems like the intent here is to run the code within the .then() handler after a short delay?

i don't have a solid suggestion for making this more readable, setImmediate(async () => {}) feels like it would be, but you'd have to also put a try/catch inside the function. maybe just a comment explaining is enough?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I fiddled a bit w/ this, this was as good as it got. The comment was supposed to explain it lol.

const { stat, cpath, sri } = await withContentSri(cache, integrity, async (cpath, sri) => {
// just lstat to ensure it exists
const stat = await fs.lstat(cpath)
return { stat, cpath, sri }
})
if (typeof size === 'number' && size !== stat.size) {
return stream.emit('error', sizeError(size, stat.size))
}

readPipeline(cpath, stat.size, sri, stream)
}, er => stream.emit('error', er))
}).catch(err => stream.emit('error', err))

return stream
}
Expand All @@ -96,7 +92,7 @@ module.exports.copy.sync = copySync

function copy (cache, integrity, dest) {
return withContentSri(cache, integrity, (cpath, sri) => {
return copyFile(cpath, dest)
return fs.copyFile(cpath, dest)
})
}

Expand All @@ -108,14 +104,17 @@ function copySync (cache, integrity, dest) {

module.exports.hasContent = hasContent

function hasContent (cache, integrity) {
async function hasContent (cache, integrity) {
if (!integrity) {
return Promise.resolve(false)
return false
}

return withContentSri(cache, integrity, (cpath, sri) => {
return lstat(cpath).then((stat) => ({ size: stat.size, sri, stat }))
}).catch((err) => {
try {
return await withContentSri(cache, integrity, async (cpath, sri) => {
const stat = await fs.lstat(cpath)
return { size: stat.size, sri, stat }
})
} catch (err) {
if (err.code === 'ENOENT') {
return false
}
Expand All @@ -128,7 +127,7 @@ function hasContent (cache, integrity) {
return false
}
}
})
}
}

module.exports.hasContent.sync = hasContentSync
Expand Down Expand Up @@ -159,61 +158,47 @@ function hasContentSync (cache, integrity) {
})
}

function withContentSri (cache, integrity, fn) {
const tryFn = () => {
const sri = ssri.parse(integrity)
// If `integrity` has multiple entries, pick the first digest
// with available local data.
const algo = sri.pickAlgorithm()
const digests = sri[algo]

if (digests.length <= 1) {
const cpath = contentPath(cache, digests[0])
return fn(cpath, digests[0])
} else {
// Can't use race here because a generic error can happen before
// a ENOENT error, and can happen before a valid result
return Promise
.all(digests.map((meta) => {
return withContentSri(cache, meta, fn)
.catch((err) => {
if (err.code === 'ENOENT') {
return Object.assign(
new Error('No matching content found for ' + sri.toString()),
{ code: 'ENOENT' }
)
}
return err
})
}))
.then((results) => {
// Return the first non error if it is found
const result = results.find((r) => !(r instanceof Error))
if (result) {
return result
}

// Throw the No matching content found error
const enoentError = results.find((r) => r.code === 'ENOENT')
if (enoentError) {
throw enoentError
}

// Throw generic error
throw results.find((r) => r instanceof Error)
})
async function withContentSri (cache, integrity, fn) {
const sri = ssri.parse(integrity)
// If `integrity` has multiple entries, pick the first digest
// with available local data.
const algo = sri.pickAlgorithm()
const digests = sri[algo]

if (digests.length <= 1) {
const cpath = contentPath(cache, digests[0])
return fn(cpath, digests[0])
} else {
// Can't use race here because a generic error can happen before
// a ENOENT error, and can happen before a valid result
const results = await Promise.all(digests.map(async (meta) => {
try {
return await withContentSri(cache, meta, fn)
} catch (err) {
if (err.code === 'ENOENT') {
return Object.assign(
new Error('No matching content found for ' + sri.toString()),
{ code: 'ENOENT' }
)
}
return err
}
}))
// Return the first non error if it is found
const result = results.find((r) => !(r instanceof Error))
if (result) {
return result
}
}

return new Promise((resolve, reject) => {
try {
tryFn()
.then(resolve)
.catch(reject)
} catch (err) {
reject(err)
// Throw the No matching content found error
const enoentError = results.find((r) => r.code === 'ENOENT')
if (enoentError) {
throw enoentError
}
})

// Throw generic error
throw results.find((r) => r instanceof Error)
}
}

function withContentSriSync (cache, integrity, fn) {
Expand Down
18 changes: 9 additions & 9 deletions lib/content/rm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ const rimraf = util.promisify(require('rimraf'))

module.exports = rm

function rm (cache, integrity) {
return hasContent(cache, integrity).then((content) => {
// ~pretty~ sure we can't end up with a content lacking sri, but be safe
if (content && content.sri) {
return rimraf(contentPath(cache, content.sri)).then(() => true)
} else {
return false
}
})
async function rm (cache, integrity) {
const content = await hasContent(cache, integrity)
// ~pretty~ sure we can't end up with a content lacking sri, but be safe
if (content && content.sri) {
await rimraf(contentPath(cache, content.sri))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we've brought @npmcli/fs in already, this could be fs.rm() instead of rimraf and the rimraf dependency can go away (after checking for other uses, of course)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just did more review and found that we use rimraf.sync in at least one place, so the above would actually require removing the synchronous interface from cacache. i'm personally in favor of doing so, but it should very much be a separate pull request

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this was ONLY supposed to move to @npmcli/fs. Switching to its functionality specifically was always intented to be a separate PR.

return true
} else {
return false
}
}
33 changes: 14 additions & 19 deletions lib/content/write.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const util = require('util')

const contentPath = require('./path')
const fixOwner = require('../util/fix-owner')
const fs = require('fs')
const fs = require('@npmcli/fs')
const moveFile = require('../util/move-file')
const Minipass = require('minipass')
const Pipeline = require('minipass-pipeline')
Expand All @@ -15,8 +15,6 @@ const ssri = require('ssri')
const uniqueFilename = require('unique-filename')
const fsm = require('fs-minipass')

const writeFile = util.promisify(fs.writeFile)

module.exports = write

async function write (cache, data, opts = {}) {
Expand All @@ -36,7 +34,7 @@ async function write (cache, data, opts = {}) {

const tmp = await makeTmp(cache, opts)
try {
await writeFile(tmp.target, data, { flag: 'wx' })
await fs.writeFile(tmp.target, data, { flag: 'wx' })
await moveToDestination(tmp, cache, sri, opts)
return { integrity: sri, size: data.length }
} finally {
Expand Down Expand Up @@ -115,7 +113,7 @@ async function handleContent (inputStream, cache, opts) {
}
}

function pipeToTmp (inputStream, cache, tmpTarget, opts) {
async function pipeToTmp (inputStream, cache, tmpTarget, opts) {
let integrity
let size
const hashStream = ssri.integrityStream({
Expand Down Expand Up @@ -143,30 +141,27 @@ function pipeToTmp (inputStream, cache, tmpTarget, opts) {
outStream
)

return pipeline.promise().then(() => ({ integrity, size }))
await pipeline.promise()
return { integrity, size }
}

function makeTmp (cache, opts) {
async function makeTmp (cache, opts) {
const tmpTarget = uniqueFilename(path.join(cache, 'tmp'), opts.tmpPrefix)
return fixOwner.mkdirfix(cache, path.dirname(tmpTarget)).then(() => ({
await fixOwner.mkdirfix(cache, path.dirname(tmpTarget))
return {
target: tmpTarget,
moved: false,
}))
}
}

function moveToDestination (tmp, cache, sri, opts) {
async function moveToDestination (tmp, cache, sri, opts) {
const destination = contentPath(cache, sri)
const destDir = path.dirname(destination)

return fixOwner
.mkdirfix(cache, destDir)
.then(() => {
return moveFile(tmp.target, destination)
})
.then(() => {
tmp.moved = true
return fixOwner.chownr(cache, destination)
})
await fixOwner.mkdirfix(cache, destDir)
await moveFile(tmp.target, destination)
tmp.moved = true
await fixOwner.chownr(cache, destination)
}

function sizeError (expected, found) {
Expand Down
Loading