fix: over eager preload (#1693)

TLDR; Preload was sending preload 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.

---

Adding file(s) causes the preload module to preload any **root** nodes for the added content. It sends a HTTP request to preload for each root CID because the API on the preload nodes will fetch any children automatically. This greatly reduces the number of HTTP requests we have make when adding large files that are chunked into multiple nodes.

However, the issue was that the tests were checking that a CID had been requested for preload, not that it had been requested only once.

I was inspecting the debug output for preload because of the recent [CORS issues we've been having](ipfs/infra#447) and noticed that multiple preload requests for the same CID were being sent. Worse still, when adding large files, the child nodes were also being requested 😱 

The issues are:

1. `ipfs.add` causes [`object.get`](https://github.com/ipfs/js-ipfs/blob/99fb64e72c1752b3c227537dc6419d8bb837cd5f/src/core/components/files.js#L39) to be called for every file added. The issue with that is that `object.get` will also attempt to preload the CID you pass it.
2. `ipfs.add` causes `pin.add` to be called for each root CID. This in turn causes `dag.get` to be called for _every_ node in the graph. The issue with that is that `dag.get` will also attempt to preload the CID you pass it.

The solution in both cases is to tell `object.get` and `pin.add` in the context of `ipfs.add` to not preload the CIDs that they are passed.

I've augmented the tests to ensure that only one of the required CIDs is requested from the preload nodes and with that change and the fixes to the code the tests are now passing.

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

Author: Alan Shaw

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)

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)
   }

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)
         })
       }
     }),

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)

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)
+        })
       })
     })
   })

test/utils/mock-preload-node.js

@@ -142,7 +142,22 @@ module.exports.waitForCids = (cids, opts, cb) => {
     getPreloadCids(opts.addr, (err, preloadCids) => {
       if (err) return cb(err)
 
-      if (cids.every(cid => preloadCids.includes(cid))) {
+      // See if our cached preloadCids includes all the cids we're looking for.
+      const { missing, duplicates } = cids.reduce((results, cid) => {
+        const count = preloadCids.filter(preloadedCid => preloadedCid === cid).length
+        if (count === 0) {
+          results.missing.push(cid)
+        } else if (count > 1) {
+          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) {
         return cb()
       }