[MRESOLVER-283] Shared executor service by cstamas · Pull Request #213 · apache/maven-resolver

cstamas · 2022-11-02T14:20:47Z

More and more component in resolver does parallel processing (BF collector, MD resolver, basic connector), and they all create, maintain their own executor instance.

Instead of this, create one shared service component and just reuse it accross resolver.

https://issues.apache.org/jira/browse/MRESOLVER-283

cstamas · 2022-11-02T14:32:46Z

@caiwei-ebay ping

caiwei-ebay · 2022-11-03T00:48:31Z

@cstamas

The PR looks good to me expect a few concerns as shown in comments. Thanks for the quick fix.
My main concern is about how to tell user pom/metadata.xml is downloaded in parallel now with BF collector.

As the special param "aether.dependencyCollector.bf.threads" has to be removed using SharedExecutor, could you put some lines to explain the parallel download behavior in param "aether.dependencyCollector.impl" where you've introduced BF and DF. With such information, user may try the BF and feedback so we can discuss whether BF could be the default based on the feedbacks.

caiwei-ebay · 2022-11-03T04:12:02Z

-        {
-            command.run();
-        }
-


Given this being deleted, I think it may introduce additional cost for DF.
DF downloads pom one by one, originally it will be downloaded by main thread directly and now it will be downloaded in a thread and the main thread has to wait

reintroduced direct strategy as well, two out of 3 users of resolver executor now relies on it.

caiwei-ebay · 2022-11-03T04:37:50Z

        {
            results.computeIfAbsent( ArtifactIdUtils.toId( artifact ),
-                    key -> this.executorService.submit( callable ) );
+                    key -> this.executor.submit( callable ) );


In the illustration below:

https://camo.githubusercontent.com/71a9619274e478245df1c37423239b79f6b3036a3af72690d7ace08419ee21ac/68747470733a2f2f6973737565732e6170616368652e6f72672f6a6972612f7365637572652f6174746163686d656e742f31333034373931392f7265736f6c76655f646570732e706e67

As we submit a Callable here, and this callable finally resorts to RepositoryConnector which will then submit a Collection (a collection with single artifact) to the executor, 2 different tasks running in one executor and besides the 2 tasks has dependencies.

The original behavior is:

we submit a Callable here, and this callable finally resorts to RepositoryConnector which will then submit a Collection (a collection with single artifact) to the executor, but this time the executor is actually a DirectExecutor which actually runs in main thread. Please check code below.

if ( maxThreads <= 1 )
{
return DirectExecutor.INSTANCE;
}
int tasks = safe( artifacts ).size() + safe( metadatas ).size();
if ( tasks <= 1 )
{
return DirectExecutor.INSTANCE;
}

Hopefully I restored the original behaviour, please recheck (latest commit)

caiwei-ebay · 2022-11-03T13:21:09Z

