[asterisk-scf-commits] asterisk-scf/integration/logger.git branch "ref-counting" updated.
Commits to the Asterisk SCF project code repositories
asterisk-scf-commits at lists.digium.com
Fri Dec 10 14:21:14 CST 2010
branch "ref-counting" has been updated
via 66f54f56f2140c6cc5af23271eb0bb2b87481326 (commit)
via 0ea05261438b104b83a27ec2d48f3f4105fdf2e1 (commit)
from 9f04ff8250a49f6185a58c30f681427f26b617ff (commit)
Summary of changes:
client/src/Logger.cpp | 59 +++++------
client/src/LoggerFactory.cpp | 16 ++--
client/src/logger.h | 196 +++++++++++++++++++++++++++++++-----
client/test/LoggerFactory-test.cpp | 26 +----
4 files changed, 214 insertions(+), 83 deletions(-)
- Log -----------------------------------------------------------------
commit 66f54f56f2140c6cc5af23271eb0bb2b87481326
Author: David M. Lee <dlee at digium.com>
Date: Fri Dec 10 14:18:52 2010 -0600
Changed Logger.mParent from raw ptr to weak_ptr.
Raw pointers had the danger of dangling, since there's no longer any
guarantee that parents will kill their children before dying (as
gruesome as that sounds). The use of weak_ptr's makes that safe, while
avoiding memory leaks that would result from circular shared_ptr
references.
diff --git a/client/src/Logger.cpp b/client/src/Logger.cpp
index 2c1de2c..c9e6fec 100644
--- a/client/src/Logger.cpp
+++ b/client/src/Logger.cpp
@@ -63,16 +63,18 @@ void LogBuf::sendBuffer()
}
Logger::LoggerImpl::LoggerImpl(const std::string& name, LogOut& out, Level logLevel) :
- mParent(0), mName(name), mOut(&out), mLogLevel(logLevel), mInheritedLevel(false)
+ mParent(), mName(name), mOut(&out), mLogLevel(logLevel), mInheritedLevel(false)
{
}
-Logger::LoggerImpl::LoggerImpl(const LoggerImpl& parent, const std::string& name) :
- mParent(&parent), mName(name), mOut(parent.mOut), mLogLevel(Off),
+Logger::LoggerImpl::LoggerImpl(const boost::shared_ptr<LoggerImpl>& parent, const std::string& name) :
+ mParent(parent), mName(name), mOut(parent->mOut), mLogLevel(Off),
mInheritedLevel(true)
{
+ // parent ptr must be non-null
+ assert(parent != 0);
// our name must begin w/ parent's name
- assert(name.find(parent.mName) == 0);
+ assert(name.find(parent->mName) == 0);
}
Logger::LoggerImpl::~LoggerImpl()
@@ -131,10 +133,13 @@ void Logger::LoggerImpl::unsetLevel()
Level Logger::LoggerImpl::getEffectiveLevel() const
{
+ // If parent is null, either we've never had a parent, or that
+ // parent has been destroyed.
+ boost::shared_ptr<const LoggerImpl> parent = mParent.lock();
// if our level is unset, inherit level from our parent.
- if (mInheritedLevel == true && mParent != 0)
+ if (mInheritedLevel == true && parent != 0)
{
- return mParent->getEffectiveLevel();
+ return parent->getEffectiveLevel();
}
else
{
@@ -142,7 +147,8 @@ Level Logger::LoggerImpl::getEffectiveLevel() const
}
}
-Logger Logger::LoggerImpl::getChild(const std::string& childName)
+Logger Logger::LoggerImpl::getChild(const boost::shared_ptr<LoggerImpl>& self,
+ const std::string& childName)
{
IceUtil::Mutex::Lock childLock(mChildrenMutex);
@@ -152,7 +158,7 @@ Logger Logger::LoggerImpl::getChild(const std::string& childName)
{
std::string fullName = getName().empty() ? childName : getName() + "."
+ childName;
- child.reset(new LoggerImpl(*this, fullName));
+ child.reset(new LoggerImpl(self, fullName));
}
return Logger(child);
}
diff --git a/client/src/logger.h b/client/src/logger.h
index 13225a2..bbfd26e 100644
--- a/client/src/logger.h
+++ b/client/src/logger.h
@@ -20,6 +20,7 @@
#include <cstdarg>
#include <boost/shared_ptr.hpp>
+#include <boost/weak_ptr.hpp>
#include "System/Logger/LoggerIf.h"
#include "Level.h"
@@ -159,11 +160,6 @@ public:
Logger(const std::string& name, LogOut& out, Level logLevel = Debug);
/**
- * Construct a child Logger.
- */
- Logger(const LoggerImpl& parent, const std::string& name);
-
- /**
* If true, this Logger would log messages of the given Level.
*
* @param level Level to check.
@@ -230,12 +226,14 @@ public:
/**
* Construct a root Logger.
*/
- ASTERISK_SCF_ICEBOX_EXPORT LoggerImpl(const std::string& name, LogOut& out, Level logLevel = Debug);
+ ASTERISK_SCF_ICEBOX_EXPORT LoggerImpl(const std::string& name, LogOut& out,
+ Level logLevel = Debug);
/**
* Construct a child Logger.
*/
- ASTERISK_SCF_ICEBOX_EXPORT LoggerImpl(const LoggerImpl& parent, const std::string& name);
+ ASTERISK_SCF_ICEBOX_EXPORT LoggerImpl(
+ const boost::shared_ptr<LoggerImpl>& parent, const std::string& name);
ASTERISK_SCF_ICEBOX_EXPORT ~LoggerImpl();
@@ -268,7 +266,8 @@ public:
*/
ASTERISK_SCF_ICEBOX_EXPORT void vlogf(Level level, char const *fmt, va_list ap) const;
- ASTERISK_SCF_ICEBOX_EXPORT Logger getChild(const std::string& childName);
+ ASTERISK_SCF_ICEBOX_EXPORT Logger getChild(
+ const boost::shared_ptr<LoggerImpl>& self, const std::string& childName);
ASTERISK_SCF_ICEBOX_EXPORT std::vector<Logger> getChildren() const;
const std::string& getName() const
@@ -312,8 +311,13 @@ private:
/**
* Parent pointer. We cannot change which parent we have, nor can we change
* our parent.
+ *
+ * This can't be a shared_ptr, because then the circular reference would
+ * cause memory leaks.
+ *
+ * It also can't be a raw pointer, since children can outlive their parents.
*/
- LoggerImpl const * const mParent;
+ boost::weak_ptr<LoggerImpl const> const mParent;
typedef std::map<std::string, boost::shared_ptr<LoggerImpl> > Children;
/**
* Map of children. The key is the next final node in that child's name.
@@ -350,11 +354,6 @@ inline Logger::Logger(const std::string& name, LogOut& out, Level logLevel) :
{
}
-inline Logger::Logger(const LoggerImpl& parent, const std::string& name) :
- impl(new LoggerImpl(parent, name))
-{
-}
-
inline bool Logger::isEnabledFor(Level level) const
{
return impl->isEnabledFor(level);
@@ -380,7 +379,7 @@ inline void Logger::vlogf(Level level, char const *fmt, va_list ap) const
}
inline Logger Logger::getChild(const std::string& childName)
{
- return impl->getChild(childName);
+ return impl->getChild(impl, childName);
}
inline std::vector<Logger> Logger::getChildren() const
{
commit 0ea05261438b104b83a27ec2d48f3f4105fdf2e1
Author: David M. Lee <dlee at digium.com>
Date: Fri Dec 10 13:49:00 2010 -0600
Added reference counting to Logger.
Improved confidence in using the Logger by making Logger instances
reference counted. This is almost source compatible, with the only
change to users is that getLogger() now returns a Logger instance rather
than a Logger&.
diff --git a/client/src/Logger.cpp b/client/src/Logger.cpp
index d410834..2c1de2c 100644
--- a/client/src/Logger.cpp
+++ b/client/src/Logger.cpp
@@ -62,30 +62,29 @@ void LogBuf::sendBuffer()
mOut.logs(nName, mLogLevel, message);
}
-Logger::Logger(const std::string& name, LogOut& out, Level logLevel) :
+Logger::LoggerImpl::LoggerImpl(const std::string& name, LogOut& out, Level logLevel) :
mParent(0), mName(name), mOut(&out), mLogLevel(logLevel), mInheritedLevel(false)
{
}
-Logger::Logger(const Logger& parent, const std::string& name) :
- mParent(&parent), mName(name), mOut(parent.mOut), mLogLevel(Off), mInheritedLevel(
- true)
+Logger::LoggerImpl::LoggerImpl(const LoggerImpl& parent, const std::string& name) :
+ mParent(&parent), mName(name), mOut(parent.mOut), mLogLevel(Off),
+ mInheritedLevel(true)
{
// our name must begin w/ parent's name
assert(name.find(parent.mName) == 0);
}
-Logger::~Logger()
+Logger::LoggerImpl::~LoggerImpl()
{
- IceUtil::Mutex::Lock childLock(mChildrenMutex);
}
-CondStream Logger::operator()(Level level) const
+CondStream Logger::LoggerImpl::operator()(Level level) const
{
return CondStream(*mOut, mName, level, isEnabledFor(level));
}
-void Logger::logs(Level level, const std::string& message) const
+void Logger::LoggerImpl::logs(Level level, const std::string& message) const
{
if (isEnabledFor(level))
{
@@ -93,15 +92,7 @@ void Logger::logs(Level level, const std::string& message) const
}
}
-void Logger::logf(Level level, char const *fmt, ...) const
-{
- va_list ap;
- va_start(ap, fmt);
- vlogf(level, fmt, ap);
- va_end(ap);
-}
-
-void Logger::vlogf(Level level, char const *fmt, va_list ap) const
+void Logger::LoggerImpl::vlogf(Level level, char const *fmt, va_list ap) const
{
if (isEnabledFor(level))
{
@@ -113,7 +104,7 @@ void Logger::vlogf(Level level, char const *fmt, va_list ap) const
}
}
-void Logger::setOutput(LogOut& out)
+void Logger::LoggerImpl::setOutput(LogOut& out)
{
this->mOut = &out;
IceUtil::Mutex::Lock childLock(mChildrenMutex);
@@ -126,19 +117,19 @@ void Logger::setOutput(LogOut& out)
}
-void Logger::setLevel(Level logLevel)
+void Logger::LoggerImpl::setLevel(Level logLevel)
{
this->mLogLevel = logLevel;
mInheritedLevel = false;
}
-void Logger::unsetLevel()
+void Logger::LoggerImpl::unsetLevel()
{
mInheritedLevel = true;
mLogLevel = Off;
}
-Level Logger::getEffectiveLevel() const
+Level Logger::LoggerImpl::getEffectiveLevel() const
{
// if our level is unset, inherit level from our parent.
if (mInheritedLevel == true && mParent != 0)
@@ -151,24 +142,24 @@ Level Logger::getEffectiveLevel() const
}
}
-Logger& Logger::getChild(const std::string& childName)
+Logger Logger::LoggerImpl::getChild(const std::string& childName)
{
IceUtil::Mutex::Lock childLock(mChildrenMutex);
// ref to ptr allows us to update the map in-place
- boost::shared_ptr<Logger>& child = mChildren[childName];
+ boost::shared_ptr<LoggerImpl>& child = mChildren[childName];
if (child == 0)
{
std::string fullName = getName().empty() ? childName : getName() + "."
+ childName;
- child.reset(new Logger(*this, fullName));
+ child.reset(new LoggerImpl(*this, fullName));
}
- return *child;
+ return Logger(child);
}
-std::vector<boost::shared_ptr<const Logger> > Logger::getChildren() const
+std::vector<Logger> Logger::LoggerImpl::getChildren() const
{
- std::vector<boost::shared_ptr<const Logger> > r;
+ std::vector<Logger> r;
IceUtil::Mutex::Lock childLock(mChildrenMutex);
for (Children::const_iterator i = mChildren.begin();
i != mChildren.end();
diff --git a/client/src/LoggerFactory.cpp b/client/src/LoggerFactory.cpp
index 81e349b..0b1e845 100644
--- a/client/src/LoggerFactory.cpp
+++ b/client/src/LoggerFactory.cpp
@@ -55,7 +55,7 @@ LoggerFactory::LoggerFactory(LogOut& out) :
{
}
-Logger& LoggerFactory::getLogger(const std::string& name)
+Logger LoggerFactory::getLogger(const std::string& name)
{
std::vector<std::string> path;
// older versions of boost output a single entry when splitting an empty
@@ -65,13 +65,13 @@ Logger& LoggerFactory::getLogger(const std::string& name)
split(path, name, std::bind1st(std::equal_to<char>(), '.'));
}
- Logger *logger = &mRoot;
+ Logger logger = mRoot;
for (std::vector<std::string>::iterator i = path.begin(); i != path.end(); ++i)
{
- logger = &logger->getChild(*i);
+ logger = logger.getChild(*i);
}
- return *logger;
+ return logger;
}
std::vector<std::string> LoggerFactory::getLoggerNames() const
@@ -81,16 +81,16 @@ std::vector<std::string> LoggerFactory::getLoggerNames() const
return r;
}
-void LoggerFactory::accumulateLoggerNames(const Logger& logger,
+void LoggerFactory::accumulateLoggerNames(Logger logger,
std::vector<std::string>& out)
{
out.push_back(logger.getName());
// recurse through the children
- const std::vector<boost::shared_ptr<const Logger> >& children = logger.getChildren();
- for (std::vector<boost::shared_ptr<const Logger> >::const_iterator i = children.begin(); i
+ const std::vector<Logger>& children = logger.getChildren();
+ for (std::vector<Logger>::const_iterator i = children.begin(); i
!= children.end(); ++i)
{
- accumulateLoggerNames(**i, out);
+ accumulateLoggerNames(*i, out);
}
}
diff --git a/client/src/logger.h b/client/src/logger.h
index 88e375a..13225a2 100644
--- a/client/src/logger.h
+++ b/client/src/logger.h
@@ -136,22 +136,108 @@ inline CondStream& CondStream::operator<<(std::ostream& (*pf)(std::ostream&))
}
/**
- * The Logger for a particular source, identified by the given name.
+ * The Logger for a particular source, identified by the given name. This is a
+ * small wrapper class, which should be passed around by copy.
*/
class Logger
{
+ // You may wonder why have this class at all. It only delegates to
+ // LoggerImpl, so why not typedef shared_ptr<LoggerImpl> LoggerPtr
+ // and use that? The problem with that approach is that we would lose
+ // the stream style debugging, or at least may it a bit more cumbersome.
+ // (*lg)(Debug) << "you'd have to dereference the smart pointer"
+ // lg->stream(Debug) << "or have a named function"
+private:
+ /** Implementation class */
+ class LoggerImpl;
+public:
+ /** Construct a Logger for an existing implementation */
+ Logger(const boost::shared_ptr<LoggerImpl>& impl);
+ /**
+ * Construct a root Logger.
+ */
+ Logger(const std::string& name, LogOut& out, Level logLevel = Debug);
+
+ /**
+ * Construct a child Logger.
+ */
+ Logger(const LoggerImpl& parent, const std::string& name);
+
+ /**
+ * If true, this Logger would log messages of the given Level.
+ *
+ * @param level Level to check.
+ * @return true if enabled for given level.
+ */
+ bool isEnabledFor(Level level) const;
+
+ /**
+ * Ostream style logging.
+ *
+ * @param level Level for messages sent to this stream.
+ * @return LogStream that logs at the given level.
+ */
+ CondStream operator()(Level level) const;
+
+ /**
+ * Log a single message.
+ */
+ void logs(Level level, const std::string& message) const;
+
+ /**
+ * Log a single printf-formatted message.
+ */
+ void logf(Level level, char const *fmt, ...) const;
+
+ /**
+ * Log a single vprintf-formatted message.
+ */
+ void vlogf(Level level, char const *fmt, va_list ap) const;
+
+ Logger getChild(const std::string& childName);
+
+ std::vector<Logger> getChildren() const;
+
+ const std::string& getName() const;
+
+ LogOut& getOutput() const;
+
+ void setOutput(LogOut& out);
+
+ /**
+ * Set's the current logLevel. Until unsetLevel() is called, we are no
+ * longer affected by changes to our parent's log level.
+ */
+ void setLevel(Level logLevel);
+
+ /**
+ * Changes our logLevel to now inherit from out parent.
+ */
+ void unsetLevel();
+
+ /**
+ * Returns the effective level of this Logger.
+ */
+ Level getEffectiveLevel() const;
+
+private:
+ boost::shared_ptr<LoggerImpl> impl;
+};
+
+class Logger::LoggerImpl
+{
public:
/**
* Construct a root Logger.
*/
- ASTERISK_SCF_ICEBOX_EXPORT Logger(const std::string& name, LogOut& out, Level logLevel = Debug);
+ ASTERISK_SCF_ICEBOX_EXPORT LoggerImpl(const std::string& name, LogOut& out, Level logLevel = Debug);
/**
* Construct a child Logger.
*/
- ASTERISK_SCF_ICEBOX_EXPORT Logger(const Logger& parent, const std::string& name);
+ ASTERISK_SCF_ICEBOX_EXPORT LoggerImpl(const LoggerImpl& parent, const std::string& name);
- ASTERISK_SCF_ICEBOX_EXPORT ~Logger();
+ ASTERISK_SCF_ICEBOX_EXPORT ~LoggerImpl();
/**
* If true, this Logger would log messages of the given Level.
@@ -178,29 +264,19 @@ public:
ASTERISK_SCF_ICEBOX_EXPORT void logs(Level level, const std::string& message) const;
/**
- * Log a single printf-formatted message.
- */
- ASTERISK_SCF_ICEBOX_EXPORT void logf(Level level, char const *fmt, ...) const;
-
- /**
* Log a single vprintf-formatted message.
*/
ASTERISK_SCF_ICEBOX_EXPORT void vlogf(Level level, char const *fmt, va_list ap) const;
- Logger const *getParent() const
- {
- return mParent;
- }
-
- ASTERISK_SCF_ICEBOX_EXPORT Logger& getChild(const std::string& childName);
- ASTERISK_SCF_ICEBOX_EXPORT std::vector<boost::shared_ptr<const Logger> > getChildren() const;
+ ASTERISK_SCF_ICEBOX_EXPORT Logger getChild(const std::string& childName);
+ ASTERISK_SCF_ICEBOX_EXPORT std::vector<Logger> getChildren() const;
- ASTERISK_SCF_ICEBOX_EXPORT const std::string& getName() const
+ const std::string& getName() const
{
return mName;
}
- ASTERISK_SCF_ICEBOX_EXPORT LogOut& getOutput() const
+ LogOut& getOutput() const
{
return *mOut;
}
@@ -225,8 +301,8 @@ public:
private:
// non-copyable
- Logger(const Logger&);
- const Logger& operator=(const Logger&);
+ LoggerImpl(const Logger&);
+ const LoggerImpl& operator=(const LoggerImpl&);
/**
* Mutex for access to the children field.
@@ -237,8 +313,8 @@ private:
* Parent pointer. We cannot change which parent we have, nor can we change
* our parent.
*/
- Logger const * const mParent;
- typedef std::map<std::string, boost::shared_ptr<Logger> > Children;
+ LoggerImpl const * const mParent;
+ typedef std::map<std::string, boost::shared_ptr<LoggerImpl> > Children;
/**
* Map of children. The key is the next final node in that child's name.
*/
@@ -262,6 +338,79 @@ private:
bool mInheritedLevel;
};
+// since these are all simple delegates to LoggerImpl, inline them
+
+inline Logger::Logger(const boost::shared_ptr<LoggerImpl>& impl) :
+ impl(impl)
+{
+}
+
+inline Logger::Logger(const std::string& name, LogOut& out, Level logLevel) :
+ impl(new LoggerImpl(name, out, logLevel))
+{
+}
+
+inline Logger::Logger(const LoggerImpl& parent, const std::string& name) :
+ impl(new LoggerImpl(parent, name))
+{
+}
+
+inline bool Logger::isEnabledFor(Level level) const
+{
+ return impl->isEnabledFor(level);
+}
+inline CondStream Logger::operator()(Level level) const
+{
+ return (*impl)(level);
+}
+inline void Logger::logs(Level level, const std::string& message) const
+{
+ impl->logs(level, message);
+}
+inline void Logger::logf(Level level, char const *fmt, ...) const
+{
+ va_list ap;
+ va_start(ap, fmt);
+ impl->vlogf(level, fmt, ap);
+ va_end(ap);
+}
+inline void Logger::vlogf(Level level, char const *fmt, va_list ap) const
+{
+ impl->vlogf(level, fmt, ap);
+}
+inline Logger Logger::getChild(const std::string& childName)
+{
+ return impl->getChild(childName);
+}
+inline std::vector<Logger> Logger::getChildren() const
+{
+ return impl->getChildren();
+}
+inline const std::string& Logger::getName() const
+{
+ return impl->getName();
+}
+inline LogOut& Logger::getOutput() const
+{
+ return impl->getOutput();
+}
+inline void Logger::setOutput(LogOut& out)
+{
+ impl->setOutput(out);
+}
+inline void Logger::setLevel(Level logLevel)
+{
+ impl->setLevel(logLevel);
+}
+inline void Logger::unsetLevel()
+{
+ impl->unsetLevel();
+}
+inline Level Logger::getEffectiveLevel() const
+{
+ return impl->getEffectiveLevel();
+}
+
/**
* Main entry point into the Logger system.
*/
@@ -278,7 +427,7 @@ public:
* @return Ref to the Logger.
* @thread-safe
*/
- ASTERISK_SCF_ICEBOX_EXPORT Logger& getLogger(const std::string& name);
+ ASTERISK_SCF_ICEBOX_EXPORT Logger getLogger(const std::string& name);
/**
* Returns a vector of the names of all currently configured Logger's.
@@ -291,7 +440,7 @@ public:
private:
Logger mRoot;
- static void accumulateLoggerNames(const Logger& logger, std::vector<std::string>& out);
+ static void accumulateLoggerNames(Logger logger, std::vector<std::string>& out);
};
ASTERISK_SCF_ICEBOX_EXPORT boost::shared_ptr<LogOut> buildOstreamLogger(std::ostream& out);
diff --git a/client/test/LoggerFactory-test.cpp b/client/test/LoggerFactory-test.cpp
index 8d6c3cb..160b814 100644
--- a/client/test/LoggerFactory-test.cpp
+++ b/client/test/LoggerFactory-test.cpp
@@ -23,28 +23,16 @@ using namespace AsteriskSCF::System::Logging;
BOOST_AUTO_TEST_SUITE(LoggerFactoryTest)
-BOOST_AUTO_TEST_CASE(testGetDistinct)
-{
- std::stringstream tmp;
- boost::shared_ptr<LogOut> logOut = buildOstreamLogger(tmp);
- LoggerFactory uut(*logOut);
-
- const Logger& asteriskScf = uut.getLogger("AsteriskSCF");
- const Logger& core = uut.getLogger("AsteriskSCF.Core");
-
- BOOST_CHECK_NE(&core, &asteriskScf);
- BOOST_CHECK_EQUAL(&asteriskScf, core.getParent());
- BOOST_CHECK_EQUAL(&uut.getLogger(""), asteriskScf.getParent());
-}
-
BOOST_AUTO_TEST_CASE(testInheritence_off)
{
std::stringstream actual;
boost::shared_ptr<LogOut> logOut = buildOstreamLogger(actual);
LoggerFactory uut(*logOut);
- Logger& root = uut.getLogger("");
- Logger& asteriskScf = uut.getLogger("AsteriskSCF");
+ Logger root = uut.getLogger("");
+ Logger asteriskScf = uut.getLogger("AsteriskSCF");
+
+ root = asteriskScf;
root.setLevel(Off);
@@ -58,8 +46,8 @@ BOOST_AUTO_TEST_CASE(testInheritence_on)
boost::shared_ptr<LogOut> logOut = buildOstreamLogger(actual);
LoggerFactory uut(*logOut);
- Logger& root = uut.getLogger("");
- Logger& asteriskScf = uut.getLogger("AsteriskSCF");
+ Logger root = uut.getLogger("");
+ Logger asteriskScf = uut.getLogger("AsteriskSCF");
root.setLevel(Debug);
@@ -81,8 +69,6 @@ BOOST_AUTO_TEST_CASE(testChangeLogOut)
uut.logs(Debug, "Debug Message");
out1.check();
out2.check();
-
-
}
BOOST_AUTO_TEST_SUITE_END()
-----------------------------------------------------------------------
--
asterisk-scf/integration/logger.git
More information about the asterisk-scf-commits
mailing list