jorendorff / js-loaders

Pseudoimplementation of the proposed ES6 module loaders.
54 stars 7 forks source link

loadingCount race conditions #80

Closed guybedford closed 10 years ago

guybedford commented 10 years ago

I don't know how to track this down, but it seems like the implementation of loadingCount in this sample implementation ends up being incorrect in some scenarios.

Specifically when two modules are imported at the same time, with shared dependencies:

  // module/a.js depends on module/dep.js
  // module/b.js depends on module/dep.js
  System.import('module/a').then(function(m) {
    // fires correctly
  });
  System.import('module/b').then(function(m) {
    // never fires
  });

This seems to have something to do with the loadingCount value not being reduced for the duplicated dependency at the right time.

It's difficult to know if this is just my code, or the algorithm itself, so I apologise in advance if it turns out this is something I'm missing.

Changing the detection to loop through the loads checking their loaded status as in the specification completely fixes the problem though.

guybedford commented 10 years ago

Closing this, as it isn't a primary issue.

jorendorff commented 10 years ago

All the same I'd like to track down the bug, so I'm reopening.

guybedford commented 10 years ago

I doubled checked the cause of this and I can confirm it is actually not related at all to #85.

It turns out I had a bad loop in addLoadToLinkSet, which was actually causing this.

I corrected this along with the other changes, so had to recheck to see this was the underlying cause.