Fixes MRESOLVER-7, third try #30

hwellmann · 2019-03-23T16:13:40Z

Rebased PR #10 on master and added a small change which reverts dependency management to its former somewhat broken state, see MRESOLVER-9.

This fixes failing tests in the maven and maven-integration-testing projects.

Dependencies are now processed asynchronously by an Executor Cherry-pick commit from PR#2 Nov 1, 2016

This restores legacy behaviour, but means that dependency management is broken, as noted in MRESOLVER-9.

Indicates the number of threads used for parallel resolution of artifact descriptors. Default is 5. Set to 1 to revert to serial resolution. maven.artifact.descriptor.threads can be used as alias.

slachiewicz · 2019-03-25T14:35:05Z

During verification, I found that one change is being rolled out - a245b56 I do not think it was intentional.
You can also freely squash my commit with WorkerThreadFactory

...which got lost in rebasing this branch

hwellmann · 2019-03-25T20:47:42Z

I've reintroduced the change made in a245b56 that got lost during rebase - thanks for spotting this!

What exactly did you mean regarding the "commit with WorkerThreadFactory"?

michael-o · 2019-03-30T18:28:56Z

...-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DefaultDependencyCollector.java

@@ -666,46 +679,4 @@ private static boolean isLackingDescriptor( Artifact artifact )
        }
        return repositories;
    }
