From 33d5d9c083105c6c5f8fd2d4d0c19154b83ceb32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnter=20Obiltschnig?= Date: Thu, 12 Oct 2023 10:38:04 +0200 Subject: [PATCH] #4182: Util: Make load()/save()/clear() operations on configurations thread-safe --- .../include/Poco/Util/AbstractConfiguration.h | 32 +++++++++++++++++++ Util/src/AbstractConfiguration.cpp | 1 - Util/src/IniFileConfiguration.cpp | 2 ++ Util/src/JSONConfiguration.cpp | 19 +++++------ Util/src/LayeredConfiguration.cpp | 6 ++++ Util/src/MapConfiguration.cpp | 4 +++ Util/src/PropertyFileConfiguration.cpp | 4 +++ Util/src/XMLConfiguration.cpp | 15 +++++++++ 8 files changed, 73 insertions(+), 10 deletions(-) diff --git a/Util/include/Poco/Util/AbstractConfiguration.h b/Util/include/Poco/Util/AbstractConfiguration.h index 4c10a9c790..7b8cd3a20a 100644 --- a/Util/include/Poco/Util/AbstractConfiguration.h +++ b/Util/include/Poco/Util/AbstractConfiguration.h @@ -421,6 +421,37 @@ class Util_API AbstractConfiguration: public Poco::RefCountedObject /// Returns true iff events are enabled. protected: + class ScopedLock + /// A helper class allowing to temporarily + /// lock an entire AbstractConfiguration, + /// for use by subclasses. A typical use + /// case is loading or saving an entire + /// configuration in a thread-safe way. + /// + /// Caution: Thoughtless use of this class + /// may easily lead to deadlock situations + /// in connection with events if any of the + /// mutating methods (set...(), remove()) + /// are called with the lock held. Therefore + /// this class is available to subclasses + /// only, not for general use. + { + public: + explicit ScopedLock(const AbstractConfiguration& c): + _c(c) + { + _c._mutex.lock(); + } + + ~ScopedLock() + { + _c._mutex.unlock(); + } + + private: + const AbstractConfiguration& _c; + }; + virtual bool getRaw(const std::string& key, std::string& value) const = 0; /// If the property with the given key exists, stores the property's value /// in value and returns true. Otherwise, returns false. @@ -493,6 +524,7 @@ class Util_API AbstractConfiguration: public Poco::RefCountedObject friend class ConfigurationView; friend class LocalConfigurationView; friend class ConfigurationMapper; + friend class ScopedLock; }; diff --git a/Util/src/AbstractConfiguration.cpp b/Util/src/AbstractConfiguration.cpp index 2533c40aa4..f7b9a2e08b 100644 --- a/Util/src/AbstractConfiguration.cpp +++ b/Util/src/AbstractConfiguration.cpp @@ -105,7 +105,6 @@ std::string AbstractConfiguration::getRawString(const std::string& key) const std::string AbstractConfiguration::getRawString(const std::string& key, const std::string& defaultValue) const { - Mutex::ScopedLock lock(_mutex); std::string value; diff --git a/Util/src/IniFileConfiguration.cpp b/Util/src/IniFileConfiguration.cpp index 67aeb0fbf9..6e3f5ee4a5 100644 --- a/Util/src/IniFileConfiguration.cpp +++ b/Util/src/IniFileConfiguration.cpp @@ -59,6 +59,8 @@ IniFileConfiguration::~IniFileConfiguration() void IniFileConfiguration::load(std::istream& istr) { + AbstractConfiguration::ScopedLock lock(*this); + _map.clear(); _sectionKey.clear(); while (!istr.eof()) diff --git a/Util/src/JSONConfiguration.cpp b/Util/src/JSONConfiguration.cpp index 78b9dcb613..ac08c9f602 100644 --- a/Util/src/JSONConfiguration.cpp +++ b/Util/src/JSONConfiguration.cpp @@ -67,6 +67,8 @@ void JSONConfiguration::load(const std::string& path) void JSONConfiguration::load(std::istream& istr) { + AbstractConfiguration::ScopedLock lock(*this); + JSON::Parser parser; parser.parse(istr); DynamicAny result = parser.result(); @@ -79,6 +81,8 @@ void JSONConfiguration::load(std::istream& istr) void JSONConfiguration::loadEmpty(const std::string& root) { + AbstractConfiguration::ScopedLock lock(*this); + _object = new JSON::Object(); JSON::Object::Ptr rootObject = new JSON::Object(); _object->set(root, rootObject); @@ -106,7 +110,7 @@ void JSONConfiguration::getIndexes(std::string& name, std::vector& indexes) int firstOffset = -1; int offset = 0; RegularExpression regex("\\[([0-9]+)\\]"); - while(regex.match(name, offset, matches) > 0) + while (regex.match(name, offset, matches) > 0) { if (firstOffset == -1) { @@ -131,7 +135,7 @@ JSON::Object::Ptr JSONConfiguration::findStart(const std::string& key, std::stri StringTokenizer tokenizer(key, "."); lastPart = tokenizer[tokenizer.count() - 1]; - for(int i = 0; i < tokenizer.count() - 1; ++i) + for (int i = 0; i < tokenizer.count() - 1; ++i) { std::vector indexes; std::string name = tokenizer[i]; @@ -241,7 +245,6 @@ JSON::Object::Ptr JSONConfiguration::findStart(const std::string& key, std::stri void JSONConfiguration::setValue(const std::string& key, const Poco::DynamicAny& value) { - std::string sValue; value.convert(sValue); @@ -276,12 +279,12 @@ void JSONConfiguration::setValue(const std::string& key, const Poco::DynamicAny& } JSON::Array::Ptr arr = result.extract(); - for(std::vector::iterator it = indexes.begin(); it != indexes.end() - 1; ++it) + for (std::vector::iterator it = indexes.begin(); it != indexes.end() - 1; ++it) { JSON::Array::Ptr nextArray = arr->getArray(*it); if (nextArray.isNull()) { - for(int i = static_cast(arr->size()); i <= *it; ++i) + for (int i = static_cast(arr->size()); i <= *it; ++i) { Poco::DynamicAny nullValue; arr->add(nullValue); @@ -345,14 +348,14 @@ void JSONConfiguration::enumerate(const std::string& key, Keys& range) const void JSONConfiguration::save(std::ostream& ostr, unsigned int indent) const { + AbstractConfiguration::ScopedLock lock(*this); + _object->stringify(ostr, indent); } void JSONConfiguration::removeRaw(const std::string& key) - { - std::string lastPart; JSON::Object::Ptr parentObject = findStart(key, lastPart); std::vector indexes; @@ -367,7 +370,6 @@ void JSONConfiguration::removeRaw(const std::string& key) DynamicAny result = parentObject->get(lastPart); if (!result.isEmpty() && result.type() == typeid(JSON::Array::Ptr)) { - JSON::Array::Ptr arr = result.extract(); for(std::vector::iterator it = indexes.begin(); it != indexes.end() - 1; ++it) { @@ -376,7 +378,6 @@ void JSONConfiguration::removeRaw(const std::string& key) arr->remove(indexes.back()); } } - } diff --git a/Util/src/LayeredConfiguration.cpp b/Util/src/LayeredConfiguration.cpp index bdbd827a12..c8ed780199 100644 --- a/Util/src/LayeredConfiguration.cpp +++ b/Util/src/LayeredConfiguration.cpp @@ -73,6 +73,8 @@ void LayeredConfiguration::add(AbstractConfiguration::Ptr pConfig, int priority, void LayeredConfiguration::add(AbstractConfiguration::Ptr pConfig, const std::string& label, int priority, bool writeable) { + AbstractConfiguration::ScopedLock lock(*this); + ConfigItem item; item.pConfig = pConfig; item.priority = priority; @@ -87,6 +89,8 @@ void LayeredConfiguration::add(AbstractConfiguration::Ptr pConfig, const std::st void LayeredConfiguration::removeConfiguration(AbstractConfiguration::Ptr pConfig) { + AbstractConfiguration::ScopedLock lock(*this); + for (ConfigList::iterator it = _configs.begin(); it != _configs.end(); ++it) { if (it->pConfig == pConfig) @@ -100,6 +104,8 @@ void LayeredConfiguration::removeConfiguration(AbstractConfiguration::Ptr pConfi AbstractConfiguration::Ptr LayeredConfiguration::find(const std::string& label) const { + AbstractConfiguration::ScopedLock lock(*this); + for (const auto& conf: _configs) { if (conf.label == label) return conf.pConfig; diff --git a/Util/src/MapConfiguration.cpp b/Util/src/MapConfiguration.cpp index 2b0e6aef02..223534953a 100644 --- a/Util/src/MapConfiguration.cpp +++ b/Util/src/MapConfiguration.cpp @@ -32,6 +32,8 @@ MapConfiguration::~MapConfiguration() void MapConfiguration::copyTo(AbstractConfiguration& config) { + AbstractConfiguration::ScopedLock lock(*this); + for (const auto& p: _map) { config.setString(p.first, p.second); @@ -41,6 +43,8 @@ void MapConfiguration::copyTo(AbstractConfiguration& config) void MapConfiguration::clear() { + AbstractConfiguration::ScopedLock lock(*this); + _map.clear(); } diff --git a/Util/src/PropertyFileConfiguration.cpp b/Util/src/PropertyFileConfiguration.cpp index 2539849359..6e952f5703 100644 --- a/Util/src/PropertyFileConfiguration.cpp +++ b/Util/src/PropertyFileConfiguration.cpp @@ -53,6 +53,8 @@ PropertyFileConfiguration::~PropertyFileConfiguration() void PropertyFileConfiguration::load(std::istream& istr) { + AbstractConfiguration::ScopedLock lock(*this); + clear(); while (!istr.eof()) { @@ -73,6 +75,8 @@ void PropertyFileConfiguration::load(const std::string& path) void PropertyFileConfiguration::save(std::ostream& ostr) const { + AbstractConfiguration::ScopedLock lock(*this); + MapConfiguration::iterator it = begin(); MapConfiguration::iterator ed = end(); while (it != ed) diff --git a/Util/src/XMLConfiguration.cpp b/Util/src/XMLConfiguration.cpp index 055a9f5a67..464c8a2333 100644 --- a/Util/src/XMLConfiguration.cpp +++ b/Util/src/XMLConfiguration.cpp @@ -159,6 +159,8 @@ void XMLConfiguration::load(const Poco::XML::Document* pDocument) { poco_check_ptr (pDocument); + AbstractConfiguration::ScopedLock lock(*this); + _pDocument = Poco::XML::AutoPtr(const_cast(pDocument), true); _pRoot = Poco::XML::AutoPtr(pDocument->documentElement(), true); } @@ -174,6 +176,8 @@ void XMLConfiguration::load(const Poco::XML::Node* pNode) } else { + AbstractConfiguration::ScopedLock lock(*this); + _pDocument = Poco::XML::AutoPtr(pNode->ownerDocument(), true); _pRoot = Poco::XML::AutoPtr(const_cast(pNode), true); } @@ -182,6 +186,8 @@ void XMLConfiguration::load(const Poco::XML::Node* pNode) void XMLConfiguration::loadEmpty(const std::string& rootElementName) { + AbstractConfiguration::ScopedLock lock(*this); + _pDocument = new Poco::XML::Document; _pRoot = _pDocument->createElement(rootElementName); _pDocument->appendChild(_pRoot); @@ -190,6 +196,8 @@ void XMLConfiguration::loadEmpty(const std::string& rootElementName) void XMLConfiguration::save(const std::string& path) const { + AbstractConfiguration::ScopedLock lock(*this); + Poco::XML::DOMWriter writer; writer.setNewLine("\n"); writer.setOptions(Poco::XML::XMLWriter::PRETTY_PRINT); @@ -199,6 +207,8 @@ void XMLConfiguration::save(const std::string& path) const void XMLConfiguration::save(std::ostream& ostr) const { + AbstractConfiguration::ScopedLock lock(*this); + Poco::XML::DOMWriter writer; writer.setNewLine("\n"); writer.setOptions(Poco::XML::XMLWriter::PRETTY_PRINT); @@ -208,12 +218,16 @@ void XMLConfiguration::save(std::ostream& ostr) const void XMLConfiguration::save(Poco::XML::DOMWriter& writer, const std::string& path) const { + AbstractConfiguration::ScopedLock lock(*this); + writer.writeNode(path, _pDocument); } void XMLConfiguration::save(Poco::XML::DOMWriter& writer, std::ostream& ostr) const { + AbstractConfiguration::ScopedLock lock(*this); + writer.writeNode(ostr, _pDocument); } @@ -476,4 +490,5 @@ Poco::XML::Node* XMLConfiguration::findAttribute(const std::string& name, Poco:: } } // namespace Poco::Util + #endif // POCO_UTIL_NO_XMLCONFIGURATION