Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nx affected infinite loop #2367

Closed
alfaproject opened this issue Jan 23, 2020 · 11 comments · Fixed by #2525
Closed

nx affected infinite loop #2367

alfaproject opened this issue Jan 23, 2020 · 11 comments · Fixed by #2525
Assignees
Labels

Comments

@alfaproject
Copy link
Contributor

Expected Behavior

nx affected command family completes

Current Behavior

nx affected command family does not complete (seems to go into an infinite loop)

Failure Information (for bugs)

There really isn't more that I can say. The affected commands just don't finish unless I use --all.

Steps to Reproduce

  1. ... in my project
  2. nx affected:lint
  3. ... never finishes

I can not share the project, but I'm ok to share the project graph:
project-graph.json.zip

I can also share any other metadata files like nx.json if it helps.

Context

  @nrwl/angular : 8.11.2
  @nrwl/cli : 8.11.2
  @nrwl/cypress : 8.11.2
  @nrwl/eslint-plugin-nx : Not Found
  @nrwl/express : Not Found
  @nrwl/jest : 8.11.2
  @nrwl/linter : Not Found
  @nrwl/nest : Not Found
  @nrwl/next : Not Found
  @nrwl/node : Not Found
  @nrwl/react : Not Found
  @nrwl/schematics : Not Found
  @nrwl/tao : 8.11.2
  @nrwl/web : Not Found
  @nrwl/workspace : 8.11.2
  typescript : 3.5.3
@alfaproject
Copy link
Contributor Author

I forgot to mention, but this stopped working on any version after 8.9.0 and these are related issues:

  1. affected: commands break in 8.10.0 #2274 (I had that issue as well when I upgraded to 8.10.0 and now I don't get that but it never finishes)
  2. affected error (Maximum call stack size exceeded) #1990 (I also had that issue which was fixed and now it looks like it came back)

@vsavkin
Copy link
Member

vsavkin commented Jan 26, 2020

@alfaproject could you give it a try with 8.11.2?

We rewrote how we compute the graph to make extensible, and missed a few things. Sorry about it.

@vsavkin vsavkin self-assigned this Jan 26, 2020
@vsavkin vsavkin added scope: core core nx functionality type: bug labels Jan 26, 2020
@alfaproject
Copy link
Contributor Author

@vsavkin yes, I've tried every version and all of them fail one way or another. This issue was reported using 8.11.2 as you can see in the description

@alfaproject
Copy link
Contributor Author

@vsavkin I have just tried 8.12.0 and I have the same issue ):

@alfaproject
Copy link
Contributor Author

@vsavkin this patch fixes this regression:

diff --git a/node_modules/@nrwl/workspace/src/core/affected-project-graph/affected-project-graph.js b/node_modules/@nrwl/workspace/src/core/affected-project-graph/affected-project-graph.js
index 11bab98..e3a2939 100644
--- a/node_modules/@nrwl/workspace/src/core/affected-project-graph/affected-project-graph.js
+++ b/node_modules/@nrwl/workspace/src/core/affected-project-graph/affected-project-graph.js
@@ -45,20 +45,19 @@ function addAffectedNodes(startingProject, reversed, builder, visited) {
     if (!reversed.nodes[startingProject]) {
         throw new Error(`Invalid project name is detected: "${startingProject}"`);
     }
+    visited.push(startingProject);
     builder.addNode(reversed.nodes[startingProject]);
     const ds = reversed.dependencies[startingProject];
     if (ds) {
-        ds.forEach(({ target }) => addAffectedNodes(target, reversed, builder, [...visited, startingProject]));
+        ds.forEach(({ target }) => addAffectedNodes(target, reversed, builder, visited));
     }
 }
 function addAffectedDependencies(startingProject, reversed, builder, visited) {
     if (visited.indexOf(startingProject) > -1)
         return;
+    visited.push(startingProject);
     if (reversed.dependencies[startingProject]) {
-        reversed.dependencies[startingProject].forEach(({ target }) => addAffectedDependencies(target, reversed, builder, [
-            ...visited,
-            startingProject
-        ]));
+        reversed.dependencies[startingProject].forEach(({ target }) => addAffectedDependencies(target, reversed, builder, visited));
         reversed.dependencies[startingProject].forEach(({ type, source, target }) => {
             // Since source and target was reversed,
             // we need to reverse it back to original direction.

@intellix
Copy link

@vsavkin I don't mind making the above PR if you can verify that it's the correct solution

@jaysoo
Copy link
Member

jaysoo commented Feb 19, 2020

@alfaproject @intellix It looks correct, and basically makes the visited array shared state. If you can open a PR we can take a look. Thanks!

@alfaproject
Copy link
Contributor Author

I’m in Egypt for the next 10 days and didn’t bring my laptop ):

@jaysoo
Copy link
Member

jaysoo commented Feb 21, 2020

I will be providing a PR for this today so we can have a fix in the next release.

FWIW, there wasn't an infinite loop but because we are creating a lot of new arrays + gc, the process became very CPU intensive.

jaysoo added a commit to jaysoo/nx that referenced this issue Feb 21, 2020
- Uses a shared mutable state between project traversals so we don't create new objects unnecessarily
- Closes nrwl#2367
@alfaproject
Copy link
Contributor Author

Thank you @jaysoo, much appreciated (:

vsavkin pushed a commit that referenced this issue Feb 21, 2020
- Uses a shared mutable state between project traversals so we don't create new objects unnecessarily
- Closes #2367
vsavkin pushed a commit that referenced this issue Feb 25, 2020
- Uses a shared mutable state between project traversals so we don't create new objects unnecessarily
- Closes #2367
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants