-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31619/
-----------------------------------------------------------
(Updated March 3, 2015, 12:38 p.m.)
Review request for qpid, Andrew Stitcher, Gordon Sim, and Kenneth Giusti.
Changes
-------
Put the addLock and objectLock to proper ordering.
Acquired userLock in periodicProcessing before calling debugSnapshot.
Bugs: QPID-6397
https://issues.apache.org/jira/browse/QPID-6397
Repository: qpid
Description
-------
The race condition leading to the coredump with backtraces in the JIRA when:
- traces are enabled (at least for
qpid::management::ManagementAgent::debugSnapshot scope)
- periodic processing has just started, is in debugSnapshot, and iterates
either over managementObjects (dumpMap) or over newManagementObjects
(dumpVector)
- a QMF request is being processed via handleMethodRequest and moveNewObjects
moves newly seen objects from newManagementObjects to managementObjects
Here the problem is, access to neither managementObjects or
newManagementObjects is guarded by locks within debugSnapshot, causing the
iteration over the map / vector fails when updating it concurrently.
I spotted segfaults in either dumpMap or in dumpVector, both needs to be fixed.
Proposed is a trivial patch in adding both locks. An option (in case the
locking is time consuming) is conditional locks:
Index: cpp/src/qpid/management/ManagementAgent.cpp
===================================================================
--- cpp/src/qpid/management/ManagementAgent.cpp (revision 1660046)
+++ cpp/src/qpid/management/ManagementAgent.cpp (working copy)
@@ -2710,11 +2710,17 @@
<< summarizeVector("new objects ", newManagementObjects)
<< pendingDeletedObjs.size() << " pending deletes"
<< summarizeAgents());
-
- QPID_LOG_IF(trace, managementObjects.size(),
- title << ": objects" << dumpMap(managementObjects));
- QPID_LOG_IF(trace, newManagementObjects.size(),
- title << ": new objects" <<
dumpVector(newManagementObjects));
+ bool print_traces;
+ QPID_LOG_TEST(trace, print_traces);
+ if (print_traces)
+ {
+ sys::Mutex::ScopedLock objLock (objectLock);
+ sys::Mutex::ScopedLock lock(addLock);
+ QPID_LOG_IF(trace, managementObjects.size(),
+ title << ": objects" << dumpMap(managementObjects));
+ QPID_LOG_IF(trace, newManagementObjects.size(),
+ title << ": new objects" <<
dumpVector(newManagementObjects));
+ }
}
This might work well only if std::count_if on either map or vector (called in
summarizeVector and summarizeMap) is thread-safe operation.
Diffs (updated)
-----
trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1660046
Diff: https://reviews.apache.org/r/31619/diff/
Testing
-------
The first patch successfully tested against the reproducer for 3 days, the 2nd
proposed hasnt been tested.
Thanks,
Pavel Moravec
