From 8444b63dec79b7f97634223ae793b7455be18f6b Mon Sep 17 00:00:00 2001 From: Alan Shaw Date: Mon, 5 Nov 2018 13:56:17 +0000 Subject: [PATCH 1/2] fix: over eager preload Preload was sending perload requests for EVERY dag node of an added file. This is unnecessary as the preload request will recursively slurp down the entire graph. This PR fixes that behaviour. License: MIT Signed-off-by: Alan Shaw --- src/core/components/dag.js | 11 +++++++++-- src/core/components/files.js | 4 ++-- src/core/components/object.js | 6 +----- src/core/components/pin.js | 10 ++++++---- test/core/preload.spec.js | 9 +++++++-- test/utils/mock-preload-node.js | 16 +++++++++++++++- 6 files changed, 40 insertions(+), 16 deletions(-) diff --git a/src/core/components/dag.js b/src/core/components/dag.js index 5521591834..5b33ee8214 100644 --- a/src/core/components/dag.js +++ b/src/core/components/dag.js @@ -50,7 +50,14 @@ module.exports = function dag (self) { if (typeof options === 'function') { callback = options - options = {} + + // Allow options in path position + if (typeof path !== 'string') { + options = path + path = null + } else { + options = {} + } } options = options || {} @@ -156,7 +163,7 @@ module.exports = function dag (self) { if (err) { return callback(err) } mapAsync(res.value.links, (link, cb) => { - self.dag._getRecursive(link.multihash, cb) + self.dag._getRecursive(link.multihash, options, cb) }, (err, nodes) => { // console.log('nodes:', nodes) if (err) return callback(err) diff --git a/src/core/components/files.js b/src/core/components/files.js index d6f6d889a2..f13f42ae43 100644 --- a/src/core/components/files.js +++ b/src/core/components/files.js @@ -36,7 +36,7 @@ function prepareFile (self, opts, file, callback) { waterfall([ (cb) => opts.onlyHash ? cb(null, file) - : self.object.get(file.multihash, opts, cb), + : self.object.get(file.multihash, Object.assign({}, opts, { preload: false }), cb), (node, cb) => { const b58Hash = cid.toBaseEncodedString() @@ -118,7 +118,7 @@ function pinFile (self, opts, file, cb) { const isRootDir = !file.path.includes('/') const shouldPin = pin && isRootDir && !opts.onlyHash && !opts.hashAlg if (shouldPin) { - return self.pin.add(file.hash, err => cb(err, file)) + return self.pin.add(file.hash, { preload: false }, err => cb(err, file)) } else { cb(null, file) } diff --git a/src/core/components/object.js b/src/core/components/object.js index c0a3a82551..41ad06f1cd 100644 --- a/src/core/components/object.js +++ b/src/core/components/object.js @@ -206,11 +206,7 @@ module.exports = function object (self) { return callback(err) } - if (options.preload !== false) { - self._preload(cid) - } - - self.object.get(node.multihash, callback) + self.object.get(node.multihash, { preload: options.preload }, callback) }) } }), diff --git a/src/core/components/pin.js b/src/core/components/pin.js index 1b231a86fd..39e8765ca0 100644 --- a/src/core/components/pin.js +++ b/src/core/components/pin.js @@ -119,9 +119,11 @@ module.exports = (self) => { add: promisify((paths, options, callback) => { if (typeof options === 'function') { callback = options - options = null + options = {} } - const recursive = options ? options.recursive : true + options = options || {} + + const recursive = options.recursive == null ? true : options.recursive resolvePath(self.object, paths, (err, mhs) => { if (err) { return callback(err) } @@ -137,7 +139,7 @@ module.exports = (self) => { // entire graph of nested links should be pinned, // so make sure we have all the objects - dag._getRecursive(key, (err) => { + dag._getRecursive(key, { preload: options.preload }, (err) => { if (err) { return cb(err) } // found all objects, we can add the pin return cb(null, key) @@ -153,7 +155,7 @@ module.exports = (self) => { } // make sure we have the object - dag.get(new CID(multihash), (err) => { + dag.get(new CID(multihash), { preload: options.preload }, (err) => { if (err) { return cb(err) } // found the object, we can add the pin return cb(null, key) diff --git a/test/core/preload.spec.js b/test/core/preload.spec.js index 1f44814876..37528b81fe 100644 --- a/test/core/preload.spec.js +++ b/test/core/preload.spec.js @@ -141,9 +141,14 @@ describe('preload', () => { const wrappingDir = res.find(file => file.path === '') expect(wrappingDir).to.exist() - ipfs.ls(wrappingDir.hash, (err) => { + // Adding these files with have preloaded wrappingDir.hash, clear it out + MockPreloadNode.clearPreloadCids((err) => { expect(err).to.not.exist() - MockPreloadNode.waitForCids(wrappingDir.hash, done) + + ipfs.ls(wrappingDir.hash, (err) => { + expect(err).to.not.exist() + MockPreloadNode.waitForCids(wrappingDir.hash, done) + }) }) }) }) diff --git a/test/utils/mock-preload-node.js b/test/utils/mock-preload-node.js index 795413a31d..981a54aa01 100644 --- a/test/utils/mock-preload-node.js +++ b/test/utils/mock-preload-node.js @@ -142,7 +142,21 @@ module.exports.waitForCids = (cids, opts, cb) => { getPreloadCids(opts.addr, (err, preloadCids) => { if (err) return cb(err) - if (cids.every(cid => preloadCids.includes(cid))) { + const missing = [] + + // See if our cached preloadCids includes all the cids we're looking for. + for (var i = 0; i < cids.length; i++) { + const count = preloadCids.filter(cid => cid === cids[i]).length + + if (count === 0) { + missing.push(cids[i]) + } else if (count > 1) { + // Do not allow duplicates! + return cb(errCode(new Error(`Multiple occurances of ${cids[i]} found`), 'ERR_DUPLICATE')) + } + } + + if (missing.length === 0) { return cb() } From c415e9f0dae90f7f06100df35d457400909315b9 Mon Sep 17 00:00:00 2001 From: Alan Shaw Date: Mon, 5 Nov 2018 14:33:58 +0000 Subject: [PATCH 2/2] fix: appease linter License: MIT Signed-off-by: Alan Shaw --- test/utils/mock-preload-node.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/test/utils/mock-preload-node.js b/test/utils/mock-preload-node.js index 981a54aa01..713db9db22 100644 --- a/test/utils/mock-preload-node.js +++ b/test/utils/mock-preload-node.js @@ -142,18 +142,19 @@ module.exports.waitForCids = (cids, opts, cb) => { getPreloadCids(opts.addr, (err, preloadCids) => { if (err) return cb(err) - const missing = [] - // See if our cached preloadCids includes all the cids we're looking for. - for (var i = 0; i < cids.length; i++) { - const count = preloadCids.filter(cid => cid === cids[i]).length - + const { missing, duplicates } = cids.reduce((results, cid) => { + const count = preloadCids.filter(preloadedCid => preloadedCid === cid).length if (count === 0) { - missing.push(cids[i]) + results.missing.push(cid) } else if (count > 1) { - // Do not allow duplicates! - return cb(errCode(new Error(`Multiple occurances of ${cids[i]} found`), 'ERR_DUPLICATE')) + results.duplicates.push(cid) } + return results + }, { missing: [], duplicates: [] }) + + if (duplicates.length) { + return cb(errCode(new Error(`Multiple occurances of ${duplicates} found`), 'ERR_DUPLICATE')) } if (missing.length === 0) {