+    @Override
+    public void submitOrDirect( Collection<Runnable> tasks )
+    {
+        requireNonNull( tasks );


When resolves a snapshot dependency or dependency with version range, Maven resolves metadata.xml from multiple repositories in parallel: https://github.com/apache/maven/blob/master/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java#L158

With original design, metadata.xml is using a separate pool, pom or jar can use RepositoryConnector's pool but pom downloading is in serial while jar is in parallel:

metadata.xml use a separate pool to download metadata.xml from multiple repositories, each downloading calls RepositoryConnector's get(null, Collections.singleList(MetadataDownload)), it actually uses DirectExecutor without leveraging RepositoryConnector's thread pool;

pom downloading directly calls RepositoryConnector's get(Collections.singleList(ArtifactDownload), null), so it also uses DirectExecutor without leveraging RepositoryConnector's thread pool;

jar downloading calls RepositoryConnector's get(List(ArtifactDownload), null), it do leverage RepositoryConnector's thread pool.

With this approach, metadata, pom and jar all share one pool:

Suppose we have multiple repositories like releases, snapshots, commercial, legacy, testing, etc. in our private Maven nexus repository and we need to resolve 5 dependencies (snapshot or with version range) in parallel.

When resolve with BFDependencyCollector, 5 tasks to resolve each dependency have used up all 5 threads, and due to resolving each dependency (let's name with task A) need to resolve metadata (task B), the metadata resolution task B will be also submitted to the same pool. Here A depends on B but B has to wait available threads. I think this could be lead to endless waiting, right?

As it is better to use separate thread pool for separate tasks, maybe the SharedExecutor here should have separate pool for separate tasks? At least, pom/jar downloading and BF collect (it also need to download pom but pom downloading actually uses directlyExecute with your recent fix, thus safe to use the same pool) can use one pool and metadata.xml downloading still use another pool? Please advice.

I'd like also add as I understand the artifact thread pools where per repo, one this is one per all repos?

I think we need a overview what executor services where created previously and which now to compare whether this could create a bottleneck.

Bottleneck... or deadlock caused by thread exhaustion.

cstamas · 2022-11-03T16:07:44Z

Reworked to:

have split pools (service can decide using service "key")
pools are stored in session, so each session has own pool(s)
this merely reflects what was before, but all is centralized.

caiwei-ebay

The PR looks great to me. Thanks for tidy up the design.

michael-o · 2022-11-04T07:40:20Z

Will try to review today

michael-o · 2022-11-06T13:04:28Z

        List<ChecksumAlgorithmFactory> checksumAlgorithmFactories = layout.getChecksumAlgorithmFactories();
+        Collection<? extends MetadataDownload> mds = safe( metadataDownloads );
+        Collection<? extends ArtifactDownload> ads = safe( artifactDownloads );
+        ArrayList<Runnable> runnable = new ArrayList<>( mds.size() + ads.size() );


michael-o · 2022-11-06T13:09:42Z

        this.providedChecksumsSources = providedChecksumsSources;
+        this.resolverExecutor = resolverExecutorService.getResolverExecutor( session, RepositoryConnector.class,
+                ConfigUtils.getInteger( session, CONFIG_PROP_THREADS_DEFAULT, CONFIG_PROP_THREADS,
+                        "maven.artifact.threads" ) );


We should also deprecate this property since it does not reflect reality. MD is not artifacts, but this connector does both.

Maybe we should just remove it? We are going for 1.9 after all....

michael-o · 2022-11-06T13:14:41Z

-            Executor executor = getExecutor( Math.min( tasks.size(), threads ) );
-            try
+            RunnableErrorForwarder errorForwarder = new RunnableErrorForwarder();
+            ArrayList<Runnable> runnable = new ArrayList<>( tasks.size() );


gnodet

Sharing executors must be done with caution.

Is there any way that the executor may be running out of threads to service a call ? For example, resolving multiple files in parallel, each file being resolved in its own thread, then downloading dependencies and running out of threads ?

I think using a more general ForkJoinPool which supports work stealing would be safer to avoid running into such problems.

More and more component in resolver does parallel processing (BF collector, MD resolver, basic connector), and they all create, maintain their own executor instance. Instead of this, create one shared service component and just reuse it accross resolver. https://issues.apache.org/jira/browse/MRESOLVER-283

cstamas · 2022-11-08T09:40:31Z

Sharing executors must be done with caution.

Is there any way that the executor may be running out of threads to service a call ? For example, resolving multiple files in parallel, each file being resolved in its own thread, then downloading dependencies and running out of threads ?

I think using a more general ForkJoinPool which supports work stealing would be safer to avoid running into such problems.

I kinda agree with @gnodet here, and will "tone down" this PR to NOT SHARE executors, merely will introduce this facade but instances will be handled as before (as even now, it is not 100% what it was before: collector had per arg instance, now shared). And I cannot certainly asnwer the question "Is there any way that the executor may be running out of threads to service a call".

OTOH, I disagree with forkjoinpool: all these threads will do (probably blocking, maybe inifinite) HTTP IO, not to mention the refactoring needed to use them (most of "client" code remained unchanged here, using plain Runnable and Callable).

Hence, this PR will change to merely "centralize executor creation and (probably) use ResolverExecutor shim instead directly exposing ExecutorService iface"...

gnodet · 2022-11-08T10:19:40Z

OTOH, I disagree with forkjoinpool: all these threads will do (probably blocking, maybe inifinite) HTTP IO, not to mention the refactoring needed to use them (most of "client" code remained unchanged here, using plain Runnable and Callable).

ForkJoinPool is capable of handling blocking operations, just like a standard ThreadPoolExecutor. The only problem is when you don't use your own instance and reuse the system provided instance with blocking operations. The only difference between a ForkJoinPool and a ThreadPoolExecutor is that the former has more features such as thread affinity, work stealing and fork/join operations.

I think a single thread pool is a good idea, but it has to be implemented in a correct way to avoid any thread exhaustion. I.e. use a single fork/join pool and use the appropriate methods to dispatch all the work, which should ensure proper ordering of tasks execution. I've done a similar thing in apache/maven#803 (which I need to finish).

cstamas · 2022-11-08T10:35:07Z

Ok, now I feel I should not push this anymore, as I agree now with use of forkJoinPool. Hence, will close this PR, as that change would be bigger, but I want 1.9.0 out as soon as possible....

jira-importer · 2025-06-13T08:07:30Z

Resolve #959

jira-importer · 2025-06-13T08:39:03Z

Resolve #959

cstamas self-assigned this Nov 2, 2022

cstamas changed the title ~~Shared executor~~ [MRESOLVER-283] Shared executor Nov 2, 2022

caiwei-ebay reviewed Nov 3, 2022

View reviewed changes

Comment thread ...impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutor.java Outdated

caiwei-ebay reviewed Nov 3, 2022

View reviewed changes

cstamas marked this pull request as ready for review November 3, 2022 08:36

cstamas requested review from caiwei-ebay and michael-o and removed request for caiwei-ebay November 3, 2022 08:46

caiwei-ebay reviewed Nov 3, 2022

View reviewed changes

cstamas requested a review from caiwei-ebay November 3, 2022 17:05

caiwei-ebay approved these changes Nov 4, 2022

View reviewed changes

cstamas changed the title ~~[MRESOLVER-283] Shared executor~~ [MRESOLVER-283] Shared executor service Nov 4, 2022

michael-o requested changes Nov 6, 2022

View reviewed changes

cstamas requested a review from michael-o November 7, 2022 10:11

gnodet requested changes Nov 7, 2022

View reviewed changes

cstamas force-pushed the shared-executors branch from 2abdff0 to fb96453 Compare November 8, 2022 09:32

cstamas added 2 commits November 8, 2022 11:14

Do not SHARE

d5f7403

runnable -> runnables

39970d0

cstamas closed this Nov 8, 2022

cstamas deleted the shared-executors branch November 8, 2022 10:35

XenoAmess mentioned this pull request Jun 16, 2025

Refine performance (2nd) #1492

Closed

8 tasks

Conversation

cstamas commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cstamas commented Nov 2, 2022

Uh oh!

caiwei-ebay commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

caiwei-ebay Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cstamas commented Nov 3, 2022

Uh oh!

caiwei-ebay left a comment

Choose a reason for hiding this comment

Uh oh!

michael-o commented Nov 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

cstamas commented Nov 8, 2022

Uh oh!

gnodet commented Nov 8, 2022

Uh oh!

cstamas commented Nov 8, 2022

Uh oh!

jira-importer commented Jun 13, 2025

Uh oh!

jira-importer commented Jun 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cstamas commented Nov 2, 2022 •

edited

Loading

caiwei-ebay commented Nov 3, 2022 •

edited

Loading

caiwei-ebay Nov 3, 2022 •

edited

Loading