jakejs / jake

JavaScript build tool, similar to Make or Rake. Built to work with Node.js.
http://jakejs.com
Apache License 2.0
1.96k stars 190 forks source link

Concurrency and invoke() cause prerequisites to be skipped #396

Closed bno1 closed 3 years ago

bno1 commented 3 years ago

Description:

let { task, Task } = require('jake');

function timeout(ms) {
  return new Promise((resolve) => setTimeout(resolve, ms));
}

task('A', function () {
  return timeout(500).then(() => { console.log('A done'); });
});

task('B', function () {
  return timeout(500).then(() => { console.log('B done'); });
});

task('C', function () {
  return timeout(500).then(() => { console.log('C done'); });
});

task('ABC', ['A', 'B', 'C']);

task('foo', function () {
  Task['ABC'].invoke();
});

task('bar', function () {
  Task['ABC'].invoke();
});

task('buildAll', ['foo', 'bar'], { concurrency: 2 });

Output:

$ yarn exec -- jake buildAll
yarn exec v1.22.10
Starting 'foo'...
Starting 'bar'...
Finished 'bar' after 0 ms
Starting 'A'...
A done
Finished 'A' after 501 ms
Starting 'C'...
C done
Finished 'C' after 501 ms
Starting 'ABC'...
Finished 'ABC' after 0 ms
Finished 'foo' after 1104 ms
Starting 'buildAll'...
Finished 'buildAll' after 0 ms
Done in 1.18s.

Notice that the 'B' task is skipped.

I tried to debug the problem a little and found out that the nextPrereq method for ABC is being called twice in the same event loop iteration. It will start task A, but install 2 _done handlers for it. This causes handlePrereqDone to be called twice in the same event loop iteration, which increments _currentPrereqIndex twice.

mde commented 3 years ago

This is interesting. Actually not a use-case I would expect Jake to handle, considering the following:

It's clear what's happening here is the two concurrent tasks are fighting over control of the common (composite) task. This is the type of problem the FP/Immutability crowd love to talk about. JavaScript single-threadedness often saves us in the case of shared data, but here we have shared work to do.

Mixing concurrent and sequential can be really dicey, and in this case, the shared sequential tasks make things even worse. It might be possible to add more structure to Jake's runner so it handles this stuff more gracefully, but the cost would be a ton of complexity and new edge-cases which would likely be impossible to nail down completely.

In this case, the work could be easily restructured to support your use case:

let { task, Task } = require('jake');

function timeout(ms) {
  return new Promise((resolve) => setTimeout(resolve, ms));
}

task('A', function () {
  return timeout(500).then(() => { console.log('A done'); });
});

task('B', function () {
  return timeout(500).then(() => { console.log('B done'); });
});

task('C', function () {
  return timeout(500).then(() => { console.log('C done'); });
});

task('ABC', ['A', 'B', 'C']);

task('foo', function () {
  console.log('foo done');
});

task('bar', function () {
  console.log('bar done');
});

// Taken from the Jake docs, 'Evented tasks'
task('sequential', async function () {
  let t = Task['ABC'];
  return new Promise((resolve, reject) => {
    t.addListener('complete', () => {
      console.log('sequential done');
      resolve();
    });
    t.invoke();
  });
});

task('concurrent', ['foo', 'bar'], { concurrency: 2 });

task('combined', ['sequential', 'concurrent']);

We should probably rework invoke and execute to return Promises as well. That might make it less noisy than having to wrap the evented API for tasks to accomplish this.

In short: I think it makes more sense to restructure your workload here to work within Jake's limitations, rather than attempting to add more functionality to support various combinations of sequential/concurrent, and multiple levels of (share) prerequisites.

I will close this issue now, but please let me know if this solution does not work for you.