This is an automated email from the ASF dual-hosted git repository.
cstamas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/maven-resolver.git
The following commit(s) were added to refs/heads/master by this push:
new 7395cd45 [MRESOLVER-220] Refuse proactively unsupported operation
(#301)
7395cd45 is described below
commit 7395cd4584005e37fe7fe67b8cf3949c0c21aad1
Author: Tamas Cservenak <[email protected]>
AuthorDate: Tue Jun 20 14:51:45 2023 +0200
[MRESOLVER-220] Refuse proactively unsupported operation (#301)
Instead to silently "send result" (that is false/code expect true=success)
to caller, as this makes totally strange outcome: "failed to lock within 30sec"
but caller did not wait anything. Moreover, the bug where upgrade is attempted
slips in unnoticed.
In short: it was totally wrong that lock implementation immediately returns
"false", hiding the fact that user attempted non supported operation.
---
https://issues.apache.org/jira/browse/MRESOLVER-220
---
.../NamedLockFactoryAdapterTestSupport.java | 4 +--
.../NamedLockFactoryAdapterTestSupport.java | 3 +-
.../hazelcast/NamedLockFactoryTestSupport.java | 7 ++++-
.../named/support/AdaptedSemaphoreNamedLock.java | 2 +-
.../aether/named/support/FileLockNamedLock.java | 2 +-
.../support/LockUpgradeNotSupportedException.java | 36 ++++++++++++++++++++++
.../named/support/ReadWriteLockNamedLock.java | 2 +-
.../org/eclipse/aether/named/support/Retry.java | 22 +++++++++++++
.../aether/named/NamedLockFactoryTestSupport.java | 7 ++++-
9 files changed, 77 insertions(+), 8 deletions(-)
diff --git
a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/synccontext/NamedLockFactoryAdapterTestSupport.java
b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/synccontext/NamedLockFactoryAdapterTestSupport.java
index b647c07a..43f0eab4 100644
---
a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/synccontext/NamedLockFactoryAdapterTestSupport.java
+++
b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/synccontext/NamedLockFactoryAdapterTestSupport.java
@@ -31,6 +31,7 @@ import org.eclipse.aether.SyncContext;
import org.eclipse.aether.artifact.DefaultArtifact;
import org.eclipse.aether.internal.impl.synccontext.named.*;
import org.eclipse.aether.named.NamedLockFactory;
+import org.eclipse.aether.named.support.LockUpgradeNotSupportedException;
import org.eclipse.aether.repository.LocalRepository;
import org.eclipse.aether.spi.synccontext.SyncContextFactory;
import org.junit.AfterClass;
@@ -266,8 +267,7 @@ public abstract class NamedLockFactoryAdapterTestSupport {
chained.run();
}
loser.await();
- } catch (IllegalStateException e) {
- e.printStackTrace(); // for ref purposes
+ } catch (IllegalStateException |
LockUpgradeNotSupportedException e) {
loser.countDown();
winner.await();
}
diff --git
a/maven-resolver-named-locks-hazelcast/src/test/java/org/eclipse/aether/named/hazelcast/NamedLockFactoryAdapterTestSupport.java
b/maven-resolver-named-locks-hazelcast/src/test/java/org/eclipse/aether/named/hazelcast/NamedLockFactoryAdapterTestSupport.java
index f7e031b0..b5a57ca9 100644
---
a/maven-resolver-named-locks-hazelcast/src/test/java/org/eclipse/aether/named/hazelcast/NamedLockFactoryAdapterTestSupport.java
+++
b/maven-resolver-named-locks-hazelcast/src/test/java/org/eclipse/aether/named/hazelcast/NamedLockFactoryAdapterTestSupport.java
@@ -33,6 +33,7 @@ import
org.eclipse.aether.internal.impl.synccontext.named.DiscriminatingNameMapp
import org.eclipse.aether.internal.impl.synccontext.named.GAVNameMapper;
import
org.eclipse.aether.internal.impl.synccontext.named.NamedLockFactoryAdapter;
import org.eclipse.aether.named.NamedLockFactory;
+import org.eclipse.aether.named.support.LockUpgradeNotSupportedException;
import org.eclipse.aether.repository.LocalRepository;
import org.eclipse.aether.spi.synccontext.SyncContextFactory;
import org.junit.AfterClass;
@@ -230,7 +231,7 @@ public abstract class NamedLockFactoryAdapterTestSupport {
chained.run();
}
loser.await();
- } catch (IllegalStateException e) {
+ } catch (IllegalStateException |
LockUpgradeNotSupportedException e) {
loser.countDown();
winner.await();
}
diff --git
a/maven-resolver-named-locks-hazelcast/src/test/java/org/eclipse/aether/named/hazelcast/NamedLockFactoryTestSupport.java
b/maven-resolver-named-locks-hazelcast/src/test/java/org/eclipse/aether/named/hazelcast/NamedLockFactoryTestSupport.java
index ad11bddc..bc1c779a 100644
---
a/maven-resolver-named-locks-hazelcast/src/test/java/org/eclipse/aether/named/hazelcast/NamedLockFactoryTestSupport.java
+++
b/maven-resolver-named-locks-hazelcast/src/test/java/org/eclipse/aether/named/hazelcast/NamedLockFactoryTestSupport.java
@@ -23,6 +23,7 @@ import java.util.concurrent.TimeUnit;
import org.eclipse.aether.named.NamedLock;
import org.eclipse.aether.named.NamedLockFactory;
+import org.eclipse.aether.named.support.LockUpgradeNotSupportedException;
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.Rule;
@@ -114,7 +115,11 @@ public abstract class NamedLockFactoryTestSupport {
final String name = testName.getMethodName();
try (NamedLock one = namedLockFactory.getLock(name)) {
assertThat(one.lockShared(1L, TimeUnit.MILLISECONDS), is(true));
- assertThat(one.lockExclusively(1L, TimeUnit.MILLISECONDS),
is(false));
+ try {
+ one.lockExclusively(1L, TimeUnit.MILLISECONDS);
+ } catch (LockUpgradeNotSupportedException e) {
+ // good
+ }
one.unlock();
}
}
diff --git
a/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/AdaptedSemaphoreNamedLock.java
b/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/AdaptedSemaphoreNamedLock.java
index f3be9665..4b282de1 100644
---
a/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/AdaptedSemaphoreNamedLock.java
+++
b/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/AdaptedSemaphoreNamedLock.java
@@ -87,7 +87,7 @@ public class AdaptedSemaphoreNamedLock extends
NamedLockSupport {
perms.push(NONE);
return true;
} else {
- return false; // Lock upgrade not supported
+ throw new LockUpgradeNotSupportedException(this); // Lock
upgrade not supported
}
}
if (semaphore.tryAcquire(EXCLUSIVE, time, unit)) {
diff --git
a/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/FileLockNamedLock.java
b/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/FileLockNamedLock.java
index e69dba93..c70418fb 100644
---
a/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/FileLockNamedLock.java
+++
b/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/FileLockNamedLock.java
@@ -137,7 +137,7 @@ public final class FileLockNamedLock extends
NamedLockSupport {
// if we own shared, that's attempted upgrade
boolean weOwnShared = steps.contains(Boolean.TRUE);
if (weOwnShared) {
- return false; // Lock upgrade not supported
+ throw new LockUpgradeNotSupportedException(this);
// Lock upgrade not supported
} else {
// someone else owns shared, let's wait
return null;
diff --git
a/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/LockUpgradeNotSupportedException.java
b/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/LockUpgradeNotSupportedException.java
new file mode 100644
index 00000000..7cd7b137
--- /dev/null
+++
b/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/LockUpgradeNotSupportedException.java
@@ -0,0 +1,36 @@
+/*
+ * 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.named.support;
+
+/**
+ * Exception thrown when lock upgrade attempted that we do not support. This
exception when used within {@link Retry}
+ * helper should never be reattempted, hence is marked with {@link
Retry.DoNotRetry} marker.
+ *
+ * @since 1.9.13
+ */
+public final class LockUpgradeNotSupportedException extends RuntimeException
implements Retry.DoNotRetry {
+ /**
+ * Constructor for case, when current thread attempts lock upgrade on
given lock instance.
+ */
+ public LockUpgradeNotSupportedException(NamedLockSupport namedLock) {
+ super("Thread " + Thread.currentThread().getName()
+ + " already holds shared lock for '" + namedLock.name()
+ + "', but asks for exclusive lock; lock upgrade not
supported");
+ }
+}
diff --git
a/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/ReadWriteLockNamedLock.java
b/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/ReadWriteLockNamedLock.java
index df49e958..cb81818d 100644
---
a/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/ReadWriteLockNamedLock.java
+++
b/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/ReadWriteLockNamedLock.java
@@ -67,7 +67,7 @@ public class ReadWriteLockNamedLock extends NamedLockSupport {
Deque<Step> steps = threadSteps.get();
if (!steps.isEmpty()) { // we already own shared or exclusive lock
if (!steps.contains(Step.EXCLUSIVE)) {
- return false; // Lock upgrade not supported
+ throw new LockUpgradeNotSupportedException(this); // Lock
upgrade not supported
}
}
if (readWriteLock.writeLock().tryLock(time, unit)) {
diff --git
a/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/Retry.java
b/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/Retry.java
index 7cc6a9c4..0dad803c 100644
---
a/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/Retry.java
+++
b/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/support/Retry.java
@@ -37,6 +37,14 @@ import org.slf4j.LoggerFactory;
public final class Retry {
private static final Logger LOGGER = LoggerFactory.getLogger(Retry.class);
+ /**
+ * Marker interface to apply onto exceptions to make them "never retried"
when thrown. This shortcuts checks with
+ * predicate, if used.
+ *
+ * @since 1.9.13
+ */
+ public interface DoNotRetry {}
+
private Retry() {
// no instances
}
@@ -71,6 +79,13 @@ public final class Retry {
throw e;
} catch (Exception e) {
LOGGER.trace("Retry attempt {}: operation failure", attempt,
e);
+ if (e instanceof DoNotRetry) {
+ if (e instanceof RuntimeException) {
+ throw (RuntimeException) e;
+ } else {
+ throw new IllegalStateException(e);
+ }
+ }
if (retryPredicate != null && !retryPredicate.test(e)) {
throw new IllegalStateException(e);
}
@@ -111,6 +126,13 @@ public final class Retry {
throw e;
} catch (Exception e) {
LOGGER.trace("Retry attempt {}: operation failure", attempt,
e);
+ if (e instanceof DoNotRetry) {
+ if (e instanceof RuntimeException) {
+ throw (RuntimeException) e;
+ } else {
+ throw new IllegalStateException(e);
+ }
+ }
if (retryPredicate != null && !retryPredicate.test(e)) {
throw new IllegalStateException(e);
}
diff --git
a/maven-resolver-named-locks/src/test/java/org/eclipse/aether/named/NamedLockFactoryTestSupport.java
b/maven-resolver-named-locks/src/test/java/org/eclipse/aether/named/NamedLockFactoryTestSupport.java
index 72e07e4e..9904a1e9 100644
---
a/maven-resolver-named-locks/src/test/java/org/eclipse/aether/named/NamedLockFactoryTestSupport.java
+++
b/maven-resolver-named-locks/src/test/java/org/eclipse/aether/named/NamedLockFactoryTestSupport.java
@@ -21,6 +21,7 @@ package org.eclipse.aether.named;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
+import org.eclipse.aether.named.support.LockUpgradeNotSupportedException;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
@@ -132,7 +133,11 @@ public abstract class NamedLockFactoryTestSupport {
final String name = lockName();
try (NamedLock one = namedLockFactory.getLock(name)) {
assertThat(one.lockShared(1L, TimeUnit.MILLISECONDS), is(true));
- assertThat(one.lockExclusively(1L, TimeUnit.MILLISECONDS),
is(false));
+ try {
+ one.lockExclusively(1L, TimeUnit.MILLISECONDS);
+ } catch (LockUpgradeNotSupportedException e) {
+ // good
+ }
one.unlock();
}
}
