Skip to content

Commit

Permalink
[GEOS-7376] Deadlock in GWC when reloading config while under heavy t…
Browse files Browse the repository at this point in the history
…ile request load
  • Loading branch information
aaime committed Jan 16, 2016
1 parent 52e74d8 commit 5d57896
Show file tree
Hide file tree
Showing 3 changed files with 191 additions and 37 deletions.
@@ -1,4 +1,4 @@
/* (c) 2014 Open Source Geospatial Foundation - all rights reserved
/* (c) 2014 - 2016 Open Source Geospatial Foundation - all rights reserved
* (c) 2001 - 2013 OpenPlans
* This code is licensed under the GPL 2.0 license, available at the root
* application directory.
Expand Down Expand Up @@ -63,6 +63,11 @@
* @see CatalogStyleChangeListener
*/
public class CatalogConfiguration implements Configuration {

/**
* The configuration lock timeout, in seconds
*/
static final int GWC_CONFIGURATION_LOCK_TIMEOUT = Integer.getInteger("gwc.configuration.lock.timeout", 60);

/**
* {@link GeoServerTileLayer} cache loader
Expand All @@ -80,7 +85,7 @@ public GeoServerTileLayer load(String layerId) throws Exception {
GeoServerTileLayer tileLayer = null;
final GridSetBroker gridSetBroker = CatalogConfiguration.this.gridSetBroker;

lock.readLock().lock();
lock.acquireReadLock();
try {
if (pendingDeletes.contains(layerId)) {
throw new IllegalArgumentException("Tile layer '" + layerId + "' was deleted.");
Expand All @@ -97,14 +102,15 @@ public GeoServerTileLayer load(String layerId) throws Exception {
tileLayer = new GeoServerTileLayer(geoServerCatalog, layerId, gridSetBroker,
tileLayerInfo);
} finally {
lock.readLock().unlock();
lock.releaseReadLock();
}
if (null == tileLayer) {
throw new IllegalArgumentException("GeoServer layer or layer group '" + layerId
+ "' does not exist");
}
return tileLayer;
}

}

private static final Logger LOGGER = Logging.getLogger(CatalogConfiguration.class);
Expand All @@ -127,7 +133,7 @@ public GeoServerTileLayer load(String layerId) throws Exception {
*/
private final Set<String> pendingDeletes = new CopyOnWriteArraySet<String>();

private final ReadWriteLock lock = new ReentrantReadWriteLock();
private final TimeoutReadWriteLock lock = new TimeoutReadWriteLock(GWC_CONFIGURATION_LOCK_TIMEOUT * 1000, "GWC Configuration");

public CatalogConfiguration(final Catalog catalog, final TileLayerCatalog tileLayerCatalog,
final GridSetBroker gridSetBroker) {
Expand All @@ -145,7 +151,7 @@ public CatalogConfiguration(final Catalog catalog, final TileLayerCatalog tileLa
.maximumSize(100)//
.build(new TileLayerLoader(tileLayerCatalog));
}

/**
*
* @see org.geowebcache.config.Configuration#getIdentifier()
Expand Down Expand Up @@ -194,7 +200,7 @@ public List<GeoServerTileLayer> getTileLayers() {
*/
@Override
public Iterable<GeoServerTileLayer> getLayers() {
lock.readLock().lock();
lock.acquireReadLock();
try {
final Set<String> layerIds = tileLayerCatalog.getLayerIds();

Expand All @@ -207,7 +213,7 @@ public GeoServerTileLayer apply(final String layerId) {

return Iterables.transform(layerIds, lazyLayerFetch);
} finally {
lock.readLock().unlock();
lock.releaseReadLock();
}
}

Expand All @@ -218,7 +224,7 @@ public GeoServerTileLayer apply(final String layerId) {
*/
@Override
public Set<String> getTileLayerNames() {
lock.readLock().lock();
lock.acquireReadLock();
try {
final Set<String> storedNames = tileLayerCatalog.getLayerNames();
Set<String> names = null;
Expand Down Expand Up @@ -248,14 +254,14 @@ public Set<String> getTileLayerNames() {
}
return names == null ? storedNames : Collections.unmodifiableSet(names);
} finally {
lock.readLock().unlock();
lock.releaseReadLock();
}
}

@Override
public boolean containsLayer(String layerId) {
checkNotNull(layerId, "layer id is null");
lock.readLock().lock();
lock.acquireReadLock();
try {
if (pendingDeletes.contains(layerId)) {
return false;
Expand All @@ -264,7 +270,7 @@ public boolean containsLayer(String layerId) {
boolean hasLayer = layerIds.contains(layerId);
return hasLayer;
} finally {
lock.readLock().unlock();
lock.releaseReadLock();
}
}

Expand All @@ -273,12 +279,15 @@ public GeoServerTileLayer getTileLayerById(final String layerId) {
checkNotNull(layerId, "layer id is null");

GeoServerTileLayer layer;
lock.acquireReadLock();
try {
layer = layerCache.get(layerId);
} catch (ExecutionException e) {
throw propagate(e.getCause());
} catch (UncheckedExecutionException e) {
throw propagate(e.getCause());
} finally {
lock.releaseReadLock();
}

return layer;
Expand All @@ -293,14 +302,14 @@ public GeoServerTileLayer getTileLayer(final String layerName) {

final String layerId;

lock.readLock().lock();
lock.acquireReadLock();
try {
layerId = getLayerId(layerName);
if (layerId == null) {
return null;
}
} finally {
lock.readLock().unlock();
lock.releaseReadLock();
}
return getTileLayerById(layerId);
}
Expand Down Expand Up @@ -367,7 +376,7 @@ private GeoServerTileLayerInfo getTileLayerInfoByName(final String layerName) {
@Override
public int getTileLayerCount() {
int count = 0;
lock.readLock().lock();
lock.acquireReadLock();
try {
Set<String> layerIds = tileLayerCatalog.getLayerIds();
if (pendingDeletes.isEmpty()) {
Expand All @@ -381,7 +390,7 @@ public int getTileLayerCount() {
}
}
} finally {
lock.readLock().unlock();
lock.releaseReadLock();
}
return count;
}
Expand All @@ -391,7 +400,7 @@ public int getTileLayerCount() {
*/
@Override
public int initialize(GridSetBroker gridSetBroker) {
lock.writeLock().lock();
lock.acquireWriteLock();
try {
LOGGER.info("Initializing GWC configuration based on GeoServer's Catalog");
this.gridSetBroker = gridSetBroker;
Expand All @@ -415,7 +424,7 @@ public int initialize(GridSetBroker gridSetBroker) {
}
LOGGER.info("GWC configuration based on GeoServer's Catalog loaded successfuly");
} finally {
lock.writeLock().unlock();
lock.releaseWriteLock();
}
return getTileLayerCount();
}
Expand Down Expand Up @@ -470,7 +479,7 @@ public synchronized void addLayer(final TileLayer tl) {
return;
}

lock.writeLock().lock();
lock.acquireWriteLock();
try {
boolean pending = pendingModications.containsKey(info.getId());
boolean exists = null != tileLayerCatalog.getLayerById(info.getId());
Expand All @@ -484,7 +493,7 @@ public synchronized void addLayer(final TileLayer tl) {
}
pendingModications.put(info.getId(), info);
} finally {
lock.writeLock().unlock();
lock.releaseWriteLock();
}
}

Expand All @@ -503,7 +512,7 @@ public synchronized void modifyLayer(TileLayer tl) throws NoSuchElementException
checkNotNull(tileLayer.getInfo().getName(), "name is null");

final GeoServerTileLayerInfo info = tileLayer.getInfo();
lock.writeLock().lock();
lock.acquireWriteLock();
try {
final String layerId = info.getId();
// check pendingModifications too to catch unsaved adds
Expand All @@ -513,7 +522,7 @@ public synchronized void modifyLayer(TileLayer tl) throws NoSuchElementException
pendingModications.put(layerId, info);
layerCache.invalidate(layerId);
} finally {
lock.writeLock().unlock();
lock.releaseWriteLock();
}
}

Expand All @@ -526,7 +535,7 @@ public synchronized void modifyLayer(TileLayer tl) throws NoSuchElementException
@Override
public boolean removeLayer(final String layerName) {
checkNotNull(layerName);
lock.writeLock().lock();
lock.acquireWriteLock();
try {
GeoServerTileLayerInfo tileLayerInfo = getTileLayerInfoByName(layerName);
if (tileLayerInfo != null) {
Expand All @@ -539,7 +548,7 @@ public boolean removeLayer(final String layerName) {
return false;
}
} finally {
lock.writeLock().unlock();
lock.releaseWriteLock();
}
}

Expand All @@ -559,7 +568,7 @@ public synchronized void save() {
final Set<String/* name */> deletedNames = Sets.newHashSet();
final List<GeoServerTileLayerInfo[/* old, new */]> modifications = Lists.newLinkedList();

lock.writeLock().lock();
lock.acquireWriteLock();
// perform the transaction while holding the write lock, then downgrade to the read lock and
// issue the modification events (otherwise another thread asking for any changed layer
// would lock)
Expand Down Expand Up @@ -589,9 +598,8 @@ public synchronized void save() {
this.pendingModications.clear();
this.pendingDeletes.clear();
} finally {
// Downgrade by acquiring read lock before releasing write lock
lock.readLock().lock();
lock.writeLock().unlock(); // Unlock write, still hold read
// Downgrade to read
lock.downgradeToReadLock();
try {
// issue notifications
for (String deletedLayerName : deletedNames) {
Expand Down Expand Up @@ -622,7 +630,7 @@ public synchronized void save() {
}
}
} finally {
lock.readLock().unlock();
lock.releaseReadLock();
}
}
}
Expand Down Expand Up @@ -697,12 +705,12 @@ private Set<String> gridsetNames(Set<XMLGridSubset> gridSubsets) {
}

public void reset() {
lock.writeLock().lock();
lock.acquireWriteLock();
try {
this.layerCache.invalidateAll();
this.tileLayerCatalog.reset();
} finally {
lock.writeLock().unlock();
lock.releaseWriteLock();
}
}
}
@@ -0,0 +1,90 @@
/* (c) 2016 Open Source Geospatial Foundation - all rights reserved
* This code is licensed under the GPL 2.0 license, available at the root
* application directory.
*/
package org.geoserver.gwc.layer;

import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;

import org.geoserver.platform.ServiceException;

/**
* A wrapper around a ReadWriteLock that will perform all locking operations under a timeout to prevent deadlocks.
*
* @author Andrea Aime - GeoSolutions
*/
class TimeoutReadWriteLock {

ReadWriteLock lock = new ReentrantReadWriteLock();

int timeoutMs;

String name;

/**
* Builds the {@link ReadWriteLock} wrapper with a given timeout, in milliseconds
* @param timeoutMs
*/
public TimeoutReadWriteLock(int timeoutMs, String name) {
this.timeoutMs = timeoutMs;
this.name = name;
}

/**
* Acquires a read lock with the configured timeout, will throw a {@link ServiceException} if the lock is not acquired
*/
public void acquireReadLock() {
boolean acquired = false;
try {
acquired = lock.readLock().tryLock(timeoutMs, TimeUnit.MILLISECONDS);
} catch (InterruptedException e) {
throw new ServiceException("Failed to acquire read lock on '" + name + "' due to interruption", e);
}
if (!acquired) {
throw new ServiceException(
"Failed to acquire read lock on '" + name + "' in less than " + timeoutMs + " ms");
}
}

/**
* Releases a previously acquired read lock
*/
public void releaseReadLock() {
lock.readLock().unlock();
}

/**
* Acquires a write lock with the configured timeout, will throw a {@link ServiceException} if the lock is not acquired
*/
public void acquireWriteLock() {
boolean acquired = false;
try {
acquired = lock.writeLock().tryLock(timeoutMs, TimeUnit.MILLISECONDS);
} catch (InterruptedException e) {
throw new ServiceException("Failed to acquire write lock on '" + name + "' due to interruption", e);
}
if (!acquired) {
throw new ServiceException(
"Failed to acquire write lock on '" + name + "' in less than " + timeoutMs + " ms");
}
}

/**
* Releases a previously acquired write lock
*/
public void releaseWriteLock() {
lock.writeLock().unlock();
}

/**
* Downgrades a write lock to a read lock. The write lock gets released, the caller must still release the read lock after this is called
*/
public void downgradeToReadLock() {
// Downgrade by acquiring read lock before releasing write lock
lock.readLock().lock();
// Unlock write, still hold read
lock.writeLock().unlock();
}
}

0 comments on commit 5d57896

Please sign in to comment.