Skip to content

[Backport 1.28.x] Don't hold locks forever, use a configurable timeout instead #1473

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
geoserver-bot wants to merge 1 commit into
base: 1.28.x
Choose a base branch
from
+525 −114
Open

[Backport 1.28.x] Don't hold locks forever, use a configurable timeout instead #1473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions geowebcache/core/src/main/java/org/geowebcache/locks/LockProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@
*/
public interface LockProvider {

/**
* The amount of time, in seconds, to wait for a lock to be released before giving up and throwing an exception.
* Default is 2 minutes.
*/
int GWC_LOCK_TIMEOUT = Integer.parseInt(System.getProperty("GWC_LOCK_TIMEOUT", String.valueOf(2 * 60)));

/** Acquires a exclusive lock on the specified key */
public Lock getLock(String lockKey) throws GeoWebCacheException;

Expand Down
19 changes: 13 additions & 6 deletions geowebcache/core/src/main/java/org/geowebcache/locks/MemoryLockProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package org.geowebcache.locks;

import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.locks.ReentrantLock;
import java.util.logging.Level;
Expand Down Expand Up @@ -69,7 +70,15 @@ public Lock getLock(String lockKey) {
return internalLockAndCounter;
});

lockAndCounter.lock.lock();
try {
if (!lockAndCounter.lock.tryLock(GWC_LOCK_TIMEOUT, TimeUnit.SECONDS)) {
// Throwing an exception prevents the thread from hanging indefinitely
throw new RuntimeException(String.format("Lock acquisition timeout for key [%s].", lockKey));
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException("Interrupted while trying to acquire lock for key " + lockKey, e);
}

if (LOGGER.isLoggable(Level.FINE)) LOGGER.fine("Acquired lock key " + lockKey);

Expand All @@ -88,10 +97,8 @@ public void release() {
// Attempt to remove lock if no other thread is waiting for it
if (lockAndCounter.counter.decrementAndGet() == 0) {

// Try to remove the lock, but we have to check the count AGAIN inside of
// "compute"
// so that we know it hasn't been incremented since the if-statement above
// was evaluated
// Try to remove the lock, but we have to check the count AGAIN inside of "compute" so that we
// know it hasn't been incremented since the if-statement above was evaluated
lockAndCounters.compute(lockKey, (key, existingLockAndCounter) -> {
if (existingLockAndCounter == null || existingLockAndCounter.counter.get() == 0) {
return null;
Expand All @@ -111,7 +118,7 @@ public void release() {
* remove it during a release.
*/
private static class LockAndCounter {
private final java.util.concurrent.locks.Lock lock = new ReentrantLock();
private final ReentrantLock lock = new ReentrantLock();

// The count of threads holding or waiting for this lock
private final AtomicInteger counter = new AtomicInteger(0);
Expand Down
206 changes: 98 additions & 108 deletions geowebcache/core/src/main/java/org/geowebcache/locks/NIOLockProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,165 +34,155 @@
*/
public class NIOLockProvider implements LockProvider {

public static final int DEFAULT_WAIT_BEFORE_RETRY = 20;
public static Logger LOGGER = Logging.getLogger(NIOLockProvider.class.getName());

private final String root;

/** The wait to occur in case the lock cannot be acquired */
private final int waitBeforeRetry;

/** max lock attempts */
private final int maxLockAttempts;
private final int timeoutSeconds;

MemoryLockProvider memoryProvider = new MemoryLockProvider();

public NIOLockProvider(DefaultStorageFinder storageFinder) throws ConfigurationException {
this(storageFinder.getDefaultPath());
}

public NIOLockProvider(DefaultStorageFinder storageFinder, int waitBeforeRetry, int maxLockAttempts)
public NIOLockProvider(DefaultStorageFinder storageFinder, int waitBeforeRetry, int timeoutSeconds)
throws ConfigurationException {
this.root = storageFinder.getDefaultPath();
this.waitBeforeRetry = waitBeforeRetry;
this.maxLockAttempts = maxLockAttempts;
this.timeoutSeconds = timeoutSeconds;
}

public NIOLockProvider(String root) throws ConfigurationException {
public NIOLockProvider(String root) {
this.root = root;
this.waitBeforeRetry = 20;
this.maxLockAttempts = 120 * 1000 / waitBeforeRetry;
this.waitBeforeRetry = DEFAULT_WAIT_BEFORE_RETRY;
this.timeoutSeconds = GWC_LOCK_TIMEOUT;
}

public NIOLockProvider(String root, int timeoutSeconds) {
this.root = root;
this.waitBeforeRetry = DEFAULT_WAIT_BEFORE_RETRY;
this.timeoutSeconds = timeoutSeconds;
}

@Override
@SuppressWarnings({"PMD.CloseResource", "PMD.UseTryWithResources"})
@SuppressWarnings({"PMD.CloseResource"})
// complex but seemingly correct resource handling
public LockProvider.Lock getLock(final String lockKey) throws GeoWebCacheException {
File file = null;
// first off, synchronize among threads in the same jvm (the nio locks won't lock
// threads in the same JVM)
final LockProvider.Lock memoryLock = memoryProvider.getLock(lockKey);
// then synch up between different processes
final File file = getFile(lockKey);

// Track these to ensure cleanup on failure
FileOutputStream currFos = null;
FileLock currLock = null;

if (LOGGER.isLoggable(Level.FINE))
LOGGER.fine("Mapped lock key " + lockKey + " to lock file " + file + ". Attempting to lock on it.");
try {
file = getFile(lockKey);
FileOutputStream currFos = null;
FileLock currLock = null;
try {
// try to lock
int count = 0;
while (currLock == null && count < maxLockAttempts) {
// the file output stream can also fail to be acquired due to the
// other nodes deleting the file
try {
currFos = new FileOutputStream(file);
long lockTimeoutMs = timeoutSeconds * 1000L;
long startTime = System.currentTimeMillis();

currLock = currFos.getChannel().lock();
} catch (OverlappingFileLockException | IOException e) {
while (currLock == null && (System.currentTimeMillis() - startTime) < lockTimeoutMs) {
try {
currFos = new FileOutputStream(file);
currLock = currFos.getChannel().tryLock();

if (currLock == null) {
IOUtils.closeQuietly(currFos);
try {
Thread.sleep(waitBeforeRetry);
} catch (InterruptedException ie) {
Thread.currentThread().interrupt();
// ok, moving on
}
Thread.sleep(waitBeforeRetry);
}
count++;
}

// verify we managed to get the FS lock
if (count >= maxLockAttempts) {
throw new GeoWebCacheException(
"Failed to get a lock on key " + lockKey + " after " + count + " attempts");
}

if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.fine("Lock "
+ lockKey
+ " acquired by thread "
+ Thread.currentThread().getName()
+ " on file "
+ file);
} catch (OverlappingFileLockException | IOException | InterruptedException e) {
IOUtils.closeQuietly(currFos);
if (e instanceof InterruptedException) {
Thread.currentThread().interrupt();
break;
}
Thread.sleep(waitBeforeRetry);
}
}

// store the results in a final variable for the inner class to use
final FileOutputStream fos = currFos;
final FileLock lock = currLock;

// nullify so that we don't close them, the locking occurred as expected
currFos = null;
currLock = null;
if (currLock == null) {
throw new IllegalStateException("Failed to get lock on " + lockKey + " after " + lockTimeoutMs + "ms");
}

final File lockFile = file;
return new LockProvider.Lock() {
if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.fine("Lock "
+ lockKey
+ " acquired by thread "
+ Thread.currentThread().getId()
+ " on file "
+ file);
}

boolean released;
final FileOutputStream finalFos = currFos;
final FileLock finalLock = currLock;

@Override
public void release() throws GeoWebCacheException {
if (released) {
return;
}
return new LockProvider.Lock() {
boolean released;

try {
released = true;
if (!lock.isValid()) {
// do not crap out, locks usage in GWC is only there to prevent
// duplication of work
if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.fine(
"Lock key "
+ lockKey
+ " for releasing lock is unkonwn, it means "
+ "this lock was never acquired, or was released twice. "
+ "Current thread is: "
+ Thread.currentThread().getName()
+ ". "
+ "Are you running two GWC instances in the same JVM using NIO locks? "
+ "This case is not supported and will generate exactly this error message");
return;
}
@Override
public void release() throws GeoWebCacheException {
if (released) return;
try {
released = true;
if (finalLock.isValid()) {
finalLock.release();
IOUtils.closeQuietly(finalFos);
file.delete(); // Proper place for deletion

if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.fine("Lock %s mapped onto %s released by thread %d"
.formatted(
lockKey,
file,
Thread.currentThread().getId()));
}
try {
lock.release();
IOUtils.closeQuietly(fos);
lockFile.delete();

if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.fine("Lock "
+ lockKey
+ " on file "
+ lockFile
+ " released by thread "
+ Thread.currentThread().getName());
}
} catch (IOException e) {
throw new GeoWebCacheException(
"Failure while trying to release lock for key " + lockKey, e);
} else {
// do not crap out, locks usage is only there to prevent duplication
// of work
if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.fine(("Lock key %s for releasing lock is unknown, it means this lock was never"
+ " acquired, or was released twice. Current thread is: %d. Are you"
+ " running two instances in the same JVM using NIO locks? This case is"
+ " not supported and will generate exactly this error message")
.formatted(
lockKey, Thread.currentThread().getId()));
}
} finally {
memoryLock.release();
}

} catch (IOException e) {
throw new IllegalStateException("Failure releasing lock " + lockKey, e);
} finally {
memoryLock.release();
}
};
} finally {
}
};
} catch (Exception e) {
// If we get here, acquisition failed or timed out
if (currLock != null) {
try {
if (currLock != null) {
currLock.release();
}
IOUtils.closeQuietly(currFos);
file.delete();
} finally {
memoryLock.release();
currLock.release();
} catch (IOException ignored) {
}
}
} catch (IOException e) {
throw new GeoWebCacheException("Failure while trying to get lock for key " + lockKey, e);
IOUtils.closeQuietly(currFos);
memoryLock.release(); // Must release memory lock on failure
throw (e instanceof RuntimeException) ? (RuntimeException) e : new IllegalStateException(e);
}
// Note: No finally block deleting the file here, it's done in the returned lock
}

private File getFile(String lockKey) {
File locks = new File(root, "lockfiles");
locks.mkdirs();
String sha1 = DigestUtils.sha1Hex(lockKey);
// cryptographically strong and has a chance of collision around 10^-59
String sha1 = DigestUtils.sha256Hex(lockKey);
return new File(locks, sha1 + ".lck");
}
}
Loading