From 26c2c7e3b39ad05007c66afed7af1409e01087d8 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Mon, 27 Nov 2023 12:36:33 +0100 Subject: [PATCH 1/6] [MRESOLVER-321] Make collection interruptable Currently the two collection are not interruptable, does not sense interruption directly, only by some side-effect like IO. Moreover, the BF new collector may enter a "busy loop" as we seen, true, it was due bug, but nothing prevents us to have more bugs. Make main loop in both collector detect thread interruption and use global (per-collection) Args to carry state of the, interruption effectively the whole ST or MT collection. --- https://issues.apache.org/jira/browse/MRESOLVER-321 --- .../collect/DependencyCollectorDelegate.java | 7 ++++++- .../collect/bf/BfDependencyCollector.java | 19 +++++++++++++++++- .../collect/df/DfDependencyCollector.java | 19 +++++++++++++++++- ...ependencyCollectorDelegateTestSupport.java | 20 +++++++++++++++++++ 4 files changed, 62 insertions(+), 3 deletions(-) diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DependencyCollectorDelegate.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DependencyCollectorDelegate.java index 663364ac1..4105a5914 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DependencyCollectorDelegate.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DependencyCollectorDelegate.java @@ -281,7 +281,8 @@ protected abstract void doCollectDependencies( List repositories, List dependencies, List managedDependencies, - Results results); + Results results) + throws DependencyCollectionException; protected RepositorySystemSession optimizeSession(RepositorySystemSession session) { DefaultRepositorySystemSession optimized = new DefaultRepositorySystemSession(session); @@ -461,6 +462,10 @@ public Results(CollectResult result, RepositorySystemSession session) { maxCycles = ConfigUtils.getInteger(session, DEFAULT_MAX_CYCLES, CONFIG_PROP_MAX_CYCLES); } + public CollectResult getResult() { + return result; + } + public String getErrorPath() { return errorPath; } diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java index 42b78748c..9cc68e170 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java @@ -39,6 +39,7 @@ import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -48,6 +49,7 @@ import org.eclipse.aether.artifact.ArtifactType; import org.eclipse.aether.artifact.DefaultArtifact; import org.eclipse.aether.collection.CollectRequest; +import org.eclipse.aether.collection.DependencyCollectionException; import org.eclipse.aether.collection.DependencyManager; import org.eclipse.aether.collection.DependencySelector; import org.eclipse.aether.collection.DependencyTraverser; @@ -146,7 +148,8 @@ protected void doCollectDependencies( List repositories, List dependencies, List managedDependencies, - Results results) { + Results results) + throws DependencyCollectionException { boolean useSkip = ConfigUtils.getBoolean(session, DEFAULT_SKIPPER, CONFIG_PROP_SKIPPER); int nThreads = ExecutorUtils.threadCount(session, DEFAULT_THREADS, CONFIG_PROP_THREADS); logger.debug("Using thread pool with {} threads to resolve descriptors.", nThreads); @@ -200,6 +203,11 @@ protected void doCollectDependencies( processDependency( args, results, args.dependencyProcessingQueue.remove(), Collections.emptyList(), false); } + + if (args.interruptedException.get() != null) { + throw new DependencyCollectionException( + results.getResult(), "Collection interrupted", args.interruptedException.get()); + } } } @@ -210,6 +218,12 @@ private void processDependency( DependencyProcessingContext context, List relocations, boolean disableVersionManagement) { + if (Thread.interrupted()) { + args.interruptedException.set(new InterruptedException()); + } + if (args.interruptedException.get() != null) { + return; + } Dependency dependency = context.dependency; PremanagedDependency preManaged = context.premanagedDependency; @@ -603,6 +617,8 @@ static class Args { final ParallelDescriptorResolver resolver; + final AtomicReference interruptedException; + Args( RepositorySystemSession session, DataPool pool, @@ -620,6 +636,7 @@ static class Args { this.versionContext = versionContext; this.skipper = skipper; this.resolver = resolver; + this.interruptedException = new AtomicReference<>(null); } } } diff --git a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/df/DfDependencyCollector.java b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/df/DfDependencyCollector.java index 983a56551..abb9500b1 100644 --- a/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/df/DfDependencyCollector.java +++ b/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/df/DfDependencyCollector.java @@ -24,11 +24,13 @@ import java.util.Collections; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; import org.eclipse.aether.RepositorySystemSession; import org.eclipse.aether.RequestTrace; import org.eclipse.aether.artifact.Artifact; import org.eclipse.aether.collection.CollectRequest; +import org.eclipse.aether.collection.DependencyCollectionException; import org.eclipse.aether.collection.DependencyManager; import org.eclipse.aether.collection.DependencySelector; import org.eclipse.aether.collection.DependencyTraverser; @@ -88,7 +90,8 @@ protected void doCollectDependencies( List repositories, List dependencies, List managedDependencies, - Results results) { + Results results) + throws DependencyCollectionException { NodeStack nodes = new NodeStack(); nodes.push(node); @@ -110,6 +113,11 @@ protected void doCollectDependencies( ? session.getDependencyTraverser().deriveChildTraverser(context) : null, session.getVersionFilter() != null ? session.getVersionFilter().deriveChildFilter(context) : null); + + if (args.interruptedException.get() != null) { + throw new DependencyCollectionException( + results.getResult(), "Collection interrupted", args.interruptedException.get()); + } } @SuppressWarnings("checkstyle:parameternumber") @@ -123,6 +131,12 @@ private void process( DependencyManager depManager, DependencyTraverser depTraverser, VersionFilter verFilter) { + if (Thread.interrupted()) { + args.interruptedException.set(new InterruptedException()); + } + if (args.interruptedException.get() != null) { + return; + } for (Dependency dependency : dependencies) { processDependency( args, trace, results, repositories, depSelector, depManager, depTraverser, verFilter, dependency); @@ -401,6 +415,8 @@ static class Args { final CollectRequest request; + final AtomicReference interruptedException; + Args( RepositorySystemSession session, DataPool pool, @@ -416,6 +432,7 @@ static class Args { this.nodes = nodes; this.collectionContext = collectionContext; this.versionContext = versionContext; + this.interruptedException = new AtomicReference<>(null); } } } diff --git a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/DependencyCollectorDelegateTestSupport.java b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/DependencyCollectorDelegateTestSupport.java index 37116fdae..5959a5013 100644 --- a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/DependencyCollectorDelegateTestSupport.java +++ b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/DependencyCollectorDelegateTestSupport.java @@ -28,6 +28,7 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; import org.eclipse.aether.DefaultRepositorySystemSession; import org.eclipse.aether.RepositorySystemSession; @@ -158,6 +159,25 @@ protected DependencyNode path(DependencyNode root, int... coords) { } } + @Test + void testInterruption() throws Exception { + Dependency dependency = newDep("gid:aid:ext:ver", "compile"); + CollectRequest request = new CollectRequest(dependency, singletonList(repository)); + AtomicReference cause = new AtomicReference<>(null); + Thread t = new Thread(() -> { + Thread.currentThread().interrupt(); + try { + collector.collectDependencies(session, request); + fail("We should throw"); + } catch (DependencyCollectionException e) { + cause.set(e.getCause()); + } + }); + t.start(); + t.join(); + assertTrue(cause.get() instanceof InterruptedException, String.valueOf(cause.get())); + } + @Test void testSimpleCollection() throws DependencyCollectionException { Dependency dependency = newDep("gid:aid:ext:ver", "compile"); From ab2adb6be911f3364cd546f7a48497adbfb265b2 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Mon, 27 Nov 2023 12:52:32 +0100 Subject: [PATCH 2/6] Visitor made interruptible as well. --- .../aether/graph/DefaultDependencyNode.java | 3 + .../graph/DefaultDependencyNodeTest.java | 63 +++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java b/maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java index b746f7581..832e3037c 100644 --- a/maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java @@ -286,6 +286,9 @@ public void setData(Object key, Object value) { } public boolean accept(DependencyVisitor visitor) { + if (Thread.interrupted()) { + throw new RuntimeException("Thread interrupted"); + } if (visitor.visitEnter(this)) { for (DependencyNode child : children) { if (!child.accept(visitor)) { diff --git a/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java b/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java new file mode 100644 index 000000000..b412a3dc6 --- /dev/null +++ b/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java @@ -0,0 +1,63 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.eclipse.aether.graph; + +import java.util.concurrent.atomic.AtomicReference; + +import org.eclipse.aether.artifact.DefaultArtifact; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +/** + */ +public class DefaultDependencyNodeTest { + @Test + void testVisitorInterrupt() throws Exception { + DefaultDependencyNode node = + new DefaultDependencyNode(new Dependency(new DefaultArtifact("gid:aid:ver"), "compile")); + // we just use dummy visitor, as it is not visiting that matters + DependencyVisitor visitor = new DependencyVisitor() { + @Override + public boolean visitEnter(DependencyNode node) { + return true; + } + + @Override + public boolean visitLeave(DependencyNode node) { + return true; + } + }; + AtomicReference thrown = new AtomicReference<>(null); + Thread t = new Thread(() -> { + Thread.currentThread().interrupt(); + try { + node.accept(visitor); + fail("Should fail"); + } catch (Exception e) { + thrown.set(e); + } + }); + t.start(); + t.join(); + + assertTrue(thrown.get() instanceof RuntimeException, String.valueOf(thrown.get())); + } +} From 28db64e779166470f9759b385bbcce5defbd40e1 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Mon, 27 Nov 2023 12:54:00 +0100 Subject: [PATCH 3/6] Visitor should NOT reset the interrupted flag, just interrupt the work, and let that be handled somewhere else (ie. collector) --- .../java/org/eclipse/aether/graph/DefaultDependencyNode.java | 2 +- .../org/eclipse/aether/graph/DefaultDependencyNodeTest.java | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java b/maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java index 832e3037c..aa3e73574 100644 --- a/maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java @@ -286,7 +286,7 @@ public void setData(Object key, Object value) { } public boolean accept(DependencyVisitor visitor) { - if (Thread.interrupted()) { + if (Thread.currentThread().isInterrupted()) { throw new RuntimeException("Thread interrupted"); } if (visitor.visitEnter(this)) { diff --git a/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java b/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java index b412a3dc6..236bec4f0 100644 --- a/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java +++ b/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java @@ -59,5 +59,6 @@ public boolean visitLeave(DependencyNode node) { t.join(); assertTrue(thrown.get() instanceof RuntimeException, String.valueOf(thrown.get())); + assertTrue(t.isInterrupted()); } } From b3032ff3deebbb66bccb2849c55f7379a18d4812 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Mon, 27 Nov 2023 13:47:27 +0100 Subject: [PATCH 4/6] Give cause for RTEx --- .../java/org/eclipse/aether/graph/DefaultDependencyNode.java | 2 +- .../org/eclipse/aether/graph/DefaultDependencyNodeTest.java | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java b/maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java index aa3e73574..4120f49f4 100644 --- a/maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java +++ b/maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java @@ -287,7 +287,7 @@ public void setData(Object key, Object value) { public boolean accept(DependencyVisitor visitor) { if (Thread.currentThread().isInterrupted()) { - throw new RuntimeException("Thread interrupted"); + throw new RuntimeException(new InterruptedException("Thread interrupted")); } if (visitor.visitEnter(this)) { for (DependencyNode child : children) { diff --git a/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java b/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java index 236bec4f0..54688444c 100644 --- a/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java +++ b/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java @@ -59,6 +59,7 @@ public boolean visitLeave(DependencyNode node) { t.join(); assertTrue(thrown.get() instanceof RuntimeException, String.valueOf(thrown.get())); + assertTrue(( (RuntimeException) thrown.get() ).getCause() instanceof InterruptedException, String.valueOf(thrown.get())); assertTrue(t.isInterrupted()); } } From cc776159d20bb34d2570f528aa14279ed8c04e76 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Mon, 27 Nov 2023 13:48:18 +0100 Subject: [PATCH 5/6] Reformat --- .../org/eclipse/aether/graph/DefaultDependencyNodeTest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java b/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java index 54688444c..4f8cebc5f 100644 --- a/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java +++ b/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java @@ -59,7 +59,9 @@ public boolean visitLeave(DependencyNode node) { t.join(); assertTrue(thrown.get() instanceof RuntimeException, String.valueOf(thrown.get())); - assertTrue(( (RuntimeException) thrown.get() ).getCause() instanceof InterruptedException, String.valueOf(thrown.get())); + assertTrue( + ((RuntimeException) thrown.get()).getCause() instanceof InterruptedException, + String.valueOf(thrown.get())); assertTrue(t.isInterrupted()); } } From 93c2029b7c0130381e94ca8eaf7ba1920486076f Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Mon, 27 Nov 2023 13:49:28 +0100 Subject: [PATCH 6/6] Simplify UT --- .../org/eclipse/aether/graph/DefaultDependencyNodeTest.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java b/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java index 4f8cebc5f..1bdc53eb5 100644 --- a/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java +++ b/maven-resolver-api/src/test/java/org/eclipse/aether/graph/DefaultDependencyNodeTest.java @@ -45,7 +45,7 @@ public boolean visitLeave(DependencyNode node) { return true; } }; - AtomicReference thrown = new AtomicReference<>(null); + AtomicReference thrown = new AtomicReference<>(null); Thread t = new Thread(() -> { Thread.currentThread().interrupt(); try { @@ -59,9 +59,7 @@ public boolean visitLeave(DependencyNode node) { t.join(); assertTrue(thrown.get() instanceof RuntimeException, String.valueOf(thrown.get())); - assertTrue( - ((RuntimeException) thrown.get()).getCause() instanceof InterruptedException, - String.valueOf(thrown.get())); + assertTrue(thrown.get().getCause() instanceof InterruptedException, String.valueOf(thrown.get())); assertTrue(t.isInterrupted()); } }