From 222aaf76eb4e407d3f45ca9d335b2f6f2af753ab Mon Sep 17 00:00:00 2001
From: Sebastien Ponce <sponcec3@cern.ch>
Date: Tue, 12 Jun 2012 08:19:00 +0000
Subject: [PATCH] Better implementation of the CastorConfiguration class. No
inheritance from map anymore, simpler locks, and proper checks that the lock
taking succeed
---
castor/common/CastorConfiguration.cpp | 137 +++++++++++++++++---------
castor/common/CastorConfiguration.hpp | 65 ++++++------
2 files changed, 121 insertions(+), 81 deletions(-)
diff --git a/castor/common/CastorConfiguration.cpp b/castor/common/CastorConfiguration.cpp
index a77fd7b400..2b11d7ef7d 100644
--- a/castor/common/CastorConfiguration.cpp
+++ b/castor/common/CastorConfiguration.cpp
@@ -51,7 +51,7 @@ castor::common::CastorConfiguration::getConfig(std::string fileName)
// constructor
//------------------------------------------------------------------------------
castor::common::CastorConfiguration::CastorConfiguration(std::string fileName)
- throw (castor::exception::Exception) : m_fileName(fileName), m_lastUpdateTime(-1) {
+ throw (castor::exception::Exception) : m_fileName(fileName), m_lastUpdateTime(0) {
// create internal read write lock
int rc = pthread_rwlock_init(&m_lock, NULL);
if (0 != rc) {
@@ -76,69 +76,118 @@ castor::common::CastorConfiguration::getConfEnt(const std::string &category,
const std::string &key)
throw (castor::exception::Exception) {
// check whether we need to reload the configuration
- checkAndRenewConfig();
- // get read lock. No need to be fancy, just let it auto-release on exit.
- smartReadLockTaker srl(&m_lock);
- // get the category
- Configuration::const_iterator catIt = find(category);
- if (end() == catIt) {
- castor::exception::NoEntry e;
+ if (isStale()) {
+ tryToRenewConfig();
+ }
+ // get read lock
+ int rc = pthread_rwlock_rdlock(&m_lock);
+ if (0 != rc) {
+ castor::exception::Exception e(rc);
throw e;
}
// get the entry
- ConfCategory::const_iterator entIt = catIt->second.find(key);
- if (catIt->second.end() == entIt) {
- castor::exception::NoEntry e;
+ try {
+ std::map<std::string, ConfCategory>::const_iterator catIt = m_config.find(category);
+ if (m_config.end() == catIt) {
+ castor::exception::NoEntry e;
+ throw e;
+ }
+ // get the entry
+ ConfCategory::const_iterator entIt = catIt->second.find(key);
+ if (catIt->second.end() == entIt) {
+ castor::exception::NoEntry e;
+ throw e;
+ }
+ // release the lock
+ pthread_rwlock_unlock(&m_lock);
+ return entIt->second;
+ } catch (...) {
+ // release the lock
+ pthread_rwlock_unlock(&m_lock);
+ throw;
+ }
+}
+
+//------------------------------------------------------------------------------
+// isStale
+//------------------------------------------------------------------------------
+bool castor::common::CastorConfiguration::isStale()
+ throw (castor::exception::Exception) {
+ // get read lock
+ int rc = pthread_rwlock_rdlock(&m_lock);
+ if (0 != rc) {
+ castor::exception::Exception e(rc);
throw e;
}
- return entIt->second;
+ try {
+ // get the timeout
+ int timeout = getTimeoutNolock();
+ // release the lock
+ pthread_rwlock_unlock(&m_lock);
+ // return whether we should renew
+ return time(0) > m_lastUpdateTime + timeout;
+ } catch (...) {
+ // release the lock
+ pthread_rwlock_unlock(&m_lock);
+ throw;
+ }
}
//------------------------------------------------------------------------------
-// checkAndRenewConfig
+// tryToRenewConfig
//------------------------------------------------------------------------------
-void castor::common::CastorConfiguration::checkAndRenewConfig()
+void castor::common::CastorConfiguration::tryToRenewConfig()
throw (castor::exception::Exception) {
- // take read only lock
- smartReadLockTaker srl(&m_lock);
- // get the timeout
- time_t timeout = 300; // default to 300s = 5mn
- std::string &stimeout = Configuration::operator[]("Config")["ExpirationDelay"];
+ // we should probably renew. First take the write lock.
+ int rc = pthread_rwlock_wrlock(&m_lock);
+ if (0 != rc) {
+ castor::exception::Exception e(rc);
+ throw e;
+ }
+ // now check that we should really renew, because someone may have done it
+ // while we waited for the lock
+ try {
+ if (time(0) > m_lastUpdateTime + getTimeoutNolock()) {
+ // now we should really renew
+ renewConfigNolock();
+ }
+ } catch (...) {
+ // release the lock
+ pthread_rwlock_unlock(&m_lock);
+ throw;
+ }
+ // release the lock
+ pthread_rwlock_unlock(&m_lock);
+ return;
+}
+
+//------------------------------------------------------------------------------
+// getTimeoutNolock
+//------------------------------------------------------------------------------
+int castor::common::CastorConfiguration::getTimeoutNolock()
+ throw (castor::exception::Exception) {
+ // start with the default (300s = 5mn)
+ int timeout = 300;
+ // get value from config
+ std::string &stimeout = m_config["Config"]["ExpirationDelay"];
if ("" != stimeout) {
// parse the timeout into an integer
timeout = atoi(stimeout.c_str());
} else {
- // set default to 300s = 5mn into the configuration
- Configuration::operator[]("Config")["ExpirationDelay"] = "300";
- }
- // release read only lock
- srl.release();
- // check whether we should renew
- time_t currentTime = time(0);
- if (currentTime < m_lastUpdateTime + timeout) {
- // no need to renew
- return;
- }
- // we should probably renew. First take the write lock.
- // No need to be fancy, just let it auto-release on exit.
- smartWriteLockTaker swl(&m_lock);
- // now check that we should really renew, because someone may have done it
- // while we waited for the lock
- if (currentTime < m_lastUpdateTime + timeout) {
- // indeed, someone renew it for us, so give up
- return;
+ // no value found in config, set default to 300s = 5mn into the configuration
+ m_config["Config"]["ExpirationDelay"] = "300";
}
- // now we should really renew
- renewConfig();
+ // return timeout
+ return timeout;
}
//------------------------------------------------------------------------------
-// renewConfig
+// renewConfigNolock
//------------------------------------------------------------------------------
-void castor::common::CastorConfiguration::renewConfig()
+void castor::common::CastorConfiguration::renewConfigNolock()
throw (castor::exception::Exception) {
// reset the config
- clear();
+ m_config.clear();
// try to open the configuration file, throwing an exception if there is a
// failure
@@ -168,7 +217,7 @@ void castor::common::CastorConfiguration::renewConfig()
while (sline.get() == ' '){}; sline.unget(); // skip spaces
std::string value;
std::getline(sline, value, '#');
- Configuration::operator[](category)[key] = value;
+ m_config[category][key] = value;
}
m_lastUpdateTime = time(0);
}
diff --git a/castor/common/CastorConfiguration.hpp b/castor/common/CastorConfiguration.hpp
index 4e4fd15edb..6ea9fbe8e5 100644
--- a/castor/common/CastorConfiguration.hpp
+++ b/castor/common/CastorConfiguration.hpp
@@ -39,23 +39,14 @@ namespace castor {
*/
typedef std::map<std::string, std::string> ConfCategory;
- /**
- * represents a CASTOR configuration
- */
- typedef std::map<std::string, ConfCategory> Configuration;
-
/**
* a class representing the configuration of castor.
* This configurations is obtained from the local file given in the
* constructor and will be updated regularly. The time between two
* updates is taken from the Config/ExpirationDelay entry of the
* configuration itself and defaults to 5mn if no such entry is found
- *
- * Objects of this class behave like duoble level dictionnaries. This
- * means that you can look for entry A/B using obj["A"]["B"] or loop
- * over the entries of a given category using an iterator in obj["A"]
*/
- class CastorConfiguration : public Configuration {
+ class CastorConfiguration {
public:
@@ -92,16 +83,32 @@ namespace castor {
private:
/**
- * checks whether we should update our configuration.
- * If yes, do the update
+ * check whether the configuration should be renewed
+ */
+ bool isStale() throw (castor::exception::Exception);
+
+ /**
+ * tries to renew the configuration.
+ * That is : take the write lock to do it, check whether it's needed
+ * and do it only if needed before releasing the lock
*/
- void checkAndRenewConfig() throw (castor::exception::Exception);
+ void tryToRenewConfig() throw (castor::exception::Exception);
/**
- * renew the configuration. Should be called only while
- * holding the write lock
+ * gets current timeout value (in seconds)
+ * this function does not take any lock while reading the
+ * configuration. So it should never be called without holding
+ * a read or a write lock
*/
- void renewConfig() throw (castor::exception::Exception);
+ int getTimeoutNolock() throw (castor::exception::Exception);
+
+ /**
+ * renews the configuration
+ * this function does not take any lock while renewing the
+ * configuration. So it should never be called without holding
+ * the write lock
+ */
+ void renewConfigNolock() throw (castor::exception::Exception);
private:
@@ -116,29 +123,13 @@ namespace castor {
time_t m_lastUpdateTime;
/**
- * Scoped lock allowing safe lock release in all execution pathes
+ * the dictionnary of configuration items
+ * actually a dictionnary of ConfCategories, which are dictionnaries of entries
*/
- class smartLockTaker {
- protected:
- smartLockTaker(pthread_rwlock_t *lk): m_reld(false), m_lock(lk) {};
- bool m_reld;
- pthread_rwlock_t *m_lock;
- public:
- void release() { if (!m_reld) pthread_rwlock_unlock(m_lock); m_reld=true; }
- ~smartLockTaker() { release(); }
- };
-
- class smartReadLockTaker: public smartLockTaker {
- public:
- smartReadLockTaker(pthread_rwlock_t *lk): smartLockTaker(lk) { pthread_rwlock_rdlock(m_lock); }
- };
-
- class smartWriteLockTaker: public smartLockTaker {
- public:
- smartWriteLockTaker(pthread_rwlock_t *lk): smartLockTaker(lk) { pthread_rwlock_wrlock(m_lock); }
- };
+ std::map<std::string, ConfCategory> m_config;
+
/**
- * lock to garantee safe access to the configuration
+ * lock to garantee safe access to the configuration, lastUpdateTime and timeout
*/
pthread_rwlock_t m_lock;
--
GitLab