From 77ca4cf515601a7e8e249dc992f1e3ed690c06de Mon Sep 17 00:00:00 2001 From: David Kelly Date: Thu, 26 Jan 2017 17:38:32 -0700 Subject: [PATCH 1/3] Avoid holding 2 locks at same time If they are always called in same order, you are fine. But just in case, lets instead grab one lock, update the data, release it and then grab the other. No possible deadlock in that, even if the order is changed in one place or another. --- libraries/networking/src/NodeList.cpp | 40 +++++++++++++++++---------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index 7bfe1d1845..f4a02ad805 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -827,18 +827,26 @@ void NodeList::ignoreNodeBySessionID(const QUuid& nodeID, bool ignoreEnabled) { }); if (ignoreEnabled) { - QReadLocker ignoredSetLocker{ &_ignoredSetLock }; // read lock for insert - QReadLocker personalMutedSetLocker{ &_personalMutedSetLock }; // read lock for insert - // add this nodeID to our set of ignored IDs - _ignoredNodeIDs.insert(nodeID); - // add this nodeID to our set of personal muted IDs - _personalMutedNodeIDs.insert(nodeID); + { + QReadLocker ignoredSetLocker{ &_ignoredSetLock }; // read lock for insert + // add this nodeID to our set of ignored IDs + _ignoredNodeIDs.insert(nodeID); + } + { + QReadLocker personalMutedSetLocker{ &_personalMutedSetLock }; // read lock for insert + // add this nodeID to our set of personal muted IDs + _personalMutedNodeIDs.insert(nodeID); + } emit ignoredNode(nodeID, true); } else { - QWriteLocker ignoredSetLocker{ &_ignoredSetLock }; // write lock for unsafe_erase - QWriteLocker personalMutedSetLocker{ &_personalMutedSetLock }; // write lock for unsafe_erase - _ignoredNodeIDs.unsafe_erase(nodeID); - _personalMutedNodeIDs.unsafe_erase(nodeID); + { + QWriteLocker ignoredSetLocker{ &_ignoredSetLock }; // write lock for unsafe_erase + _ignoredNodeIDs.unsafe_erase(nodeID); + } + { + QWriteLocker personalMutedSetLocker{ &_personalMutedSetLock }; // write lock for unsafe_erase + _personalMutedNodeIDs.unsafe_erase(nodeID); + } emit ignoredNode(nodeID, false); } @@ -850,10 +858,14 @@ void NodeList::ignoreNodeBySessionID(const QUuid& nodeID, bool ignoreEnabled) { void NodeList::removeFromIgnoreMuteSets(const QUuid& nodeID) { // don't remove yourself, or nobody if (!nodeID.isNull() && _sessionUUID != nodeID) { - QWriteLocker ignoredSetLocker{ &_ignoredSetLock }; - QWriteLocker personalMutedSetLocker{ &_personalMutedSetLock }; - _ignoredNodeIDs.unsafe_erase(nodeID); - _personalMutedNodeIDs.unsafe_erase(nodeID); + { + QWriteLocker ignoredSetLocker{ &_ignoredSetLock }; + _ignoredNodeIDs.unsafe_erase(nodeID); + } + { + QWriteLocker personalMutedSetLocker{ &_personalMutedSetLock }; + _personalMutedNodeIDs.unsafe_erase(nodeID); + } } } From 069c85c7c250773f1302870d3679e1d1fd187c62 Mon Sep 17 00:00:00 2001 From: David Kelly Date: Fri, 3 Feb 2017 08:52:33 -0700 Subject: [PATCH 2/3] I like this better --- libraries/networking/src/NodeList.cpp | 53 ++++++++++++++------------- 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index f4a02ad805..ee8c2e1ecf 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -827,26 +827,28 @@ void NodeList::ignoreNodeBySessionID(const QUuid& nodeID, bool ignoreEnabled) { }); if (ignoreEnabled) { - { - QReadLocker ignoredSetLocker{ &_ignoredSetLock }; // read lock for insert - // add this nodeID to our set of ignored IDs - _ignoredNodeIDs.insert(nodeID); - } - { - QReadLocker personalMutedSetLocker{ &_personalMutedSetLock }; // read lock for insert - // add this nodeID to our set of personal muted IDs - _personalMutedNodeIDs.insert(nodeID); - } + // add this nodeID to our set of ignored IDs + _ignoredSetLock.lockForRead(); + _ignoredNodeIDs.insert(nodeID); + _ignoredSetLock.unlock(); + + // add this nodeID to our set of personal muted IDs + _personalMutedSetLock.lockForRead(); + _personalMutedNodeIDs.insert(nodeID); + _personalMutedSetLock.unlock(); + emit ignoredNode(nodeID, true); } else { - { - QWriteLocker ignoredSetLocker{ &_ignoredSetLock }; // write lock for unsafe_erase - _ignoredNodeIDs.unsafe_erase(nodeID); - } - { - QWriteLocker personalMutedSetLocker{ &_personalMutedSetLock }; // write lock for unsafe_erase - _personalMutedNodeIDs.unsafe_erase(nodeID); - } + // write lock for unsafe_erase + _ignoredSetLock.lockForWrite(); + _ignoredNodeIDs.unsafe_erase(nodeID); + _ignoredSetLock.unlock(); + + // write lock for unsafe_erase + _personalMutedSetLock.lockForWrite(); + _personalMutedNodeIDs.unsafe_erase(nodeID); + _personalMutedSetLock.unlock(); + emit ignoredNode(nodeID, false); } @@ -858,14 +860,13 @@ void NodeList::ignoreNodeBySessionID(const QUuid& nodeID, bool ignoreEnabled) { void NodeList::removeFromIgnoreMuteSets(const QUuid& nodeID) { // don't remove yourself, or nobody if (!nodeID.isNull() && _sessionUUID != nodeID) { - { - QWriteLocker ignoredSetLocker{ &_ignoredSetLock }; - _ignoredNodeIDs.unsafe_erase(nodeID); - } - { - QWriteLocker personalMutedSetLocker{ &_personalMutedSetLock }; - _personalMutedNodeIDs.unsafe_erase(nodeID); - } + _ignoredSetLock.lockForWrite(); + _ignoredNodeIDs.unsafe_erase(nodeID); + _ignoredSetLock.unlock(); + + _personalMutedSetLock.lockForWrite(); + _personalMutedNodeIDs.unsafe_erase(nodeID); + _personalMutedSetLock.unlock(); } } From 8612302f07798cc5cc25cdad738b652d61043635 Mon Sep 17 00:00:00 2001 From: David Kelly Date: Fri, 3 Feb 2017 10:08:46 -0700 Subject: [PATCH 3/3] Revert "I like this better" This reverts commit 069c85c7c250773f1302870d3679e1d1fd187c62. --- libraries/networking/src/NodeList.cpp | 53 +++++++++++++-------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index ee8c2e1ecf..f4a02ad805 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -827,28 +827,26 @@ void NodeList::ignoreNodeBySessionID(const QUuid& nodeID, bool ignoreEnabled) { }); if (ignoreEnabled) { - // add this nodeID to our set of ignored IDs - _ignoredSetLock.lockForRead(); - _ignoredNodeIDs.insert(nodeID); - _ignoredSetLock.unlock(); - - // add this nodeID to our set of personal muted IDs - _personalMutedSetLock.lockForRead(); - _personalMutedNodeIDs.insert(nodeID); - _personalMutedSetLock.unlock(); - + { + QReadLocker ignoredSetLocker{ &_ignoredSetLock }; // read lock for insert + // add this nodeID to our set of ignored IDs + _ignoredNodeIDs.insert(nodeID); + } + { + QReadLocker personalMutedSetLocker{ &_personalMutedSetLock }; // read lock for insert + // add this nodeID to our set of personal muted IDs + _personalMutedNodeIDs.insert(nodeID); + } emit ignoredNode(nodeID, true); } else { - // write lock for unsafe_erase - _ignoredSetLock.lockForWrite(); - _ignoredNodeIDs.unsafe_erase(nodeID); - _ignoredSetLock.unlock(); - - // write lock for unsafe_erase - _personalMutedSetLock.lockForWrite(); - _personalMutedNodeIDs.unsafe_erase(nodeID); - _personalMutedSetLock.unlock(); - + { + QWriteLocker ignoredSetLocker{ &_ignoredSetLock }; // write lock for unsafe_erase + _ignoredNodeIDs.unsafe_erase(nodeID); + } + { + QWriteLocker personalMutedSetLocker{ &_personalMutedSetLock }; // write lock for unsafe_erase + _personalMutedNodeIDs.unsafe_erase(nodeID); + } emit ignoredNode(nodeID, false); } @@ -860,13 +858,14 @@ void NodeList::ignoreNodeBySessionID(const QUuid& nodeID, bool ignoreEnabled) { void NodeList::removeFromIgnoreMuteSets(const QUuid& nodeID) { // don't remove yourself, or nobody if (!nodeID.isNull() && _sessionUUID != nodeID) { - _ignoredSetLock.lockForWrite(); - _ignoredNodeIDs.unsafe_erase(nodeID); - _ignoredSetLock.unlock(); - - _personalMutedSetLock.lockForWrite(); - _personalMutedNodeIDs.unsafe_erase(nodeID); - _personalMutedSetLock.unlock(); + { + QWriteLocker ignoredSetLocker{ &_ignoredSetLock }; + _ignoredNodeIDs.unsafe_erase(nodeID); + } + { + QWriteLocker personalMutedSetLocker{ &_personalMutedSetLock }; + _personalMutedNodeIDs.unsafe_erase(nodeID); + } } }