[GitHub] [maven-resolver] suztomo commented on a change in pull request #39: [MRESOLVER-93] - PathRecordingDependencyVisitor to handle 3 cycles
suztomo commented on a change in pull request #39: [MRESOLVER-93] -
PathRecordingDependencyVisitor to handle 3 cycles
URL: https://github.com/apache/maven-resolver/pull/39#discussion_r316202790
##
File path:
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/visitor/PathRecordingDependencyVisitor.java
##
@@ -101,6 +96,7 @@ public boolean visitEnter( DependencyNode node )
{
boolean accept = filter == null || filter.accept( node, parents );
+boolean hasDuplicateNodeInParent = parents.contains( node );
Review comment:
@belingueres In PR summary, I just added explanation on why I now think
`parent.contains(node)` performance should not be a problem.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-resolver] suztomo commented on a change in pull request #39: [MRESOLVER-93] - PathRecordingDependencyVisitor to handle 3 cycles
suztomo commented on a change in pull request #39: [MRESOLVER-93] -
PathRecordingDependencyVisitor to handle 3 cycles
URL: https://github.com/apache/maven-resolver/pull/39#discussion_r316199100
##
File path:
maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/visitor/PathRecordingDependencyVisitorTest.java
##
@@ -141,12 +141,12 @@ public void testGetPaths_HandlesCycles_threePaths()
{
DependencyNode root = parse( "cycle-3paths.txt" );
-PathRecordingDependencyVisitor visitor = new
PathRecordingDependencyVisitor( new ArtifactMatcher(), false );
+PathRecordingDependencyVisitor visitor = new
PathRecordingDependencyVisitor( new ArtifactMatcher() );
root.accept( visitor );
List> paths = visitor.getPaths();
-assertEquals( paths.toString(), 4, paths.size() );
-assertPath( paths.get( 0 ), "a", "b", "c");
+assertEquals( paths.toString(), 1, paths.size() );
Review comment:
Thank you for confirmation.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-resolver] suztomo commented on a change in pull request #39: [MRESOLVER-93] - PathRecordingDependencyVisitor to handle 3 cycles
suztomo commented on a change in pull request #39: [MRESOLVER-93] - PathRecordingDependencyVisitor to handle 3 cycles URL: https://github.com/apache/maven-resolver/pull/39#discussion_r316157438 ## File path: maven-resolver-util/src/test/resources/visitor/path-recorder/cycle-3paths.txt ## @@ -0,0 +1,7 @@ +gid:a:1 (1) ++- gid:b:0 +| \- ^1 ++- gid:b:1 +| \- ^1 +\- match:b:2 + \- ^1 Review comment: This dependency graph is similar setup as my example Maven project reproducing StackOverflowError. https://github.com/suztomo/maven-cyclic-dependency-with-range This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [maven-resolver] suztomo commented on a change in pull request #39: [MRESOLVER-93] - PathRecordingDependencyVisitor to handle 3 cycles
suztomo commented on a change in pull request #39: [MRESOLVER-93] -
PathRecordingDependencyVisitor to handle 3 cycles
URL: https://github.com/apache/maven-resolver/pull/39#discussion_r316134016
##
File path:
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/visitor/PathRecordingDependencyVisitor.java
##
@@ -101,6 +96,7 @@ public boolean visitEnter( DependencyNode node )
{
boolean accept = filter == null || filter.accept( node, parents );
+boolean hasDuplicateNodeInParent = parents.contains( node );
Review comment:
> The reason is that the same dependency should not be introspected together
with its dependencies if this process was done in some previous visit.
I agree that visited set was _supposed_ to do the job. However, when it
deals with 3 or more cycles, it suffers from StackOverflowError because of
`visited.remove( node )`.
> The limitation of this impl is the identity-base
Even if the visited set becomes `equal`-based set and `DefaultDependencyNode
` has proper `equals` and `hashCode`, I think the StackOverflowError remains.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-resolver] suztomo commented on a change in pull request #39: [MRESOLVER-93] - PathRecordingDependencyVisitor to handle 3 cycles
suztomo commented on a change in pull request #39: [MRESOLVER-93] -
PathRecordingDependencyVisitor to handle 3 cycles
URL: https://github.com/apache/maven-resolver/pull/39#discussion_r316131184
##
File path:
maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/visitor/PathRecordingDependencyVisitorTest.java
##
@@ -141,12 +141,12 @@ public void testGetPaths_HandlesCycles_threePaths()
{
DependencyNode root = parse( "cycle-3paths.txt" );
-PathRecordingDependencyVisitor visitor = new
PathRecordingDependencyVisitor( new ArtifactMatcher(), false );
+PathRecordingDependencyVisitor visitor = new
PathRecordingDependencyVisitor( new ArtifactMatcher() );
root.accept( visitor );
List> paths = visitor.getPaths();
-assertEquals( paths.toString(), 4, paths.size() );
-assertPath( paths.get( 0 ), "a", "b", "c");
+assertEquals( paths.toString(), 1, paths.size() );
Review comment:
> Changed unit test indicates modified behavior of the impl unexpectedly.
This unit test takes a new input `cycle-3paths.txt`, thus the expected
output is different than the one above. The unit test above
(`testGetPaths_HandlesCycles` with `cycle.txt`) is unchanged.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-resolver] suztomo commented on a change in pull request #39: [MRESOLVER-93] - PathRecordingDependencyVisitor to handle 3 cycles
suztomo commented on a change in pull request #39: [MRESOLVER-93] -
PathRecordingDependencyVisitor to handle 3 cycles
URL: https://github.com/apache/maven-resolver/pull/39#discussion_r315974988
##
File path:
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/visitor/PathRecordingDependencyVisitor.java
##
@@ -101,6 +96,7 @@ public boolean visitEnter( DependencyNode node )
{
boolean accept = filter == null || filter.accept( node, parents );
+boolean hasDuplicateNodeInParent = parents.contains( node );
Review comment:
Thank you for the comment. That approach of using visited “set” was the
cause of this bug.
Multiset (probably via IdentityHashMap) would solve
the problem and performance. However, the resulting code would be more complex
than current solution. Until we know there is a performance problem in this
class, I would choose simpler solution.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
