----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31619/#review74743 -----------------------------------------------------------
trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp <https://reviews.apache.org/r/31619/#comment121438> I think you have to include the userLock in this as well (stuff like packages and remoteAgents are being touched). There's a specific locking order that needs to be maintained when taking multiple locks: 1) first take userLock, 2) then take addLock 3) finally take objectLock (see ManagementAgent.h). Taking objectLock first, then addlock (as done in this patch) can cause a deadlock. - Kenneth Giusti On March 2, 2015, 9:56 a.m., Pavel Moravec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31619/ > ----------------------------------------------------------- > > (Updated March 2, 2015, 9:56 a.m.) > > > Review request for qpid, Andrew Stitcher, Gordon Sim, and Kenneth Giusti. > > > 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 > ----- > > 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 > >
