Skip to content

Commit 319e6ed

Browse files
aaimeaaime
committed
Don't hold locks forever, use a configurable timeout instead
1 parent 3ac809f commit 319e6ed

File tree

5 files changed

+523
-114
lines changed

5 files changed

+523
-114
lines changed

‎geowebcache/core/src/main/java/org/geowebcache/locks/LockProvider.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,12 @@
2222
*/
2323
public interface LockProvider {
2424

25+
/**
26+
* The amount of time, in seconds, to wait for a lock to be released before giving up and throwing an exception.
27+
* Default is 2 minutes.
28+
*/
29+
int GWC_LOCK_TIMEOUT = Integer.parseInt(System.getProperty("GWC_LOCK_TIMEOUT", String.valueOf(2 * 60)));
30+
2531
/** Acquires a exclusive lock on the specified key */
2632
public Lock getLock(String lockKey) throws GeoWebCacheException;
2733

‎geowebcache/core/src/main/java/org/geowebcache/locks/MemoryLockProvider.java‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
package org.geowebcache.locks;
1515

1616
import java.util.concurrent.ConcurrentHashMap;
17+
import java.util.concurrent.TimeUnit;
1718
import java.util.concurrent.atomic.AtomicInteger;
1819
import java.util.concurrent.locks.ReentrantLock;
1920
import java.util.logging.Level;
@@ -69,7 +70,15 @@ public Lock getLock(String lockKey) {
6970
return internalLockAndCounter;
7071
});
7172

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

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

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

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

116123
// The count of threads holding or waiting for this lock
117124
private final AtomicInteger counter = new AtomicInteger(0);

‎geowebcache/core/src/main/java/org/geowebcache/locks/NIOLockProvider.java‎

