lukeed / taskr

A fast, concurrency-focused task automation tool.
MIT License
2.54k stars 73 forks source link

Make plugins autoload more consistent (fix #268) #278

Closed karimsa closed 7 years ago

karimsa commented 7 years ago

This PR is to make the autoloading of fly-* plugins more consistent as per #268.

The way this is done is by moving the plugin autoloading from the cli spawing process directly into the fly class. That way the autoloading occurs under both cases: for CLI-use as well as with manual use.

karimsa commented 7 years ago

@jbucaran - informally requesting review; I've fixed the failing tests.

jorgebucaran commented 7 years ago

@karimsa Thanks!

karimsa commented 7 years ago

Just wanted to bump this PR.

jorgebucaran commented 7 years ago

@karimsa This branch has conflicts with the master branch. You have to rebase and apply your changes on top of the latest master.

screen shot 2017-07-02 at 16 48 37
jorgebucaran commented 7 years ago

Please don't update the LICENSE file.

karimsa commented 7 years ago

Sorry, that was the merge conflict between the master when I created this PR & now.

I believe it should work now. The only thing is the error handling - how did you want me to implement that? Just emit an error event on this?

jorgebucaran commented 7 years ago

Thanks @karimsa. I think @lukeed can take it from here. 👋😉

lukeed commented 7 years ago

Hi all! 👋 Sorry, I forgot to get to this after the adjustments were happening.

Thanks for your work @karimsa 🙏 I attempted to solved #268 on my own too, and arrived at nearly the exact same code.

The problem is that Bluebird.coroutine is not blocking, unlike the popular alternative tj/co. So what happens is that if, for whatever reason, the plugins.load is slower than the rest of the calls, your instance & tasks will fire off without any plugins loaded. (This is the case on practically all <= 3yr old machines.)

Worse, because of the this.pluginsReady flag, the "internal" *_start method has the option to never fire --- and that, indeed, is the what's happening here with Travis. This is also why you needed to call fly.start() inside the tests' spawn() helper. Is definitely a major, breaking change.

You're not alone though. I had similar issues, and this is why I punted the branch. You can see my explantation here.

For a simple run-down, this code block summarizes the problem perfectly:

const co = require('bluebird').coroutine;
const sleep = ms => new Promise(res => setTimeout(res,ms));

function test() {
  console.log('1');
  co(function * () {
    console.log('2 - inside');
    yield sleep(3000);
    console.log('3 - slept');
  })();
  console.log('4 - done?');
}

test();

Your output will be:

1
2 - inside
4 - done?
undefined
> 3 - slept

Thanks again for the time you spent on this, @karimsa! I do appreciate it, but unfortunately I can't merge this at this time. 💔 😿

I have an idea for how to approach this in the near-future. I'll ping you at such time for a second set of eyes.

karimsa commented 7 years ago

Sounds good. I'll be closing my fork for now.