Fix __dirname sometimes pointing to wrong package

[pmowrer]

The bug appears to be due to a race condition. As such, it was difficult writing a test. The one I came up with fails reasonably consistently at about 1/3 of the time on my machine.

[zertosh]

@pmowrer I noticed that your PR is using the latest master. But since #80 was merged in 30 min before your PR, I can't help but wonder if maybe while you were dev'ing this fix you were testing against an older master. I don't know if #80 fixes the __dirname bug, but can you confirm that you're still able to repro the bug?

[pmowrer]

@zertosh I saw that merge and rebased the PR: https://github.com/pmowrer/module-deps/commits/a046e24c930952421143f86b0cdbd0f559bd8585. Anyway, I double checked and I was still able to reproduce the error using the test in the PR (it may require running the suite up to 10 times, at least on my end).

[zertosh]

@pmowrer I haven't forgotten about this. I haven't had time to verify it :(

[pmowrer]

Thanks for the update @zertosh. It's a major problem imo as this prevents bundle outputs from being deterministic (which is one of the major goals of module-deps in the first place). When running the tests for our plugin (https://github.com/Updater/browserify-resolutions), at least one fails ~50% of the time the tests are run. With the proposed fix, the tests are 100% reliable.

[zertosh]

@pmowrer I've been playing around with this on-and-off, and I'm (kinda) happy to report that I can reproduce it 100% of the time now. The "root" cause is similar to #80 - essentially, browser-resolve receiving A and B, but B finishing before A.

To reliably repro, I pass in my own resolver function and enforce a particular order. Here is a modified version of your test, that can demo the bug in an unpatched module-deps:

var mdeps = require('../');
var test = require('tape');
var path = require('path');
var browserResolve = require('browser-resolve');

var dirname = path.join(__dirname, '/pkg');

test('pkg', function (t) {
    t.plan(4);

    var queue = [];

    var d = mdeps({
      resolve: function(id, opts, cb) {
        if (id === 'pkga' || id === 'pkgb') {
          queue.push({id: id, opts: opts, cb: cb});
        } else {
          return process.nextTick(browserResolve.bind(null, id, opts, cb));
        }
        if (queue.length !== 2) return;

        // "pop" since resolve is called in the order that
        // the deps appear in a file - and pkga comes before pkgb
        var pkgb = queue.pop();
        var pkga = queue.pop();

        /* always FAILS this test */
        browserResolve(pkgb.id, pkgb.opts, function() {
          pkgb.args = [].slice.call(arguments);
          browserResolve(pkga.id, pkga.opts, function() {
            pkga.args = [].slice.call(arguments);
            pkga.cb.apply(null, pkga.args);
            pkgb.cb.apply(null, pkgb.args);
          });
        });

        /* always PASSES this test */
        // browserResolve(pkga.id, pkga.opts, function() {
        //   pkga.args = [].slice.call(arguments);
        //   browserResolve(pkgb.id, pkgb.opts, function() {
        //     pkgb.args = [].slice.call(arguments);
        //     pkgb.cb.apply(null, pkgb.args);
        //     pkga.cb.apply(null, pkga.args);
        //   });
        // });
      }
    });
    d.on('package', function (pkg_) {
        var pkg = require(dirname + pkg_.dir + '/package.json');
        pkg.__dirname = dirname + pkg_.dir;

        t.deepEqual(pkg_, pkg);
    });
    d.end(path.join(__dirname, '/pkg/main.js'));
    d.resume();
});

The reason that module-deps is susceptible to this bug is because parent in Deps.prototype.resolve = function (id, parent, cb) {} is the exact same object for every dep in a file. parent is named current at its origin, and it comes from here. Notice how it's the same object passed into walk, which then passes it to resolve for every dep. The bug is that a new parent.packageFilter function is tacked on to the shared parent object. So if multiple resolves are in flight, the "last" version of packageFilter and its enclosing environment wins out - thus "deciding" the pkgdir for all in flight.

The reason your solution works is because the resolver will call packageFilter before the onresolve function with the same instance of the p/pkg object. So, since your version of packageFilter doesn't use any enclosed variable (except for opts - but that's the same obj for everyone), it doesn't matter that it gets overwritten for every dep in transit.

Here is an alternate fix. It simply makes sure that every resolve gets its own parent object. So parent.packageFilter doesn't get clobbered, and pkgdir is properly preserved in its environment:

diff --git a/index.js b/index.js
index 307924e6..df9c1ab1 100644
--- a/index.js
+++ b/index.js
@@ -376,30 +376,30 @@ Deps.prototype.walk = function (id, parent, cb) {
             if (deps) fromDeps(file, src, pkg, deps);
         }
     });

     function fromDeps (file, src, pkg, deps) {
         var p = deps.length;
-        var current = {
-            id: file,
-            filename: file,
-            paths: self.paths,
-            package: pkg
-        };
         var resolved = {};

         if (input) --self.inputPending;

         (function resolve () {
             if (self.inputPending > 0) return setTimeout(resolve);
             deps.forEach(function (id) {
                 if (opts.filter && !opts.filter(id)) {
                     resolved[id] = false;
                     if (--p === 0) done();
                     return;
                 }
+                var current = {
+                    id: file,
+                    filename: file,
+                    paths: self.paths,
+                    package: pkg
+                };
                 self.walk(id, current, function (err, r) {
                     resolved[id] = r;
                     if (--p === 0) done();
                 });
             });
             if (deps.length === 0) done();

But I like your solution too because it makes it easier to reason about the value of pkgdir. So, I'll merge yours and then add mine 😄 But, there is a small tweak I think you should make. pkgdir is always a value since path.dirname always returns something. So p.__dirname = path.dirname(x); after calling opts.packageFilter(p, x); should suffice. My guess is that if (pkg && pkgdir) pkg.__dirname = pkgdir; was there before in case something else clobbered the parent.packageFilter function.

Don't worry about enforcing a resolve order in your test - unless you can think of a neater way to do it than my example. My code looks nasty :( I need a better test.

Sorry for the super long explanation. I'm trying to form documentation for these things for the benefit of others (especially collaborators like @jmm and @terinjokes).

PS: I ran this PR against the browserify tests and they pass.

[pmowrer]

@zertosh Nice work! It's unfortunate its so difficult to write a deterministic test for this. Makes sense it'd take a custom resolve function to get there. I've updated the PR with removal of that null check. Good call.

[zertosh]

Looks good! But sorry I forgot one last thing... In the test, can you fs.readFileSync+ JSON.parse, instead of require the package.jsons, so we don't pollute the global module cache? I'd change it myself but I don't want to steal your commit points 😄

[zertosh]

@pmowrer Something along these lines.

[jmm]

Sorry for the super long explanation. I'm trying to form documentation for these things for the benefit of others (especially collaborators like @jmm and @terinjokes).

👍 for the detailed post-mortem (and tackling these tricky issues in the first place). I didn't look at much of the code, but I read the post and I'm sure that'll be super helpful if I have to debug something related or similar to that.

[pmowrer]

@zertosh Haha, thanks! I've made the update. 😀

[terinjokes]

Initial pass looks good, and thanks @zertosh for the post-mortem.

@pmowrer mind squashing these commits?

[pmowrer]

@terinjokes I squashed them into 2, one for the failing test and one for the fix.

[zertosh]

Thanks a ton @pmowrer!

[zertosh]

Published as v3.7.9.

@pmowrer Just update your browserify and you're good to go - since the version floats. Nonetheless, I'll bump browserify's module-deps so that the changelog there reflects that this is fixed.

[pmowrer]

Awesome, thanks everyone!