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

Already on GitHub? Sign in to your account

Circular dep between buster and when.js #204

Closed
briancavalier opened this Issue Jun 22, 2012 · 19 comments

Comments

Projects
None yet
5 participants

Hey guys, I don't know when this started, but it had to have been after when.js 1.3 was released, since the Travis build was good at that point. It seems like we have some sort of circular dep now, with buster using when internally, and when using buster as its unit test framework. It's pretty easy for me to reproduce it:

  1. git clone https://github.com/cujojs/when.git
  2. cd when
  3. npm install

npm dies for me with a circular dependency. If instead of step 3, I install buster globally, it will install ok, and then npm test works just fine.

Here it is happening in travis as well.

Any ideas what might be going on and how we can fix it?

Possibly related: My buster-server instance keeps crashing with the following error (happened since I installed the 0.6.0 Beta 4 version)

/usr/local/lib/node_modules/buster/node_modules/when/when.js:246
                throw new Error("already completed");
          ^
Error: already completed
    at alreadyCompleted (/usr/local/lib/node_modules/buster/node_modules/when/when.js:246:11)
    at Object.resolve (/usr/local/lib/node_modules/buster/node_modules/when/when.js:189:4)
    at Object._onSessionLoaded (/usr/local/lib/node_modules/buster/node_modules/buster-server-cli/node_modules/buster-capture-server/lib/slave.js:103:35)
    at /usr/local/lib/node_modules/buster/node_modules/buster-server-cli/node_modules/buster-capture-server/lib/pubsub-client.js:84:47
    at Object.trigger (/usr/local/lib/node_modules/buster/node_modules/buster-server-cli/node_modules/buster-capture-server/node_modules/faye/node/faye-node.js:383:19)
    at Object.distributeMessage (/usr/local/lib/node_modules/buster/node_modules/buster-server-cli/node_modules/buster-capture-server/node_modules/faye/node/faye-node.js:666:30)
    at Object._deliverMessage (/usr/local/lib/node_modules/buster/node_modules/buster-server-cli/node_modules/buster-capture-server/node_modules/faye/node/faye-node.js:1068:20)
    at Object.<anonymous> (/usr/local/lib/node_modules/buster/node_modules/buster-server-cli/node_modules/buster-capture-server/node_modules/faye/node/faye-node.js:1007:12)
    at Object.pipeThroughExtensions (/usr/local/lib/node_modules/buster/node_modules/buster-server-cli/node_modules/buster-capture-server/node_modules/faye/node/faye-node.js:529:44)
    at Object.receiveMessage (/usr/local/lib/node_modules/buster/node_modules/buster-server-cli/node_modules/buster-capture-server/node_modules/faye/node/faye-node.js:1003:10)

It runs fine for a minute or so, but then crashes repeatedly

@danielstocks My instinct is that this is a separate issue. The problem I'm seeing is during development of when.js, at the point of using npm install to install buster locally in the when.js node_modules folder. This particular problem doesn't seem to be related to buster.js runtime.

Filing new issue might be best, although I'll defer to @cjohansen and @augustl on that.

That said, it looks like buster-server-cli _onSessionLoad is attempting to resolve the same promise more than once, in which case when.js will throw that particular exception. I've used the same version (0.6.0 Beta 4) of buster server a little, but not a lot, and haven't seen that happen, so maybe it's something about your particular configuration that reveals the problem.

Owner

augustl commented Jun 26, 2012

Sorry for the delayed response, Chris is completely AFK and I'm somewhat AFK.

Not sure what the solution is here.. I pinged Isaac S. on IRC, I'll post here when he responds (he usually does).

@danielstocks this seems like an unrelated issue, feel free to file a separate issue for it :)

Thanks @augustl. It definitely has something to do with running npm install inside the when.js dev dir. The weird thing is that it seems to have just started happening. Maybe there is a correlation with switching buster's dependency on when.js from a github url to the npm package?

Thanks for pinging Isaac, hopefully he knows the magic.

It seems like the problem happens only when the version number in when.js's package.json matches the one that the version of buster, also specified in when.js package.json, depends on. If I change the version number in my local when.js package.json to something that doesn't match, then everything works.

So, it seems like the short-term workaround for me is to change the version in my package.json and push that to get travis back on track. Going forward, though, we'll need to figure out how to handle this.

Well, I tried pushing a change to the package.json version, and while that did get past the circular dep issue, it failed in a different way. For some reason, when running buster now (via npm test), it can't find the "when" module, which seems absurd.

Any ideas?

Owner

augustl commented Jun 29, 2012

I guess the easiest way to fix this is to create a step-by-step instruction on how to reproduce this, and post it as an issue to the npm project. This seems like some kind of npm bug to me.

Can you create the step-by-step list and post the issue? Seems like that might be easier for you, since you probably already have the steps required in your head. Let me know if you want me to do something though.

I can try to do that. The circular dep is easy to reproduce, but I've tried everything I can think of and haven't been able to reproduce the second failure outside of travis, so that one could be tougher. So, I guess I'll start with the circular dep.

Owner

cjohansen commented Jul 3, 2012

Seems to me this problem occurred as we switched from tarball dependencies to "proper" dependencies for when. If that is indeed the case, we could easily go back to using tarball dependencies for third-party deps. @briancavalier do you think that's the case?

Ah, that very well could be it--the timing seems right. I wonder if we can devise a way to test it--maybe by pushing a one-off version to npm under a different name, and then I update when to depend on it and see if Travis gives it the ok? What do you think? Any other ideas?

It kinda sucks that you'd need revert to using a github url, tho.

Owner

cjohansen commented Jul 4, 2012

The problem is that When is a dependency on several submodules, so doing the one-off thing is a bit cumbersome :| I'll see if I can figure out the problem with two dummy modules in stead.

Owner

cjohansen commented Jul 5, 2012

I studied the "other" error you got, and I recognize it from our own failing Travis builds. I'm not exactly sure why, or if it's a bug, but as far as I can tell, this happens:

  • npm install starts resolving dependencies
  • Upon encountering buster-test, which depends on when, the when dependency is skipped, because the project is already available on the load path (i.e., in the root when project)
  • For some reason, the same load path is not available to buster-test at runtime, so missing the installed dependency, it is not able to load when.

@briancavalier, if you're not in a hurry I will try to revert to tarball URLs for the 0.6.1 release (should be a minor bugfix release in a week or so). Ok?

@cjohansen cjohansen was assigned Jul 5, 2012

Thanks for the detective work, @cjno, I appreciate it. That second error sure sounds like a Travis bug, or at least an edge case that isn't covered. Sure, reverting to the tarball URL a week or so is cool with me.

I created a when.js branch that shows the circular sep problem, but I haven't had time to create an npm issue. Hopefully I can find some time for that tonight or tomorrow, at least to see if the npm folks have any insight.

Owner

cjohansen commented Jul 5, 2012

Reverted everything to tarballs. We'll see how it plays out when I drop a new release.

Owner

cjohansen commented Jul 9, 2012

@briancavalier We're back on tarball dependencies. Care to check if your Travis setup works with Buster 0.6.1?

Thanks for the heads up. I'll give it a go when I get back to a real keyboard tonight and let you know.

\o/ It's a win

Thanks guys. It stinks that you had to go back to using a tarball, but I really appreciate it.

Owner

cjohansen commented Jul 11, 2012

Nah, don't sweat it. So long as we can keep using the wonderful when, I'm not the one to complain over how we link it :)

Owner

dominykas commented Nov 8, 2015

Ran a quick test - this is now fixed: npm/npm#2063 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment