You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by tr...@apache.org on 2010/09/14 22:50:50 UTC
svn commit: r997089 - in /qpid/trunk/qpid/cpp/src/qpid/agent:
ManagementAgentImpl.cpp ManagementAgentImpl.h
Author: tross
Date: Tue Sep 14 20:50:50 2010
New Revision: 997089
URL: http://svn.apache.org/viewvc?rev=997089&view=rev
Log:
Fixed a thread safety issue in which the managementObjects map was used in an unsafe way
(i.e. without the lock held).
Replaced a raw pointer with a boost::shared_ptr to protect objects during method calls.
Modified:
qpid/trunk/qpid/cpp/src/qpid/agent/ManagementAgentImpl.cpp
qpid/trunk/qpid/cpp/src/qpid/agent/ManagementAgentImpl.h
Modified: qpid/trunk/qpid/cpp/src/qpid/agent/ManagementAgentImpl.cpp
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/agent/ManagementAgentImpl.cpp?rev=997089&r1=997088&r2=997089&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/agent/ManagementAgentImpl.cpp (original)
+++ qpid/trunk/qpid/cpp/src/qpid/agent/ManagementAgentImpl.cpp Tue Sep 14 20:50:50 2010
@@ -121,19 +121,6 @@ ManagementAgentImpl::~ManagementAgentImp
connThread.join();
pubThread.join();
- // Release the memory associated with stored management objects.
- {
- sys::Mutex::ScopedLock lock(agentLock);
-
- moveNewObjectsLH();
- for (ManagementObjectMap::iterator iter = managementObjects.begin ();
- iter != managementObjects.end ();
- iter++) {
- ManagementObject* object = iter->second;
- delete object;
- }
- managementObjects.clear();
- }
if (pipeHandle) {
delete pipeHandle;
pipeHandle = 0;
@@ -294,7 +281,7 @@ ObjectId ManagementAgentImpl::addObject(
objectId.setAgentName(name_address);
object->setObjectId(objectId);
- newManagementObjects[objectId] = object;
+ newManagementObjects[objectId] = boost::shared_ptr<ManagementObject>(object);
return objectId;
}
@@ -561,24 +548,31 @@ void ManagementAgentImpl::invokeMethodRe
inArgs = (mid->second).asMap();
}
- ManagementObjectMap::iterator iter = managementObjects.find(objId);
- if (iter == managementObjects.end() || iter->second->isDeleted()) {
+ boost::shared_ptr<ManagementObject> oPtr;
+ {
+ sys::Mutex::ScopedLock lock(agentLock);
+ ObjectMap::iterator iter = managementObjects.find(objId);
+ if (iter != managementObjects.end() && !iter->second->isDeleted())
+ oPtr = iter->second;
+ }
+
+ if (oPtr.get() == 0) {
sendException(replyTo, cid, Manageable::StatusText(Manageable::STATUS_UNKNOWN_OBJECT),
Manageable::STATUS_UNKNOWN_OBJECT);
failed = true;
} else {
- iter->second->doMethod(methodName, inArgs, callMap);
- }
+ oPtr->doMethod(methodName, inArgs, callMap);
- if (callMap["_status_code"].asUint32() == 0) {
- outMap["_arguments"] = Variant::Map();
- for (Variant::Map::const_iterator iter = callMap.begin();
- iter != callMap.end(); iter++)
- if (iter->first != "_status_code" && iter->first != "_status_text")
- outMap["_arguments"].asMap()[iter->first] = iter->second;
- } else {
- sendException(replyTo, cid, callMap["_status_text"], callMap["_status_code"]);
- failed = true;
+ if (callMap["_status_code"].asUint32() == 0) {
+ outMap["_arguments"] = Variant::Map();
+ for (Variant::Map::const_iterator iter = callMap.begin();
+ iter != callMap.end(); iter++)
+ if (iter->first != "_status_code" && iter->first != "_status_text")
+ outMap["_arguments"].asMap()[iter->first] = iter->second;
+ } else {
+ sendException(replyTo, cid, callMap["_status_text"], callMap["_status_code"]);
+ failed = true;
+ }
}
} catch(types::InvalidConversion& e) {
@@ -643,11 +637,16 @@ void ManagementAgentImpl::handleGetQuery
i = inMap.find("_object_id");
if (i != inMap.end() && i->second.getType() == qpid::types::VAR_MAP) {
ObjectId objId(i->second.asMap());
+ boost::shared_ptr<ManagementObject> object;
- ManagementObjectMap::iterator iter = managementObjects.find(objId);
- if (iter != managementObjects.end()) {
- ManagementObject* object = iter->second;
+ {
+ sys::Mutex::ScopedLock lock(agentLock);
+ ObjectMap::iterator iter = managementObjects.find(objId); // XXX - Unprotected
+ if (iter != managementObjects.end())
+ object = iter->second;
+ }
+ if (object.get() != 0) {
if (object->getConfigChanged() || object->getInstChanged())
object->setUpdateTime();
@@ -684,11 +683,24 @@ void ManagementAgentImpl::handleGetQuery
if (s_iter != schemaIdMap.end() && s_iter->second.getType() == qpid::types::VAR_STRING)
packageName = s_iter->second.asString();
+ typedef list<boost::shared_ptr<ManagementObject> > StageList;
+ StageList staging;
+
+ {
+ sys::Mutex::ScopedLock lock(agentLock);
+ for (ObjectMap::iterator iter = managementObjects.begin();
+ iter != managementObjects.end();
+ iter++) {
+ ManagementObject* object = iter->second.get();
+ if (object->getClassName() == className &&
+ (packageName.empty() || object->getPackageName() == packageName))
+ staging.push_back(iter->second);
+ }
+ }
+
unsigned int objCount = 0;
- for (ManagementObjectMap::iterator iter = managementObjects.begin();
- iter != managementObjects.end();
- iter++) {
- ManagementObject* object = iter->second;
+ for (StageList::iterator iter = staging.begin(); iter != staging.end(); iter++) {
+ ManagementObject* object = iter->get();
if (object->getClassName() == className &&
(packageName.empty() || object->getPackageName() == packageName)) {
@@ -700,7 +712,7 @@ void ManagementAgentImpl::handleGetQuery
object->setUpdateTime();
object->mapEncodeValues(values, true, true); // write both stats and properties
- iter->first.mapEncode(oidMap);
+ object->getObjectId().mapEncode(oidMap);
map_["_values"] = values;
map_["_object_id"] = oidMap;
object->writeTimestamps(map_);
@@ -922,7 +934,7 @@ ManagementAgentImpl::PackageMap::iterato
void ManagementAgentImpl::moveNewObjectsLH()
{
sys::Mutex::ScopedLock lock(addLock);
- for (ManagementObjectMap::iterator iter = newManagementObjects.begin();
+ for (ObjectMap::iterator iter = newManagementObjects.begin();
iter != newManagementObjects.end();
iter++)
managementObjects[iter->first] = iter->second;
@@ -977,7 +989,7 @@ void ManagementAgentImpl::periodicProces
{
string addr_key_base = "agent.ind.data.";
sys::Mutex::ScopedLock lock(agentLock);
- list<pair<ObjectId, ManagementObject*> > deleteList;
+ list<ObjectId> deleteList;
if (!connected)
return;
@@ -989,10 +1001,10 @@ void ManagementAgentImpl::periodicProces
//
// Clear the been-here flag on all objects in the map.
//
- for (ManagementObjectMap::iterator iter = managementObjects.begin();
+ for (ObjectMap::iterator iter = managementObjects.begin();
iter != managementObjects.end();
iter++) {
- ManagementObject* object = iter->second;
+ ManagementObject* object = iter->second.get();
object->setFlags(0);
if (clientWasAdded) {
object->setForcePublish(true);
@@ -1006,10 +1018,10 @@ void ManagementAgentImpl::periodicProces
//
uint32_t v2Objs = 0;
- for (ManagementObjectMap::iterator baseIter = managementObjects.begin();
+ for (ObjectMap::iterator baseIter = managementObjects.begin();
baseIter != managementObjects.end();
baseIter++) {
- ManagementObject* baseObject = baseIter->second;
+ ManagementObject* baseObject = baseIter->second.get();
//
// Skip until we find a base object requiring a sent message.
@@ -1041,10 +1053,10 @@ void ManagementAgentImpl::periodicProces
headers["qmf.content"] = "_data";
headers["qmf.agent"] = name_address;
- for (ManagementObjectMap::iterator iter = baseIter;
+ for (ObjectMap::iterator iter = baseIter;
iter != managementObjects.end();
iter++) {
- ManagementObject* object = iter->second;
+ ManagementObject* object = iter->second.get();
bool send_stats, send_props;
if (baseObject->isSameClass(*object) && object->getFlags() == 0) {
object->setFlags(1);
@@ -1081,7 +1093,7 @@ void ManagementAgentImpl::periodicProces
}
if (object->isDeleted())
- deleteList.push_back(pair<ObjectId, ManagementObject*>(iter->first, object));
+ deleteList.push_back(iter->first);
object->setForcePublish(false);
}
}
@@ -1094,14 +1106,10 @@ void ManagementAgentImpl::periodicProces
}
// Delete flagged objects
- for (list<pair<ObjectId, ManagementObject*> >::reverse_iterator iter = deleteList.rbegin();
+ for (list<ObjectId>::reverse_iterator iter = deleteList.rbegin();
iter != deleteList.rend();
- iter++) {
- delete iter->second;
- managementObjects.erase(iter->first);
- }
-
- deleteList.clear();
+ iter++)
+ managementObjects.erase(*iter);
}
Modified: qpid/trunk/qpid/cpp/src/qpid/agent/ManagementAgentImpl.h
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/agent/ManagementAgentImpl.h?rev=997089&r1=997088&r2=997089&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/agent/ManagementAgentImpl.h (original)
+++ qpid/trunk/qpid/cpp/src/qpid/agent/ManagementAgentImpl.h Tue Sep 14 20:50:50 2010
@@ -142,9 +142,12 @@ class ManagementAgentImpl : public Manag
PackageMap packages;
AgentAttachment attachment;
- management::ManagementObjectMap managementObjects;
- management::ManagementObjectMap newManagementObjects;
- MethodQueue methodQueue;
+
+ typedef std::map<ObjectId, boost::shared_ptr<ManagementObject> > ObjectMap;
+
+ ObjectMap managementObjects;
+ ObjectMap newManagementObjects;
+ MethodQueue methodQueue;
void received (client::Message& msg);
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:commits-subscribe@qpid.apache.org