You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maven.apache.org by cs...@apache.org on 2022/11/08 09:20:11 UTC
[maven-resolver] branch master updated: [MRESOLVER-285] File locking tweaks for Windows OS (#216)
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 53fda4b7 [MRESOLVER-285] File locking tweaks for Windows OS (#216)
53fda4b7 is described below
commit 53fda4b7ba1b97a3576c0eba424456b7b862f580
Author: Tamas Cservenak <ta...@cservenak.net>
AuthorDate: Tue Nov 8 10:20:07 2022 +0100
[MRESOLVER-285] File locking tweaks for Windows OS (#216)
That has concurrency issue at FS level when it comes to deletion of files.
This PR adds several "tweak" flags that allow users on Windows to circumvent the issue.
By default (w/o any user "tweaking") the file locking should now work fine on Windows, not leaving behind any "garbage" (0 byte lock files).
Related Java bug: https://bugs.openjdk.org/browse/JDK-8252883
---
https://issues.apache.org/jira/browse/MRESOLVER-285
---
.../named/providers/FileLockNamedLockFactory.java | 74 ++++++++++++++++++++--
.../aether/named/support/FileLockNamedLock.java | 18 +-----
.../org/eclipse/aether/named/support/Retry.java | 46 ++++++++++++++
.../java/org/eclipse/aether/named/RetryTest.java | 29 ++++++++-
4 files changed, 144 insertions(+), 23 deletions(-)
diff --git a/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/providers/FileLockNamedLockFactory.java b/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/providers/FileLockNamedLockFactory.java
index cacabe8e..5f1b9da4 100644
--- a/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/providers/FileLockNamedLockFactory.java
+++ b/maven-resolver-named-locks/src/main/java/org/eclipse/aether/named/providers/FileLockNamedLockFactory.java
@@ -22,6 +22,7 @@ package org.eclipse.aether.named.providers;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.channels.FileChannel;
+import java.nio.file.AccessDeniedException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
@@ -36,6 +37,8 @@ import org.eclipse.aether.named.support.FileLockNamedLock;
import org.eclipse.aether.named.support.NamedLockFactorySupport;
import org.eclipse.aether.named.support.NamedLockSupport;
+import static org.eclipse.aether.named.support.Retry.retry;
+
/**
* Named locks factory of {@link FileLockNamedLock}s. This is a bit special implementation, as it
* expects locks names to be fully qualified absolute file system paths.
@@ -49,6 +52,33 @@ public class FileLockNamedLockFactory
{
public static final String NAME = "file-lock";
+ /**
+ * Tweak: on Windows, the presence of {@link StandardOpenOption#DELETE_ON_CLOSE} causes concurrency issues. This
+ * flag allows to have it removed from effective flags, at the cost that lockfile directory becomes crowded
+ * with 0 byte sized lock files that are never cleaned up. Default value is {@code true}.
+ *
+ * @see <a href="https://bugs.openjdk.org/browse/JDK-8252883">JDK-8252883</a>
+ */
+ private static final boolean DELETE_LOCK_FILES = Boolean.parseBoolean(
+ System.getProperty( "aether.named.file-lock.deleteLockFiles", Boolean.TRUE.toString() ) );
+
+ /**
+ * Tweak: on Windows, the presence of {@link StandardOpenOption#DELETE_ON_CLOSE} causes concurrency issues. This
+ * flag allows to implement similar fix as referenced JDK bug report: retry and hope the best. Default value is
+ * 5 attempts (will retry 4 times).
+ *
+ * @see <a href="https://bugs.openjdk.org/browse/JDK-8252883">JDK-8252883</a>
+ */
+ private static final int ATTEMPTS = Integer.parseInt(
+ System.getProperty( "aether.named.file-lock.attempts", "5" ) );
+
+ /**
+ * Tweak: When {@link #ATTEMPTS} used, the amount of milliseconds to sleep between subsequent retries. Default
+ * value is 50 milliseconds.
+ */
+ private static final long SLEEP_MILLIS = Long.parseLong(
+ System.getProperty( "aether.named.file-lock.sleepMillis", "50" ) );
+
private final ConcurrentMap<String, FileChannel> fileChannels;
public FileLockNamedLockFactory()
@@ -65,11 +95,45 @@ public class FileLockNamedLockFactory
try
{
Files.createDirectories( path.getParent() );
- return FileChannel.open(
- path,
- StandardOpenOption.READ, StandardOpenOption.WRITE,
- StandardOpenOption.CREATE, StandardOpenOption.DELETE_ON_CLOSE
- );
+ FileChannel channel = retry( ATTEMPTS, SLEEP_MILLIS, () ->
+ {
+ try
+ {
+ if ( DELETE_LOCK_FILES )
+ {
+ return FileChannel.open(
+ path,
+ StandardOpenOption.READ, StandardOpenOption.WRITE,
+ StandardOpenOption.CREATE, StandardOpenOption.DELETE_ON_CLOSE
+ );
+ }
+ else
+ {
+ return FileChannel.open(
+ path,
+ StandardOpenOption.READ, StandardOpenOption.WRITE,
+ StandardOpenOption.CREATE
+ );
+ }
+ }
+ catch ( AccessDeniedException e )
+ {
+ return null;
+ }
+ }, null, null );
+
+ if ( channel == null )
+ {
+ throw new IllegalStateException( "Could not open file channel for '"
+ + name + "' after " + ATTEMPTS + " attempts; giving up" );
+ }
+ return channel;
+ }
+ catch ( InterruptedException e )
+ {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException( "Interrupted while opening file channel for '"
+ + name + "'", e );
}
catch ( IOException e )
{
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 729ff861..557a1664 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
@@ -112,8 +112,6 @@ public final class FileLockNamedLock
{
if ( obtainedLock.isShared() )
{
- // TODO No counterpart in other lock impls, drop or make consistent?
- logger.trace( "{} lock (shared={})", name(), true );
steps.push( Boolean.TRUE );
return true;
}
@@ -123,8 +121,6 @@ public final class FileLockNamedLock
boolean weOwnExclusive = steps.contains( Boolean.FALSE );
if ( weOwnExclusive )
{
- // TODO No counterpart in other lock impls, drop or make consistent?
- logger.trace( "{} lock (shared={})", name(), true );
steps.push( Boolean.TRUE );
return true;
}
@@ -136,8 +132,6 @@ public final class FileLockNamedLock
}
}
- // TODO No counterpart in other lock impls, drop or make consistent?
- logger.trace( "{} no obtained lock: obtain shared file lock", name() );
FileLock fileLock = obtainFileLock( true );
if ( fileLock != null )
{
@@ -170,10 +164,6 @@ public final class FileLockNamedLock
boolean weOwnShared = steps.contains( Boolean.TRUE );
if ( weOwnShared )
{
- // TODO No counterpart in other lock impls, drop or make consistent?
- logger.trace(
- "{} steps not empty, has not exclusive lock: lock-upgrade not supported", name()
- );
return false; // Lock upgrade not supported
}
else
@@ -188,8 +178,6 @@ public final class FileLockNamedLock
boolean weOwnExclusive = steps.contains( Boolean.FALSE );
if ( weOwnExclusive )
{
- // TODO No counterpart in other lock impls, drop or make consistent?
- logger.trace( "{} lock (shared={})", name(), false );
steps.push( Boolean.FALSE );
return true;
}
@@ -201,8 +189,6 @@ public final class FileLockNamedLock
}
}
- // TODO No counterpart in other lock impls, drop or make consistent?
- logger.trace( "{} no obtained lock: obtain exclusive file lock", name() );
FileLock fileLock = obtainFileLock( false );
if ( fileLock != null )
{
@@ -230,9 +216,7 @@ public final class FileLockNamedLock
{
throw new IllegalStateException( "Wrong API usage: unlock without lock" );
}
- Boolean shared = steps.pop();
- // TODO No counterpart in other lock impls, drop or make consistent?
- logger.trace( "{} unlock (shared = {})", name(), shared );
+ steps.pop();
if ( steps.isEmpty() && !anyOtherThreadHasSteps() )
{
try
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 b0914ed4..595166ac 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
@@ -90,4 +90,50 @@ public final class Retry
}
return result == null ? defaultResult : result;
}
+
+ /**
+ * Retries attempting max given times the passed in operation, sleeping given
+ * {@code sleepMills} between retries. In case operation returns {@code null}, it is assumed
+ * "is not done yet" state, so retry will happen (if attempt count allows). If all attempts
+ * used, and still {@code null} ("is not done yet") is returned from operation, the
+ * {@code defaultResult} is returned.
+ * <p>
+ * Just to clear things up: 5 attempts is really 4 retries (once do it and retry 4 times). 0 attempts means
+ * "do not even try it", and this method returns without doing anything.
+ */
+ public static <R> R retry( final int attempts,
+ final long sleepMillis,
+ final Callable<R> operation,
+ final Predicate<Exception> retryPredicate,
+ final R defaultResult ) throws InterruptedException
+ {
+ int attempt = 1;
+ R result = null;
+ while ( attempt <= attempts && result == null )
+ {
+ try
+ {
+ result = operation.call();
+ if ( result == null )
+ {
+ LOGGER.trace( "Retry attempt {}: no result", attempt );
+ Thread.sleep( sleepMillis );
+ }
+ }
+ catch ( InterruptedException e )
+ {
+ throw e;
+ }
+ catch ( Exception e )
+ {
+ LOGGER.trace( "Retry attempt {}: operation failure", attempt, e );
+ if ( retryPredicate != null && !retryPredicate.test( e ) )
+ {
+ throw new IllegalStateException( e );
+ }
+ }
+ attempt++;
+ }
+ return result == null ? defaultResult : result;
+ }
}
diff --git a/maven-resolver-named-locks/src/test/java/org/eclipse/aether/named/RetryTest.java b/maven-resolver-named-locks/src/test/java/org/eclipse/aether/named/RetryTest.java
index c26aeea3..bdd5cdb9 100644
--- a/maven-resolver-named-locks/src/test/java/org/eclipse/aether/named/RetryTest.java
+++ b/maven-resolver-named-locks/src/test/java/org/eclipse/aether/named/RetryTest.java
@@ -60,11 +60,38 @@ public class RetryTest
}
@Test
- public void happyOnSomeAttempt() throws InterruptedException
+ public void happyAfterSomeTime() throws InterruptedException
{
LongAdder retries = new LongAdder();
String result = retry( 1L, TimeUnit.SECONDS, RETRY_SLEEP_MILLIS, () -> { retries.increment(); return retries.sum() == 2 ? "got it" : null; }, null, "notHappy" );
assertThat( result, equalTo( "got it" ) );
assertThat( retries.sum(), equalTo( 2L ) );
}
+
+ @Test
+ public void happyFirstAttempt() throws InterruptedException
+ {
+ LongAdder retries = new LongAdder();
+ String result = retry( 5, RETRY_SLEEP_MILLIS, () -> { retries.increment(); return "happy"; }, null, "notHappy" );
+ assertThat( result, equalTo( "happy" ) );
+ assertThat( retries.sum(), equalTo( 1L ) );
+ }
+
+ @Test
+ public void notHappyAnyAttempt() throws InterruptedException
+ {
+ LongAdder retries = new LongAdder();
+ String result = retry( 5, RETRY_SLEEP_MILLIS, () -> { retries.increment(); return null; }, null, "notHappy" );
+ assertThat( result, equalTo( "notHappy" ) );
+ assertThat( retries.sum(), equalTo( 5L ) );
+ }
+
+ @Test
+ public void happyAfterSomeAttempt() throws InterruptedException
+ {
+ LongAdder retries = new LongAdder();
+ String result = retry( 5, RETRY_SLEEP_MILLIS, () -> { retries.increment(); return retries.sum() == 3 ? "got it" : null; }, null, "notHappy" );
+ assertThat( result, equalTo( "got it" ) );
+ assertThat( retries.sum(), equalTo( 3L ) );
+ }
}