-
-    private static List<? extends Version> filterVersions( Dependency dependency, VersionRangeResult rangeResult,


Why has this method been removed in this commit?

It has been moved to DependencyCollectionUtils.

michael-o · 2019-03-30T18:31:48Z

...-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DefaultDependencyCollector.java

@@ -89,6 +90,10 @@

    static final String CONFIG_PROP_MAX_CYCLES = "aether.dependencyCollector.maxCycles";

+    private static final String CONFIG_PROP_NUM_ARTIFACT_DESCRIPTOR_THREADS = "aether.artifact.descriptor.threads";
+    private static final String CONFIG_PROP_NUM_ARTIFACT_DESCRIPTOR_THREADS_ALT = "maven.artifact.descriptor.threads";


I don't see a need for an alternative. As soon as the package names will change we will likely rename properties too.

Ok, removed.

michael-o · 2019-03-30T18:33:32Z

...solver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/PremanagedDependency.java

@@ -35,18 +42,39 @@

    final Boolean premanagedOptional;

+    /**
+     * @since 1.1.0


Please change to 1.4.0.

michael-o · 2019-03-30T18:33:37Z

...solver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/PremanagedDependency.java

+    final Collection<Exclusion> premanagedExclusions;
+
+    /**
+     * @since 1.1.0


Please change to 1.4.0.

michael-o

It looks good in general. Please take care of the comments. I will test against Core ITs. If everything is fine, I will merge for 1.4.0.

michael-o · 2019-03-30T20:25:10Z

All ITs pass for me. As soon as the requested changes are performed I will merge into master.

michael-o · 2019-03-31T19:50:48Z

I have now pushed a branch with minimal changes. Everyone's invited to have look. If I hear no objections, I'll merge in a couple of days.

nilson-aguiar · 2019-06-13T11:34:18Z

@michael-o this is not going to be merged?

michael-o · 2019-06-13T13:03:50Z

??? This has been merged two months ago.

nilson-aguiar · 2019-06-13T16:37:50Z

Sorry then, saw it as closed instead of merged then i asked. Thanks for the reply.

The parallel download of POMs [1] introduced in version 1.4.0 seems to cause probelms with our "jgnash-core dependencies are detected correctly" test in "MavenTest". To reproduce, do $ rm -fr ~/.m2/repository/org/apache/poi/poi-ooxml-schemas/3.14/ $ ./gradlew :analyzer:funTest --tests com.here.ort.analyzer.managers.MavenTest [1] apache/maven-resolver#30 Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>

The parallel download of POMs [1] introduced in version 1.4.0 seems to cause probelms with our "jgnash-core dependencies are detected correctly" test in "MavenTest". To reproduce, do $ rm -fr ~/.m2/repository/org/apache/poi/poi-ooxml-schemas/3.14/ $ ./gradlew :analyzer:funTest --tests com.here.ort.analyzer.managers.MavenTest which makes the test fail as none of the dependencies of "poi-ooxml-schemas" are found. Interestingly, at the time ORT creates the dependency nodes, the POM for "poi-ooxml-schemas" is not downloaded yet, but it *does* get downloaded afterwards. As a quick solution to the problem, simply revert to the most recent version of maven-resolver that does not include the parallel download of POMs. Later, we should probably inspect whether this is a user error on our side, or a bug in maven-resolver. [1] apache/maven-resolver#30 Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>

The parallel download of POMs [1] introduced in version 1.4.0 seems to cause problems with our "jgnash-core dependencies are detected correctly" test in "MavenTest". To reproduce, do $ rm -fr ~/.m2/repository/org/apache/poi/poi-ooxml-schemas/3.14/ $ ./gradlew :analyzer:funTest --tests com.here.ort.analyzer.managers.MavenTest which makes the test fail as none of the dependencies of "poi-ooxml-schemas" are found. Interestingly, at the time ORT creates the dependency nodes, the POM for "poi-ooxml-schemas" is not downloaded yet, but it *does* get downloaded afterwards. As a quick solution to the problem, simply revert to the most recent version of maven-resolver that does not include the parallel download of POMs. Later, we should probably inspect whether this is a user error on our side, or a bug in maven-resolver. [1] apache/maven-resolver#30 Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>

sschuberth · 2020-01-15T10:42:26Z

Does this new feature imply some API usage changes? Because in our OSS Review Toolkit (which analyzes project dependencies) we actually see some POMs only being downloaded after we expect them to be present. Downgrading to version 1.3.3 resolves the issue. For details please see oss-review-toolkit/ort#2113.

michael-o · 2020-01-15T11:53:27Z

@sschuberth This has been reverted because it suffered from race conditions.

sschuberth · 2020-01-15T11:58:36Z

Thanks @michael-o, I was just reading about that in MRESOLVER-92 which is mentioned in the 1.4.1 release announcement. However, interestingly the issue we're seeing occurs with both versions 1.4.0 and 1.4.1. So maybe it's something that's not caused by the parallel download of POMs after all.

michael-o · 2020-01-15T13:05:02Z

@sschuberth If you see some other error, please report.

sschuberth · 2020-01-15T14:37:47Z

@michael-o, in our case, the root cause turned out to be a side effect of https://www.alphabot.com/security/blog/2020/java/Your-Java-builds-might-break-starting-January-13th.html.

Eskibear · 2020-02-26T15:28:14Z

Any update? Will you guys re-enable this improvement to download pom files in parallel?

michael-o · 2020-02-26T19:37:02Z

Any update? Will you guys re-enable this improvement to download pom files in parallel?

No, unless someone is willing to work on the race conditions.

Eskibear · 2020-03-05T03:37:57Z

All I can find about the race conditions is @Tibor17 's comments in MRESOLVER-7 . And it seems that details were discussed in the ASF slack but it is internal. So it might be hard for someone to work on it without knowing the details.

@hwellmann @slachiewicz May I ask that, are you still working on the fix, or some other ASF guys will take it over?

michael-o · 2020-03-05T07:56:34Z

@Eskibear, no, no one is working on it. We prefer stability over performance.

Eskibear · 2020-03-05T11:54:37Z

@michael-o thanks for the information. BTW, could you share some details (or point out the link) about the failed cases, race conditions mentioned above. My point is, if someone from community is willing to fix that, your previous discussion would help a lot.

michael-o · 2020-03-05T13:06:37Z

Basically, we have frequent race conditions in the Maven ITs. @Tibor17 Do you remember the discussion the mailing list? I guess it was on Slack only :-(

@Eskibear. I think you can reproduce it yourself. Fetch the first broken Resolver release and run Maven ITs multiple times. I think you should see failures not related to Maven itself.

Tibor17 · 2020-05-27T08:55:59Z

@michael-o
@Eskibear
Hello guys. You'r right! We should restart the work.
Can we start again during the weekend?

Eskibear · 2020-06-15T02:16:53Z

Any news or follow-ups?

michael-o · 2020-06-16T09:21:15Z

@Eskibear, since we have other serious issues with concurency this isn't happening soon.

Tibor17 · 2020-06-16T21:03:11Z

@michael-o @Eskibear
If you have time these days or weeks we can start analysing the code @hwellmann was working on.
We should define some order of tasks.
Lets start with the first analysis where we will find the parts where the downloads can be made parallel.

michael-o · 2020-06-16T21:05:37Z

@michael-o No, I won't. Looking at https://issues.apache.org/jira/browse/MRESOLVER-114 will make things worse because we do not have decent concurrency support. Even if we solve this one, the synchronization will trick us anyway.

Eskibear · 2020-06-17T06:02:23Z

@michael-o I just glanced at MRESOLVER-114, and I don't think it's related with MRESOLVER-7.
Learning for the comments, MRESOLVER-114 is about resolving artifacts in parallel.
While here in MRESOLVER-7, the problem is, when the list of dependencies is already known, it performs downloading of the corresponding pom files in a single thread. Given you guys have more context information, correct me if I'm wrong.
What I see is, networking is the bottleneck. It would be a great performance boost even if we just make the downloading parallel.

Lets start with the first analysis where we will find the parts where the downloads can be made parallel.

@Tibor17 Agree with you that we should do some first analysis. Below is what I find in master branch, where downloading dependencies is mono-threaded. As far as I learn, basic idea of this PR was to make below logic parallel.

maven-resolver/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DefaultDependencyCollector.java

Lines 347 to 351 in 86c1cb2

    
           for ( Dependency dependency : dependencies ) 
        
           { 
        
               processDependency( args, results, repositories, depSelector, depManager, depTraverser, verFilter, 
        
                                  dependency ); 
        
           }

michael-o · 2020-06-17T12:05:47Z

Please move discussion to the ticket because this PR has been closed.

Tibor17 · 2020-06-17T18:50:51Z

@Eskibear
We have to find the place where the files are physically downloaded, because it is the place which should run in parallel. The caller methods should not go to the thread pool because Maven's implementations of objects are not thread safe. I do not want to make al of them immutable. And why to call the Java mess in forked thread. It is not a performance penalty. The penatly we pay is the physical download.

Tibor17 · 2020-06-17T19:04:57Z

@Eskibear
If I am right, here is one of the low level places. I do not like shared objects on instance and here is the shared state.

michael-o · 2020-06-17T20:08:28Z

Maven does not use the HTTP Transporter. We use Resolver via Wagon only.

hwellmann and others added 3 commits March 22, 2019 17:43

[MRESOLVER-7] Download dependency POMs in parallel

38a0c20

Dependencies are now processed asynchronously by an Executor Cherry-pick commit from PR#2 Nov 1, 2016

[MRESOLVER-7] Use WorkerThreadFactory to create daemon threads

16a5558

[MRESOLVER-7] select dependencies based on pre-managed state

884928f

This restores legacy behaviour, but means that dependency management is broken, as noted in MRESOLVER-9.

hwellmann force-pushed the MRESOLVER-7v3 branch from 3a537f9 to 884928f Compare March 23, 2019 16:17

hwellmann mentioned this pull request Mar 23, 2019

[MRESOLVER-7] Download dependency POMs in parallel v2 #10

Closed

hwellmann added 2 commits March 24, 2019 10:19

[MRESOLVER-7] deleted redundant code

17b369b

[MRESOLVER-7] added config property aether.artifact.descriptor.threads

b038088

Indicates the number of threads used for parallel resolution of artifact descriptors. Default is 5. Set to 1 to revert to serial resolution. maven.artifact.descriptor.threads can be used as alias.

[MRESOLVER-7] re-added change of a245b56

8caf87e

...which got lost in rebasing this branch

michael-o reviewed Mar 30, 2019

View reviewed changes

michael-o requested changes Mar 30, 2019

View reviewed changes

[MRESOLVER-7] cleanup after review

caa8bc3

michael-o approved these changes Mar 31, 2019

View reviewed changes

asfgit closed this in 9d3b324 Apr 12, 2019

sschuberth mentioned this pull request Jan 15, 2020

Gradle: Downgrade the maven-resolver to version 1.3.3 again oss-review-toolkit/ort#2113

Closed

Eskibear mentioned this pull request Mar 31, 2023

update default vmargs of jdtls to resolve dependencies in parallel redhat-developer/vscode-java#3030

Merged

Fixes MRESOLVER-7, third try #30

Fixes MRESOLVER-7, third try #30

Conversation

hwellmann commented Mar 23, 2019

slachiewicz commented Mar 25, 2019

hwellmann commented Mar 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-o left a comment

Choose a reason for hiding this comment

michael-o commented Mar 30, 2019

michael-o commented Mar 31, 2019

nilson-aguiar commented Jun 13, 2019

michael-o commented Jun 13, 2019

nilson-aguiar commented Jun 13, 2019

sschuberth commented Jan 15, 2020

michael-o commented Jan 15, 2020

sschuberth commented Jan 15, 2020

michael-o commented Jan 15, 2020

sschuberth commented Jan 15, 2020

Eskibear commented Feb 26, 2020

michael-o commented Feb 26, 2020

Eskibear commented Mar 5, 2020

michael-o commented Mar 5, 2020

Eskibear commented Mar 5, 2020

michael-o commented Mar 5, 2020

Tibor17 commented May 27, 2020

Eskibear commented Jun 15, 2020

michael-o commented Jun 16, 2020

Tibor17 commented Jun 16, 2020

michael-o commented Jun 16, 2020

Eskibear commented Jun 17, 2020

michael-o commented Jun 17, 2020

Tibor17 commented Jun 17, 2020

Tibor17 commented Jun 17, 2020

michael-o commented Jun 17, 2020