[java-oidc-common] branch main updated: Improve batch catch read write locking including various cleanups
Phil Smart
philip.smart at jisc.ac.uk
Thu Mar 10 16:19:51 UTC 2022
This is an automated email from the git hooks/post-receive script.
philsmart pushed a commit to branch main
in repository java-oidc-common.
View the commit online:
http://git.shibboleth.net/view/?p=java-oidc-common.git;a=commit;h=ae65b63f832b358ca48ecf4f80a19b584d3260a7
The following commit(s) were added to refs/heads/main by this push:
new ae65b63 Improve batch catch read write locking including various cleanups
ae65b63 is described below
commit ae65b63f832b358ca48ecf4f80a19b584d3260a7
Author: Phil Smart <philip.smart at jisc.ac.uk>
AuthorDate: Thu Mar 10 16:19:44 2022 +0000
Improve batch catch read write locking including various cleanups
---
.../metadata/cache/impl/BatchMetadataCache.java | 194 ++++++++++++---------
.../cache/impl/BatchMetadataCacheTest.java | 42 +++--
2 files changed, 146 insertions(+), 90 deletions(-)
diff --git a/oidc-common-metadata-impl/src/main/java/net/shibboleth/oidc/metadata/cache/impl/BatchMetadataCache.java b/oidc-common-metadata-impl/src/main/java/net/shibboleth/oidc/metadata/cache/impl/BatchMetadataCache.java
index 93cb2d8..32161d5 100644
--- a/oidc-common-metadata-impl/src/main/java/net/shibboleth/oidc/metadata/cache/impl/BatchMetadataCache.java
+++ b/oidc-common-metadata-impl/src/main/java/net/shibboleth/oidc/metadata/cache/impl/BatchMetadataCache.java
@@ -24,11 +24,14 @@ import java.util.Collections;
import java.util.List;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.function.Function;
import java.util.function.Predicate;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -36,6 +39,7 @@ import org.slf4j.LoggerFactory;
import net.shibboleth.oidc.metadata.BatchBackingStore;
import net.shibboleth.oidc.metadata.cache.CacheLoadingContext;
import net.shibboleth.oidc.metadata.cache.LoadingStrategy;
+import net.shibboleth.oidc.metadata.cache.MetadataCache;
import net.shibboleth.oidc.metadata.cache.MetadataCacheException;
import net.shibboleth.utilities.java.support.annotation.constraint.NonnullAfterInit;
import net.shibboleth.utilities.java.support.annotation.constraint.NonnullElements;
@@ -46,8 +50,8 @@ import net.shibboleth.utilities.java.support.logic.Constraint;
import net.shibboleth.utilities.java.support.resolver.CriteriaSet;
/**
- * A metadata cache implementation that supports 'refresh-ahead' semantics for batch cache updates.
- * Does not support 'read-through' semantics if an entry does not exist in the cache.
+ * A {@link MetadataCache metadata cache} implementation that supports 'refresh-ahead' semantics for batch
+ * cache updates. Does not support 'read-through' semantics if an entry does not exist in the cache.
*
* <p> The metadata source could either be an aggregate with more than one metadata entry, or a single source
* which contains a single metadata entry.</p>
@@ -64,10 +68,17 @@ import net.shibboleth.utilities.java.support.resolver.CriteriaSet;
* <li>If metadata expiry exists but is less than the min refresh delay, use the min refresh delay.</li>
* <li>If metadata expiry exists and is greater than the min, use the metadata expiry.</li>
* </ol>
+ *
+ * <p>Does not support:</p>
+ * <ol>
+ * <li>Manual metadata refresh. Reloads only occur via the scheduled task. Supporting manual refresh
+ * would require alterations to remove/cancel existing tasks.</li>
+ * </ol>
*
* @param <IdentifierType> the metadata identifier type.
* @param <MetadataType> the metadata type.
*/
+ at ThreadSafe
public class BatchMetadataCache<IdentifierType, MetadataType>
extends AbstractMetadataCache<IdentifierType, MetadataType> {
@@ -99,6 +110,9 @@ public class BatchMetadataCache<IdentifierType, MetadataType>
* all known metadata will be returned. Defaults to true - a match on identifier is required.
*/
@Nonnull private boolean matchOnIdentifierRequired;
+
+ /** A lock to use when reading from and loading the cache.*/
+ @Nonnull private final ReadWriteLock readWriteLock;
/**
@@ -121,6 +135,7 @@ public class BatchMetadataCache<IdentifierType, MetadataType>
@Nullable final ScheduledExecutorService executor) {
super(store, executor);
matchOnIdentifierRequired = true;
+ readWriteLock = new ReentrantReadWriteLock();
}
@Override
@@ -312,40 +327,55 @@ public class BatchMetadataCache<IdentifierType, MetadataType>
return maxRefreshDelay;
}
- //TODO we sure get does not need synchornization with loadingCache?
+ /**
+ * {@inheritDoc}
+ *
+ * <p>Acquires a read lock to prevent reading while the cache is loading.</p>
+ */
@Override @Nonnull @NonnullElements
public List<MetadataType> get(@Nonnull final CriteriaSet criteria) throws MetadataCacheException {
if (!isInitialized()) {
throw new MetadataCacheException("Metadata cache has not been initialized");
}
-
- final IdentifierType identifier = getCriteriaToIdentifierStrategy().apply(criteria);
- log.debug("{} Resolved criteria to identifier: {}", getLogPrefix(), identifier);
-
- if (identifier != null) {
-
- final List<MetadataType> allMetadata = lookupIdentifier(identifier);
- if (allMetadata.isEmpty()) {
- log.debug("{} No metadata candidates for '{}' found, returning empty result",
- getLogPrefix(), identifier);
+
+ readWriteLock.readLock().lock();
+
+ try {
+ final IdentifierType identifier = getCriteriaToIdentifierStrategy().apply(criteria);
+ log.debug("{} Resolved criteria to identifier: {}", getLogPrefix(), identifier);
+
+ if (identifier != null) {
+
+ final List<MetadataType> allMetadata = lookupIdentifier(identifier);
+ if (allMetadata.isEmpty()) {
+ log.debug("{} No metadata candidates for '{}' found, returning empty result",
+ getLogPrefix(), identifier);
+ return Collections.emptyList();
+ } else {
+ log.debug("{} There are {} metadata candidates for '{}' found in cache",
+ getLogPrefix(), allMetadata.size(), identifier);
+ return allMetadata;
+ }
+ } else if (!matchOnIdentifierRequired) {
+ log.debug("{} No identifier found to lookup, identifier match is not required, returning all known "
+ + "metadata",getLogPrefix());
+ return Collections.unmodifiableList(getBackingStore().getOrderedValues());
+ } else {
+ log.debug("{} No identifier found to lookup, returning empty result", getLogPrefix());
return Collections.emptyList();
- } else {
- log.debug("{} There are {} metadata candidates for '{}' found in cache",
- getLogPrefix(), allMetadata.size(), identifier);
- return allMetadata;
}
- } else if (!matchOnIdentifierRequired) {
- log.debug("{} No identifier found to lookup, identifier match is not required, returning all known "
- + "metadata",getLogPrefix());
- return Collections.unmodifiableList(getBackingStore().getOrderedValues());
- } else {
- log.debug("{} No identifier found to lookup, returning empty result", getLogPrefix());
- return Collections.emptyList();
+ } finally {
+ readWriteLock.readLock().unlock();
}
- // TODO: see SAML version, could resolve from criteriafrom secondary index.
+ // TODO: see SAML version, could resolve from criteria from secondary index.
}
+ /**
+ * Create a cache loading context based on the last refresh and last update time of the backing store.
+ *
+ * @return the cache loading context
+ */
private CacheLoadingContext createLoadingContext() {
return new CacheLoadingContext(getBackingStore().getLastUpdate(), getBackingStore().getLastRefresh());
}
@@ -353,69 +383,73 @@ public class BatchMetadataCache<IdentifierType, MetadataType>
/**
* Reload the entire backing-store cache using the loading strategy.
*
- * <p>Use an intrinsic object lock when loading. Although it is possible a {@link #get(CriteriaSet)} will
- * occur at the same time as this load.</p>
+ * <p>Acquires a write lock to prevent loading while the cache is being read from.</p>
*
* @throws MetadataCacheException on loading error.
*/
- //TODO lock? could be loading while a read is happening
- private synchronized void loadCache() throws MetadataCacheException{
+ private void loadCache() throws MetadataCacheException {
- log.debug("{} Populating metadata cache for '{}'",getLogPrefix(), loadingStrategy.getSourceIdentifier());
- final Instant now = Instant.now();
- Instant metadataExpiration = null;
+ readWriteLock.writeLock().lock();
try {
- if (isDestroyed()) {
- return;
- }
-
- // Any exception here is caught
- final byte[] rawFetchedMetadata = loadingStrategy.load(createLoadingContext());
- if (rawFetchedMetadata != null) {
- if (sourceMetadataValidPredicate.test(rawFetchedMetadata)) {
-
- final List<MetadataType> parsedMetadata = parsingStrategy.apply(rawFetchedMetadata);
- if (parsedMetadata != null && !parsedMetadata.isEmpty()) {
- log.info("{} Parsed {} metadata candidates, loading into cache",
- getLogPrefix(), parsedMetadata.size());
- freshLoad(parsedMetadata);
- // Store away the original, raw, metadata bytes.
- getBackingStore().setOriginalValue(rawFetchedMetadata);
- // Set last update time, technically there is no guarantee the metadata was stored correctly
- // at this point.
- getBackingStore().setLastUpdate(now);
- }
-
- // Compute metadata expiration from whatever is in the cache (updated or not) will
- // remain null if no cached original value.
- metadataExpiration = sourceMetadataExpiryStrategy.apply(getBackingStore().getOriginalValue());
+ log.debug("{} Populating metadata cache for '{}'",getLogPrefix(), loadingStrategy.getSourceIdentifier());
+ final Instant now = Instant.now();
+ Instant metadataExpiration = null;
+ try {
+ if (isDestroyed()) {
+ return;
+ }
+
+ // Any exception here is caught
+ final byte[] rawFetchedMetadata = loadingStrategy.load(createLoadingContext());
+ if (rawFetchedMetadata != null) {
+ if (sourceMetadataValidPredicate.test(rawFetchedMetadata)) {
+
+ final List<MetadataType> parsedMetadata = parsingStrategy.apply(rawFetchedMetadata);
+ if (parsedMetadata != null && !parsedMetadata.isEmpty()) {
+ log.info("{} Parsed {} metadata candidates, loading into cache",
+ getLogPrefix(), parsedMetadata.size());
+ freshLoad(parsedMetadata);
+ // Store away the original, raw, metadata bytes.
+ getBackingStore().setOriginalValue(rawFetchedMetadata);
+ // Set last update time, technically there is no guarantee the metadata was stored correctly
+ // at this point.
+ getBackingStore().setLastUpdate(now);
+ }
+
+ // Compute metadata expiration from whatever is in the cache (updated or not) will
+ // remain null if no cached original value.
+ metadataExpiration = sourceMetadataExpiryStrategy.apply(getBackingStore().getOriginalValue());
+ } else {
+ // Metadata is not valid
+ log.warn("{} Source metadata is not valid, nothing to load", getLogPrefix());
+ //TODO do we do anything else here, or just let it tick over and try again
+ // on the next refresh cycle
+ }
} else {
- // Metadata is not valid
- log.warn("{} Source metadata is not valid, nothing to load", getLogPrefix());
- //TODO MUST FINISH THIS !!
+ log.info("{} Metadata has not changed since last refresh", getLogPrefix());
}
- } else {
- log.info("{} Metadata has not changed since last refresh", getLogPrefix());
- }
-
- } catch (final Throwable t) {
- log.error("{} Error loading or parsing metadata",getLogPrefix(), t);
- if (t instanceof Exception) {
- throw new MetadataCacheException((Exception) t);
- } else {
- throw new MetadataCacheException(String.format("Saw an error of type '%s' with message '%s'",
- t.getClass().getName(), t.getMessage()));
- }
- } finally {
- if (metadataExpiration == null || metadataExpiration.isBefore(now)) {
- // Null, so forced to use max refresh delay.
- scheduleNextRefresh(null);
- } else {
- final Duration nextRefreshDelay = computeNextRefreshDelay(metadataExpiration);
- scheduleNextRefresh(nextRefreshDelay);
+
+ } catch (final Throwable t) {
+ log.error("{} Error loading or parsing metadata",getLogPrefix(), t);
+ if (t instanceof Exception) {
+ throw new MetadataCacheException((Exception) t);
+ } else {
+ throw new MetadataCacheException(String.format("Saw an error of type '%s' with message '%s'",
+ t.getClass().getName(), t.getMessage()));
+ }
+ } finally {
+ if (metadataExpiration == null || metadataExpiration.isBefore(now)) {
+ // Null, so forced to use max refresh delay.
+ scheduleNextRefresh(null);
+ } else {
+ final Duration nextRefreshDelay = computeNextRefreshDelay(metadataExpiration);
+ scheduleNextRefresh(nextRefreshDelay);
+ }
+ // Set last attempted refresh even if failure.
+ getBackingStore().setLastRefresh(now);
}
- // Set last attempted refresh even if failure.
- getBackingStore().setLastRefresh(now);
+ } finally {
+ readWriteLock.writeLock().unlock();
}
}
diff --git a/oidc-common-metadata-impl/src/test/java/net/shibboleth/oidc/metadata/cache/impl/BatchMetadataCacheTest.java b/oidc-common-metadata-impl/src/test/java/net/shibboleth/oidc/metadata/cache/impl/BatchMetadataCacheTest.java
index d0d4807..8c0a0e4 100644
--- a/oidc-common-metadata-impl/src/test/java/net/shibboleth/oidc/metadata/cache/impl/BatchMetadataCacheTest.java
+++ b/oidc-common-metadata-impl/src/test/java/net/shibboleth/oidc/metadata/cache/impl/BatchMetadataCacheTest.java
@@ -139,7 +139,7 @@ public class BatchMetadataCacheTest {
localCache.setParsingStrategy(defaultParsingStrategy);
localCache.setLoadingStrategy(defaultLoadingStrategy);
- localCache.setSourceMetadataExpiryStrategy(b -> Instant.now().plus(Duration.ofMinutes(5)));
+ localCache.setSourceMetadataExpiryStrategy(b -> Instant.now().plus(Duration.ofSeconds(1)));
localCache.setIdentifierExtractionStrategy(OIDCProviderMetadata::getIssuer);
localCache.setMinRefreshDelay(Duration.ofMillis(100));
localCache.setMaxRefreshDelay(Duration.ofMillis(200));
@@ -286,7 +286,7 @@ public class BatchMetadataCacheTest {
}
@Test
- public void testMetadataExpiryBelowMinDelay_UseMinDelay() throws ComponentInitializationException, MetadataCacheException {
+ public void testMetadataExpiryBelowMinDelay_UseMinDelay() throws Exception {
cache.setMinRefreshDelay(Duration.ofMinutes(10));
// Set MD expiry in 1 minute which is below the min refresh delay.
@@ -354,8 +354,7 @@ public class BatchMetadataCacheTest {
}
@Test
- public void testNoUsableIdentifierInCriteria_EmptyList() throws ComponentInitializationException,
- InterruptedException, MetadataCacheException {
+ public void testNoUsableIdentifierInCriteria_EmptyList() throws Exception {
cache.initialize();
// Strategy does not accept entityID criterion, so no identifier returned. Hence
// no results
@@ -366,7 +365,7 @@ public class BatchMetadataCacheTest {
}
@Test
- public void testGetNotCached_Success() throws MetadataCacheException, ComponentInitializationException {
+ public void testGetNotCached_Success() throws Exception {
cache.initialize();
final List<OIDCProviderMetadata> metadata =
cache.get(new CriteriaSet(new IssuerIDCriterion(new Issuer("http://www.example.org"))));
@@ -384,8 +383,8 @@ public class BatchMetadataCacheTest {
}
- @Test(enabled=false)
- public void testSourceNotValid_EntryAllreadyExist_Success() throws Exception {
+ @Test
+ public void testSourceNotValidMetadaNotValid_EntryAllreadyExist_Success() throws Exception {
final Issuer iss = new Issuer("http://www.example.org");
// Add an entry
@@ -394,11 +393,13 @@ public class BatchMetadataCacheTest {
new URI("http://www.example.org/metadata"))));
cache.setSourceMetadataValidPredicate(Predicates.alwaysFalse());
+ cache.setMetadataValidPredicate(Predicates.alwaysFalse());
cache.initialize();
final List<OIDCProviderMetadata> metadata =
cache.get(new CriteriaSet(new IssuerIDCriterion(iss)));
assertTrue(metadata.isEmpty() == true);
}
+
@Test
public void testGetNotCached_RefreshAHead_Success()
@@ -410,14 +411,35 @@ public class BatchMetadataCacheTest {
assertTrue(metadata.isEmpty() == false);
}
+
+ /* Ensure a get does not interfere with a load. Not for CI, but for help testing concurrency. */
+ @Test(enabled=false)
+ public void testLoadGetRaceCondition() throws Exception {
+ cache.initialize();
+
+ final ExecutorService service = Executors.newFixedThreadPool(3);
+ final Future<?> futureTwo = service.submit(() -> cache.get(
+ new CriteriaSet(new IssuerIDCriterion(new Issuer("http://www.example.org")))));
+ final Future<?> futureThree = service.submit(() -> cache.get(
+ new CriteriaSet(new IssuerIDCriterion(new Issuer("http://www.example.org")))));
+ final Future<?> futureOne = service.submit(() -> {try {
+ Thread.sleep(5000);
+ } catch (final InterruptedException e) {
+ //do nothing
+ } scheduler.triggerScheduledTasks();
+ });
+
+ Thread.sleep(10000);
+
+ }
@Test
- public void testGetAndLoad_Success()
- throws ComponentInitializationException, InterruptedException, ExecutionException {
+ public void testGetAndLoad_Success()throws Exception {
cache.initialize();
final ExecutorService service = Executors.newFixedThreadPool(3);
- final Future<?> futureOne = service.submit(() -> cache.get(new CriteriaSet(new IssuerIDCriterion(new Issuer("http://www.example.org")))));
+ final Future<?> futureOne = service.submit(() -> cache.get(new CriteriaSet(
+ new IssuerIDCriterion(new Issuer("http://www.example.org")))));
scheduler.triggerScheduledTasks();
final List<?> firstMetadata = (List<?>) futureOne.get();
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list