nrwl / nx

Smart Monorepos · Fast CI
https://nx.dev
MIT License
23.55k stars 2.35k forks source link

Circular dependencies are not caught when nodes have been added to dep graph via plugin. #7546

Closed vanbujm closed 2 years ago

vanbujm commented 3 years ago

Current Behavior

I got myself into a situation where I was adding nodes to the project graph via a plugin. ( I am working with java projects)

Consider this reverse dependencies graph:

{
    'PROJECT_C': [
      'PROJECT_D',
      'PROJECT_A'
    ],
    'PROJECT_D': [
      'PROJECT_C',
      'PROJECT_A'
    ],
    'PROJECT_B': [ 'PROJECT_A' ],
    'PROJECT_A': []
}

Project D and Project C have circular dependencies. As the TasksSchedule class goes through the list and completes tasks, it runs complete, which then runs removeTasksFromTaskGraph. removeTasksFromTaskGraph will finish the last task it can, but then it will queue up 0 new roots because:

Object.keys(dependencies).filter((k) => dependencies[k].length === 0);

evaluates to [] This causes everything to stop. The observer completes WITHOUT completing subscribe events and to the user, it looks like everything is done with success. However, the last two tasks were never run.

Expected Behavior

removeTasksFromTaskGraph should evaluate Object.keys(dependencies).filter((k) => dependencies[k].length === 0); Then realise that there are still dependencies but nothing it can add to root, and report and circular dependencies error to the user so that they can fix their deps.

Steps to Reproduce

Kinda hard. But create a plugin that creates a circular dependency and try and run a build task on it where:

"targetDependencies": {
    "build": [
      {
        "target": "build",
        "projects": "dependencies"
      }
    ],
}

Sorry, don't have a minimum example up yet. But I have isolated a location (removeTasksFromTaskGraph) Where you could add a check for circular deps.

Failure Logs

None. The bug does not cause any error. In fact, it reports a successful run to the user despite not completing all tasks.

vanbujm commented 3 years ago

I ended up needing to be able to resolve the circular dependency (in my case it didn't matter which project I ran first so it was safe to blindly process the next root even if it had deps).

I updated :

public scheduleNextTasks() {
    if (process.env.NX_BATCH_MODE === 'true') {
      this.scheduleBatches();
    }
    // eslint-disable-next-line prefer-const
    for (let root of this.notScheduledTaskGraph.roots) {
      if (this.canBeScheduled(root)) {
        this.scheduleTask(root);
      }
    }
    if (
      this.scheduledTasks.length === 0 &&
      this.notScheduledTaskGraph.roots.length !== 0
    ) {
      console.warn(
        chalk.red.bold('Circular dependency detected!'),
        chalk.red(JSON.stringify(this.notScheduledTaskGraph.roots, null, 2))
      );
      this.circularDepDetected = true;

      this.notScheduledTaskGraph.roots.forEach((root) => {
        this.scheduleTask(root);
      });
    }
  }

And

export function removeTasksFromTaskGraph(
  graph: TaskGraph,
  ids: string[]
): TaskGraph {
  const tasks = {};
  const dependencies = {};
  const removedSet = new Set(ids);
  for (const taskId of Object.keys(graph.tasks)) {
    if (!removedSet.has(taskId)) {
      tasks[taskId] = graph.tasks[taskId];
      dependencies[taskId] = graph.dependencies[taskId].filter(
        (depTaskId) => !removedSet.has(depTaskId)
      );
    }
  }

  const sortedDeps = Object.entries(
    dependencies as Record<string, string[]>
  ).reduce((acc, [project, deps]) => {
    if (!acc[deps.length]) {
      acc[deps.length] = [project];
    } else {
      acc[deps.length].push(project);
    }
    return acc;
  }, {} as Record<number, string[]>);

  const newDeps = Object.entries(sortedDeps).sort(
    (a, b) => Number(b[0]) - Number(a[0])
  );

  const roots = newDeps.length > 0 ? newDeps.pop()[1] : [];

  return {
    tasks,
    dependencies: dependencies,
    roots,
  };
}

Great work on this project. The fact that I can solve this type of problem by writing my own task-runner is a testament to how nicely the library is designed. ^_^

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it hasn't had any recent activity. It will be closed in 14 days if no further activity occurs. If we missed this issue please reply to keep it active. Thanks for being a part of the Nx community! 🙏

github-actions[bot] commented 1 year ago

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.