[cpp-opensaml] 02/02: SSPCPP-756 - Dynamic Metadata Provider cleanup
Scott Cantor
cantor.2 at osu.edu
Thu May 17 19:14:28 EDT 2018
This is an automated email from the git hooks/post-receive script.
scantor pushed a commit to branch master
in repository cpp-opensaml.
View the commit online:
http://git.shibboleth.net/view/?p=cpp-opensaml.git;a=commit;h=69a116df430e40184796aa3258f5f8524eba95e6
commit 69a116df430e40184796aa3258f5f8524eba95e6
Author: Scott Cantor <cantor.2 at osu.edu>
AuthorDate: Thu May 17 19:12:28 2018 -0400
SSPCPP-756 - Dynamic Metadata Provider cleanup
https://issues.shibboleth.net/jira/browse/SSPCPP-756
Improve caching and hopefully fix a number of race conditions.
---
.../metadata/AbstractDynamicMetadataProvider.h | 30 ++++-
.../impl/AbstractDynamicMetadataProvider.cpp | 132 ++++++++++++++-------
.../metadata/impl/LocalDynamicMetadataProvider.cpp | 55 +++++++--
saml/saml2/metadata/impl/NullMetadataProvider.cpp | 4 +-
4 files changed, 162 insertions(+), 59 deletions(-)
diff --git a/saml/saml2/metadata/AbstractDynamicMetadataProvider.h b/saml/saml2/metadata/AbstractDynamicMetadataProvider.h
index 66d043f..ec90e90 100644
--- a/saml/saml2/metadata/AbstractDynamicMetadataProvider.h
+++ b/saml/saml2/metadata/AbstractDynamicMetadataProvider.h
@@ -68,25 +68,45 @@ namespace opensaml {
/**
* Resolves a metadata instance using the supplied criteria.
*
+ * <p>A null return value indicates the instance hasn't changed since the prevous request
+ * for the same instance.</p>
+ *
+ * <p>The cache tag may be modified on output to update it for future calls.</p>
+ *
* @param criteria lookup criteria
- * @return a valid metadata instance (never nullptr)
+ * @param cacheTag implementation specific cache tag
+ *
+ * @return a valid metadata instance or null
* @throws an exception if resolution failed
*/
- virtual EntityDescriptor* resolve(const Criteria& criteria) const = 0;
+ virtual EntityDescriptor* resolve(const Criteria& criteria, std::string& cacheTag) const=0;
/**
* Index an entity and cache the fact of it being indexed.
*
* @param entity what to cache
- * @param locked have we locked ourself exclusive first?
+ * @param cacheTag cache tag
+ * @param locked have we locked ourselves exclusively first?
+ *
* @return the cache ttl (for logging purposes)
*/
- virtual time_t cacheEntity(EntityDescriptor* entity, bool locked = false) const;
+ virtual time_t cacheEntity(EntityDescriptor* entity, const std::string& cacheTag, bool locked=false) const;
+
+ /**
+ * Compute the number of seconds until the next refresh attempt.
+ *
+ * @param entity entity to evaluate
+ * @param currentTime baseline for calculation
+ *
+ * @return the cache ttl
+ */
+ time_t computeNextRefresh(const EntityDescriptor& entity, time_t currentTime) const;
/**
* Parse and unmarshal the provided stream, returning the EntityDescriptor if there is one.
*
* @param stream the stream to parse
+ *
* @return the entity, or nullptr if there isn't one
*/
EntityDescriptor* entityFromStream(std::istream& stream) const;
@@ -97,7 +117,7 @@ namespace opensaml {
boost::scoped_ptr<xmltooling::RWLock> m_lock;
double m_refreshDelayFactor;
time_t m_minCacheDuration, m_maxCacheDuration;
- typedef std::map<xmltooling::xstring,time_t> cachemap_t;
+ typedef std::map< xmltooling::xstring, std::pair<time_t,std::string> > cachemap_t;
mutable cachemap_t m_cacheMap;
bool m_negativeCache;
diff --git a/saml/saml2/metadata/impl/AbstractDynamicMetadataProvider.cpp b/saml/saml2/metadata/impl/AbstractDynamicMetadataProvider.cpp
index f6a642c..3bb0f67 100644
--- a/saml/saml2/metadata/impl/AbstractDynamicMetadataProvider.cpp
+++ b/saml/saml2/metadata/impl/AbstractDynamicMetadataProvider.cpp
@@ -85,11 +85,18 @@ AbstractDynamicMetadataProvider::AbstractDynamicMetadataProvider(bool defaultNeg
m_cleanupInterval(XMLHelper::getAttrInt(e, 1800, cleanupInterval)),
m_cleanupTimeout(XMLHelper::getAttrInt(e, 1800, cleanupTimeout))
{
- if (m_minCacheDuration > m_maxCacheDuration) {
- Category::getInstance(SAML_LOGCAT ".MetadataProvider.Dynamic").error(
- "minCacheDuration setting exceeds maxCacheDuration setting, lowering to match it"
+ if (m_minCacheDuration < 30) {
+ Category::getInstance(SAML_LOGCAT ".MetadataProvider.Dynamic").warn(
+ "minCacheDuration setting must be at least 30 seconds, raising to 30"
+ );
+ m_minCacheDuration = 30;
+ }
+
+ if (m_maxCacheDuration < m_minCacheDuration) {
+ Category::getInstance(SAML_LOGCAT ".MetadataProvider.Dynamic").warn(
+ "maxCacheDuration setting is less than minCacheDuration setting, raising to match it"
);
- m_minCacheDuration = m_maxCacheDuration;
+ m_maxCacheDuration = m_minCacheDuration;
}
const XMLCh* delay = e ? e->getAttributeNS(nullptr, refreshDelayFactor) : nullptr;
@@ -97,7 +104,7 @@ AbstractDynamicMetadataProvider::AbstractDynamicMetadataProvider(bool defaultNeg
auto_ptr_char temp(delay);
m_refreshDelayFactor = atof(temp.get());
if (m_refreshDelayFactor <= 0.0 || m_refreshDelayFactor >= 1.0) {
- Category::getInstance(SAML_LOGCAT ".MetadataProvider.Dynamic").error(
+ Category::getInstance(SAML_LOGCAT ".MetadataProvider.Dynamic").warn(
"invalid refreshDelayFactor setting, using default"
);
m_refreshDelayFactor = 0.75;
@@ -164,9 +171,9 @@ void* AbstractDynamicMetadataProvider::cleanup_fn(void* pv)
time_t now = time(nullptr);
// Dual iterator loop so we can remove entries while walking the map.
- for (map<xstring, time_t>::iterator i = provider->m_cacheMap.begin(), i2 = i; i != provider->m_cacheMap.end(); i = i2) {
+ for (cachemap_t::iterator i = provider->m_cacheMap.begin(), i2 = i; i != provider->m_cacheMap.end(); i = i2) {
++i2;
- if (now > i->second + provider->m_cleanupTimeout) {
+ if (now > i->second.first + provider->m_cleanupTimeout) {
if (log.isDebugEnabled()) {
auto_ptr_char id(i->first.c_str());
log.debug("removing cache entry for (%s)", id.get());
@@ -240,7 +247,7 @@ pair<const EntityDescriptor*,const RoleDescriptor*> AbstractDynamicMetadataProvi
cit = m_cacheMap.end();
}
if (cit != m_cacheMap.end()) {
- if (time(nullptr) <= cit->second)
+ if (time(nullptr) <= cit->second.first)
return entity;
}
@@ -264,29 +271,61 @@ pair<const EntityDescriptor*,const RoleDescriptor*> AbstractDynamicMetadataProvi
else
log.info("resolving metadata for (%s)", name.c_str());
+ string cacheTag(cit != m_cacheMap.end() ? cit->second.second : "");
+
try {
// Try resolving it.
- auto_ptr<EntityDescriptor> entity2(resolve(criteria));
+ auto_ptr<EntityDescriptor> entity2(resolve(criteria, cacheTag));
+
+ // A null here means we probably already have metadata in place and should reuse it.
+ // If we don't, then that means we did at one time, but it's now invalid (but nothing
+ // newer is apparently available).
+ if (!entity2.get()) {
+ if (entity.first) {
+ log.info("metadata for (%s) is unchanged, resetting next refresh time", name.c_str());
+
+ time_t now = time(nullptr);
+ time_t cacheExp = computeNextRefresh(*entity.first, now);
+ xstring key(entity.first->getEntityID());
+
+ pair<time_t,string> oldValues = cit != m_cacheMap.end() ? cit->second : pair<time_t,string>(0, string());
+
+ // Elevate to write lock.
+ m_lock->unlock();
+ m_lock->wrlock();
+ writeLocked = true;
+
+ // Update cache map if nothing got in behind us.
+ cit = m_cacheMap.find(key);
+ if (cit != m_cacheMap.end() && cit->second == oldValues) {
+ cit->second.first = now + cacheExp;
+ }
+
+ // Downgrade back to a read lock.
+ m_lock->unlock();
+ m_lock->rdlock();
+
+ // Rinse and repeat.
+ return getEntityDescriptor(criteria);
+ }
+ throw MetadataException("No updated metadata available to refresh invalid instance.");
+ }
// Verify the entityID.
if (criteria.entityID_unicode && !XMLString::equals(criteria.entityID_unicode, entity2->getEntityID())) {
- log.error("metadata instance did not match expected entityID");
- return entity;
+ throw MetadataException("Metadata instance did not match expected entityID.");
}
else if (criteria.artifact) {
auto_ptr_char temp2(entity2->getEntityID());
const string hashed(SecurityHelper::doHash("SHA1", temp2.get(), strlen(temp2.get()), true));
- if (hashed != name) {
- log.error("metadata instance did not match expected entityID");
- return entity;
- }
+ if (hashed != name)
+ throw MetadataException("Metadata instance did not match expected entityID.");
+
}
else {
auto_ptr_XMLCh temp2(name.c_str());
- if (!XMLString::equals(temp2.get(), entity2->getEntityID())) {
- log.error("metadata instance did not match expected entityID");
- return entity;
- }
+ if (!XMLString::equals(temp2.get(), entity2->getEntityID()))
+ throw MetadataException("Metadata instance did not match expected entityID.");
}
// Preprocess the metadata (even if we schema-validated).
@@ -318,7 +357,7 @@ pair<const EntityDescriptor*,const RoleDescriptor*> AbstractDynamicMetadataProvi
// Notify observers.
emitChangeEvent(*entity2);
- time_t cacheExp = cacheEntity(entity2.get(), true);
+ time_t cacheExp = cacheEntity(entity2.get(), cacheTag, true);
entity2.release();
log.info("next refresh of metadata for (%s) no sooner than %lu seconds", name.c_str(), cacheExp);
@@ -330,23 +369,27 @@ pair<const EntityDescriptor*,const RoleDescriptor*> AbstractDynamicMetadataProvi
if (m_negativeCache) {
// This will return entries that are beyond their cache period,
// but not beyond their validity unless that criteria option was set.
- // Bump the cache period to prevent retries, making sure we have a write lock
+ // Bump the cache period to prevent retries, making sure we have a write lock.
if (!writeLocked) {
m_lock->unlock();
m_lock->wrlock();
writeLocked = true;
}
- if (entity.first)
- m_cacheMap[entity.first->getEntityID()] = time(nullptr) + m_minCacheDuration;
- else if (criteria.entityID_unicode)
- m_cacheMap[criteria.entityID_unicode] = time(nullptr) + m_minCacheDuration;
+
+ if (criteria.entityID_unicode) {
+ m_cacheMap[criteria.entityID_unicode] = make_pair(time(nullptr) + m_minCacheDuration, cacheTag);
+ }
else {
auto_ptr_XMLCh widetemp(name.c_str());
- m_cacheMap[widetemp.get()] = time(nullptr) + m_minCacheDuration;
+ m_cacheMap[widetemp.get()] = make_pair(time(nullptr) + m_minCacheDuration, cacheTag);
}
- log.warn("next refresh of metadata for (%s) no sooner than %u seconds", name.c_str(), m_minCacheDuration);
+ log.warn("next refresh of metadata for (%s) no sooner than %lu seconds", name.c_str(), m_minCacheDuration);
+ }
+ else {
+ // With no negative caching, we can viably return the search result directly
+ // because we don't open a lock window that could invalidate the objects.
+ return entity;
}
- return entity;
}
// Downgrade back to a read lock.
@@ -359,18 +402,33 @@ pair<const EntityDescriptor*,const RoleDescriptor*> AbstractDynamicMetadataProvi
return getEntityDescriptor(criteria);
}
-time_t AbstractDynamicMetadataProvider::cacheEntity(EntityDescriptor* entity, bool writeLocked) const
+time_t AbstractDynamicMetadataProvider::cacheEntity(EntityDescriptor* entity, const string& cacheTag, bool writeLocked) const
{
- time_t now = time(nullptr);
if (!writeLocked) {
m_lock->wrlock();
}
Locker locker(writeLocked ? nullptr : const_cast<AbstractDynamicMetadataProvider*>(this), false);
+ time_t now = time(nullptr);
+ time_t cacheExp = computeNextRefresh(*entity, now);
+
+ // Record the proper refresh time and cache tag.
+ m_cacheMap[entity->getEntityID()] = make_pair(now + cacheExp, cacheTag);
+
+ // Make sure we clear out any existing copies, including stale metadata or if somebody snuck in.
+ unindex(entity->getEntityID(), true); // actually frees the old instance with this ID
+ time_t exp(SAMLTIME_MAX);
+ indexEntity(entity, exp);
+
+ return cacheExp;
+}
+
+time_t AbstractDynamicMetadataProvider::computeNextRefresh(const EntityDescriptor& entity, time_t currentTime) const
+{
// Compute the smaller of the validUntil / cacheDuration constraints.
- time_t cacheExp = (entity->getValidUntil() ? entity->getValidUntilEpoch() : SAMLTIME_MAX) - now;
- if (entity->getCacheDuration())
- cacheExp = min(cacheExp, entity->getCacheDurationEpoch());
+ time_t cacheExp = (entity.getValidUntil() ? entity.getValidUntilEpoch() : SAMLTIME_MAX) - currentTime;
+ if (entity.getCacheDuration())
+ cacheExp = min(cacheExp, entity.getCacheDurationEpoch());
// Adjust for the delay factor.
cacheExp *= m_refreshDelayFactor;
@@ -381,14 +439,6 @@ time_t AbstractDynamicMetadataProvider::cacheEntity(EntityDescriptor* entity, bo
else if (cacheExp < m_minCacheDuration)
cacheExp = m_minCacheDuration;
- // Record the proper refresh time.
- m_cacheMap[entity->getEntityID()] = now + cacheExp;
-
- // Make sure we clear out any existing copies, including stale metadata or if somebody snuck in.
- unindex(entity->getEntityID(), true); // actually frees the old instance with this ID
- time_t exp(SAMLTIME_MAX);
- indexEntity(entity, exp);
-
return cacheExp;
}
diff --git a/saml/saml2/metadata/impl/LocalDynamicMetadataProvider.cpp b/saml/saml2/metadata/impl/LocalDynamicMetadataProvider.cpp
index b841f90..93bf22f 100644
--- a/saml/saml2/metadata/impl/LocalDynamicMetadataProvider.cpp
+++ b/saml/saml2/metadata/impl/LocalDynamicMetadataProvider.cpp
@@ -25,16 +25,18 @@
*/
#include <fstream>
+#include <boost/lexical_cast.hpp>
#include <boost/algorithm/string.hpp>
-
#include "internal.h"
#include <xmltooling/logging.h>
+#include <xmltooling/XMLToolingConfig.h>
#include <xmltooling/security/SecurityHelper.h>
+#include <xmltooling/util/PathResolver.h>
+#include <xmltooling/util/XMLHelper.h>
#include <binding/SAMLArtifact.h>
-#include <saml2/metadata/Metadata.h>
#include <saml2/metadata/AbstractDynamicMetadataProvider.h>
@@ -66,11 +68,11 @@ namespace opensaml {
void init() {};
protected:
- virtual EntityDescriptor* resolve(const Criteria& criteria) const;
+ virtual EntityDescriptor* resolve(const Criteria& criteria, string& cacheTag) const;
private:
- string m_sourceDirectory;
Category& m_log;
+ string m_sourceDirectory;
};
MetadataProvider* SAML_DLLLOCAL LocalDynamicMetadataProviderFactory(const DOMElement* const & e)
@@ -82,17 +84,19 @@ namespace opensaml {
LocalDynamicMetadataProvider::LocalDynamicMetadataProvider(const DOMElement* e)
: MetadataProvider(e), AbstractDynamicMetadataProvider(false, e),
- m_sourceDirectory(XMLHelper::getAttrString(e, nullptr, sourceDirectory)),
- m_log(Category::getInstance(SAML_LOGCAT ".MetadataProvider.LocalDynamic"))
+ m_log(Category::getInstance(SAML_LOGCAT ".MetadataProvider.LocalDynamic")),
+ m_sourceDirectory(XMLHelper::getAttrString(e, nullptr, sourceDirectory))
{
if (m_sourceDirectory.empty())
throw MetadataException("LocalDynamicMetadataProvider: sourceDirectory=\"whatever\" must be present");
+ XMLToolingConfig::getConfig().getPathResolver()->resolve(m_sourceDirectory, PathResolver::XMLTOOLING_CFG_FILE);
+
if (!boost::algorithm::ends_with(m_sourceDirectory, "/"))
m_sourceDirectory += '/';
}
-EntityDescriptor* LocalDynamicMetadataProvider::resolve(const Criteria& criteria) const
+EntityDescriptor* LocalDynamicMetadataProvider::resolve(const Criteria& criteria, string& cacheTag) const
{
string name, from;
if (criteria.entityID_ascii) {
@@ -101,8 +105,8 @@ EntityDescriptor* LocalDynamicMetadataProvider::resolve(const Criteria& criteria
}
else if (criteria.entityID_unicode) {
auto_ptr_char temp(criteria.entityID_unicode);
- from = criteria.entityID_ascii;
- SecurityHelper::doHash("SHA1", from.c_str(), from.length());
+ from = temp.get();
+ name = SecurityHelper::doHash("SHA1", from.c_str(), from.length());
}
else if (criteria.artifact) {
from = name = criteria.artifact->getSource();
@@ -110,14 +114,43 @@ EntityDescriptor* LocalDynamicMetadataProvider::resolve(const Criteria& criteria
name = m_sourceDirectory + name + ".xml";
m_log.debug("transformed name from (%s) to (%s)", from.c_str(), name.c_str());
+ time_t lastaccess;
+#ifdef WIN32
+ struct _stat stat_buf;
+ if (_stat(name.c_str(), &stat_buf) == 0)
+#else
+ struct stat stat_buf;
+ if (stat(name.c_str(), &stat_buf) == 0)
+#endif
+ lastaccess = stat_buf.st_mtime;
+ else
+ throw IOException("Unable to access local file ($1)", params(1, name.c_str()));
+
+ // Note that we're at minimum under a read lock here overall, which precludes the cleanup
+ // thread in the base class from running a cleanup pass, and potentially invalidating
+ // state during this evaluation. That should prevent a race condition where we determine
+ // no update is needed but the original copy is purged before the query finishes.
+
+ try {
+ string newCacheTag = boost::lexical_cast<string>(lastaccess);
+ if (cacheTag == newCacheTag)
+ return nullptr;
+ cacheTag = newCacheTag;
+ }
+ catch (const boost::bad_lexical_cast& e) {
+ m_log.error("exception converting between cache tag and access time: %s", e.what());
+ cacheTag = "";
+ }
+
ifstream source(name.c_str());
if (!source) {
- m_log.debug("local metadata file (%s) not found for input (%s)", name.c_str(), from.c_str());
- throw IOException("Local metadata file not found.");
+ m_log.debug("local metadata file (%s) not accessible for input (%s)", name.c_str(), from.c_str());
+ throw IOException("Unable to access local file ($1)", params(1, name.c_str()));
}
EntityDescriptor* result = entityFromStream(source);
if (!result)
throw MetadataException("No entity resolved from file."); // shouldn't happen
+
return result;
}
diff --git a/saml/saml2/metadata/impl/NullMetadataProvider.cpp b/saml/saml2/metadata/impl/NullMetadataProvider.cpp
index c1c052e..b579022 100644
--- a/saml/saml2/metadata/impl/NullMetadataProvider.cpp
+++ b/saml/saml2/metadata/impl/NullMetadataProvider.cpp
@@ -52,7 +52,7 @@ namespace opensaml {
void init() {}
protected:
- EntityDescriptor* resolve(const MetadataProvider::Criteria& criteria) const;
+ EntityDescriptor* resolve(const MetadataProvider::Criteria& criteria, string& cacheTag) const;
private:
scoped_ptr<EntityDescriptor> m_template;
@@ -65,7 +65,7 @@ namespace opensaml {
};
};
-EntityDescriptor* NullMetadataProvider::resolve(const MetadataProvider::Criteria& criteria) const
+EntityDescriptor* NullMetadataProvider::resolve(const MetadataProvider::Criteria& criteria, string& cacheTag) const
{
// Resolving for us just means fabricating a new dummy element.
EntityDescriptor* entity = m_template.get() ? m_template->cloneEntityDescriptor() : EntityDescriptorBuilder::buildEntityDescriptor();
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list