Lines changed: 96 additions & 108 deletions
Original file line numberDiff line numberDiff line change
@@ -34,165 +34,153 @@
3434
*/
3535
public class NIOLockProvider implements LockProvider {
3636

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

3940
private final String root;
4041

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

44-
/** max lock attempts */
45-
private final int maxLockAttempts;
45+
private final int timeoutSeconds;
4646

4747
MemoryLockProvider memoryProvider = new MemoryLockProvider();
4848

4949
public NIOLockProvider(DefaultStorageFinder storageFinder) throws ConfigurationException {
5050
this(storageFinder.getDefaultPath());
5151
}
5252

53-
public NIOLockProvider(DefaultStorageFinder storageFinder, int waitBeforeRetry, int maxLockAttempts)
53+
public NIOLockProvider(DefaultStorageFinder storageFinder, int waitBeforeRetry, int timeoutSeconds)
5454
throws ConfigurationException {
5555
this.root = storageFinder.getDefaultPath();
5656
this.waitBeforeRetry = waitBeforeRetry;
57-
this.maxLockAttempts = maxLockAttempts;
57+
this.timeoutSeconds = timeoutSeconds;
5858
}
5959

60-
public NIOLockProvider(String root) throws ConfigurationException {
60+
public NIOLockProvider(String root) {
6161
this.root = root;
62-
this.waitBeforeRetry = 20;
63-
this.maxLockAttempts = 120 * 1000 / waitBeforeRetry;
62+
this.waitBeforeRetry = DEFAULT_WAIT_BEFORE_RETRY;
63+
this.timeoutSeconds = GWC_LOCK_TIMEOUT;
64+
}
65+
66+
public NIOLockProvider(String root, int timeoutSeconds) {
67+
this.root = root;
68+
this.waitBeforeRetry = DEFAULT_WAIT_BEFORE_RETRY;
69+
this.timeoutSeconds = timeoutSeconds;
6470
}
6571

6672
@Override
67-
@SuppressWarnings({"PMD.CloseResource", "PMD.UseTryWithResources"})
73+
@SuppressWarnings({"PMD.CloseResource"})
6874
// complex but seemingly correct resource handling
6975
public LockProvider.Lock getLock(final String lockKey) throws GeoWebCacheException {
70-
File file = null;
7176
// first off, synchronize among threads in the same jvm (the nio locks won't lock
7277
// threads in the same JVM)
7378
final LockProvider.Lock memoryLock = memoryProvider.getLock(lockKey);
74-
// then synch up between different processes
79+
final File file = getFile(lockKey);
80+
81+
// Track these to ensure cleanup on failure
82+
FileOutputStream currFos = null;
83+
FileLock currLock = null;
84+
85+
if (LOGGER.isLoggable(Level.FINE))
86+
LOGGER.fine("Mapped lock key " + lockKey + " to lock file " + file + ". Attempting to lock on it.");
7587
try {
76-
file = getFile(lockKey);
77-
FileOutputStream currFos = null;
78-
FileLock currLock = null;
79-
try {
80-
// try to lock
81-
int count = 0;
82-
while (currLock == null && count < maxLockAttempts) {
83-
// the file output stream can also fail to be acquired due to the
84-
// other nodes deleting the file
85-
try {
86-
currFos = new FileOutputStream(file);
88+
long lockTimeoutMs = timeoutSeconds * 1000L;
89+
long startTime = System.currentTimeMillis();
8790

88-
currLock = currFos.getChannel().lock();
89-
} catch (OverlappingFileLockException | IOException e) {
91+
while (currLock == null && (System.currentTimeMillis() - startTime) < lockTimeoutMs) {
92+
try {
93+
currFos = new FileOutputStream(file);
94+
currLock = currFos.getChannel().tryLock();
95+
96+
if (currLock == null) {
9097
IOUtils.closeQuietly(currFos);
91-
try {
92-
Thread.sleep(waitBeforeRetry);
93-
} catch (InterruptedException ie) {
94-
Thread.currentThread().interrupt();
95-
// ok, moving on
96-
}
98+
Thread.sleep(waitBeforeRetry);
9799
}
98-
count++;
99-
}
100-
101-
// verify we managed to get the FS lock
102-
if (count >= maxLockAttempts) {
103-
throw new GeoWebCacheException(
104-
"Failed to get a lock on key " + lockKey + " after " + count + " attempts");
105-
}
106-
107-
if (LOGGER.isLoggable(Level.FINE)) {
108-
LOGGER.fine("Lock "
109-
+ lockKey
110-
+ " acquired by thread "
111-
+ Thread.currentThread().getName()
112-
+ " on file "
113-
+ file);
100+
} catch (OverlappingFileLockException | IOException | InterruptedException e) {
101+
IOUtils.closeQuietly(currFos);
102+
if (e instanceof InterruptedException) {
103+
Thread.currentThread().interrupt();
104+
break;
105+
}
106+
Thread.sleep(waitBeforeRetry);
114107
}
108+
}
115109

116-
// store the results in a final variable for the inner class to use
117-
final FileOutputStream fos = currFos;
118-
final FileLock lock = currLock;
119-
120-
// nullify so that we don't close them, the locking occurred as expected
121-
currFos = null;
122-
currLock = null;
110+
if (currLock == null) {
111+
throw new IllegalStateException("Failed to get lock on " + lockKey + " after " + lockTimeoutMs + "ms");
112+
}
123113

124-
final File lockFile = file;
125-
return new LockProvider.Lock() {
114+
if (LOGGER.isLoggable(Level.FINE)) {
115+
LOGGER.fine("Lock "
116+
+ lockKey
117+
+ " acquired by thread "
118+
+ Thread.currentThread().getId()
119+
+ " on file "
120+
+ file);
121+
}
126122

127-
boolean released;
123+
final FileOutputStream finalFos = currFos;
124+
final FileLock finalLock = currLock;
128125

129-
@Override
130-
public void release() throws GeoWebCacheException {
131-
if (released) {
132-
return;
133-
}
126+
return new LockProvider.Lock() {
127+
boolean released;
134128

135-
try {
136-
released = true;
137-
if (!lock.isValid()) {
138-
// do not crap out, locks usage in GWC is only there to prevent
139-
// duplication of work
140-
if (LOGGER.isLoggable(Level.FINE)) {
141-
LOGGER.fine(
142-
"Lock key "
143-
+ lockKey
144-
+ " for releasing lock is unkonwn, it means "
145-
+ "this lock was never acquired, or was released twice. "
146-
+ "Current thread is: "
147-
+ Thread.currentThread().getName()
148-
+ ". "
149-
+ "Are you running two GWC instances in the same JVM using NIO locks? "
150-
+ "This case is not supported and will generate exactly this error message");
151-
return;
152-
}
129+
@Override
130+
public void release() throws GeoWebCacheException {
131+
if (released) return;
132+
try {
133+
released = true;
134+
if (finalLock.isValid()) {
135+
finalLock.release();
136+
IOUtils.closeQuietly(finalFos);
137+
file.delete(); // Proper place for deletion
138+
139+
if (LOGGER.isLoggable(Level.FINE)) {
140+
LOGGER.fine(String.format(
141+
"Lock %s mapped onto %s released by thread %d",
142+
lockKey, file, Thread.currentThread().getId()));
153143
}
154-
try {
155-
lock.release();
156-
IOUtils.closeQuietly(fos);
157-
lockFile.delete();
158-
159-
if (LOGGER.isLoggable(Level.FINE)) {
160-
LOGGER.fine("Lock "
161-
+ lockKey
162-
+ " on file "
163-
+ lockFile
164-
+ " released by thread "
165-
+ Thread.currentThread().getName());
166-
}
167-
} catch (IOException e) {
168-
throw new GeoWebCacheException(
169-
"Failure while trying to release lock for key " + lockKey, e);
144+
} else {
145+
// do not crap out, locks usage is only there to prevent duplication
146+
// of work
147+
if (LOGGER.isLoggable(Level.FINE)) {
148+
LOGGER.fine(String.format(
149+
"Lock key %s for releasing lock is unknown, it means this lock was never"
150+
+ " acquired, or was released twice. Current thread is: %d. Are you"
151+
+ " running two instances in the same JVM using NIO locks? This case is"
152+
+ " not supported and will generate exactly this error message",
153+
lockKey, Thread.currentThread().getId()));
170154
}
171-
} finally {
172-
memoryLock.release();
173155
}
156+
157+
} catch (IOException e) {
158+
throw new IllegalStateException("Failure releasing lock " + lockKey, e);
159+
} finally {
160+
memoryLock.release();
174161
}
175-
};
176-
} finally {
162+
}
163+
};
164+
} catch (Exception e) {
165+
// If we get here, acquisition failed or timed out
166+
if (currLock != null) {
177167
try {
178-
if (currLock != null) {
179-
currLock.release();
180-
}
181-
IOUtils.closeQuietly(currFos);
182-
file.delete();
183-
} finally {
184-
memoryLock.release();
168+
currLock.release();
169+
} catch (IOException ignored) {
185170
}
186171
}
187-
} catch (IOException e) {
188-
throw new GeoWebCacheException("Failure while trying to get lock for key " + lockKey, e);
172+
IOUtils.closeQuietly(currFos);
173+
memoryLock.release(); // Must release memory lock on failure
174+
throw (e instanceof RuntimeException) ? (RuntimeException) e : new IllegalStateException(e);
189175
}
176+
// Note: No finally block deleting the file here, it's done in the returned lock
190177
}
191178

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

0 commit comments

Comments
 (0)