Split CNode::cs_vSend: message processing and message sending

TheBlueMatt · furszy · commit ec23964c508c · 2020-08-01T01:18:34.000-03:00
cs_vSend is used for two purposes - to lock the datastructures used
to queue messages to place on the wire and to only call
SendMessages once at a time per-node. I believe SendMessages used
to access some of the vSendMsg stuff, but it doesn't anymore, so
these locks do not need to be on the same mutex, and also make
deadlocking much more likely.
diff --git a/src/net.cpp b/src/net.cpp
@@ -1225,8 +1225,8 @@ void ThreadSocketHandler()
                 // * We wait for data to be received (and disconnect after timeout).
                 // * We process a message in the buffer (message handler thread).
                 {
-                    TRY_LOCK(pnode->cs_vSend, lockSend);
-                    if (lockSend && !pnode->vSendMsg.empty()) {
+                    LOCK(pnode->cs_vSend);
+                    if (!pnode->vSendMsg.empty()) {
                         FD_SET(pnode->hSocket, &fdsetSend);
                         continue;
                     }
@@ -1320,9 +1320,8 @@ void ThreadSocketHandler()
             if (pnode->hSocket == INVALID_SOCKET)
                 continue;
             if (FD_ISSET(pnode->hSocket, &fdsetSend)) {
-                TRY_LOCK(pnode->cs_vSend, lockSend);
-                if (lockSend)
-                    SocketSendData(pnode);
+                LOCK(pnode->cs_vSend);
+                SocketSendData(pnode);
             }
 
             //
@@ -1842,7 +1841,7 @@ void ThreadMessageHandler()
 
             // Send messages
             {
-                TRY_LOCK(pnode->cs_vSend, lockSend);
+                TRY_LOCK(pnode->cs_sendProcessing, lockSend);
                 if (lockSend)
                     GetNodeSignals().SendMessages(pnode);
             }
diff --git a/src/net.h b/src/net.h
@@ -321,6 +321,8 @@ class CNode
     std::deque<CSerializeData> vSendMsg;
     RecursiveMutex cs_vSend;
 
+    RecursiveMutex cs_sendProcessing;
+
     std::deque<CInv> vRecvGetData;
     std::deque<CNetMessage> vRecvMsg;
     RecursiveMutex cs_vRecvMsg;

Original file line number	Diff line number	Diff line change
`@@ -1225,8 +1225,8 @@ void ThreadSocketHandler()`
`1225`	`1225`	`// * We wait for data to be received (and disconnect after timeout).`
`1226`	`1226`	`// * We process a message in the buffer (message handler thread).`
`1227`	`1227`	`{`
`1228`		`- TRY_LOCK(pnode->cs_vSend, lockSend);`
`1229`		`- if (lockSend && !pnode->vSendMsg.empty()) {`
	`1228`	`+ LOCK(pnode->cs_vSend);`
	`1229`	`+ if (!pnode->vSendMsg.empty()) {`
`1230`	`1230`	`FD_SET(pnode->hSocket, &fdsetSend);`
`1231`	`1231`	`continue;`
`1232`	`1232`	`}`
`@@ -1320,9 +1320,8 @@ void ThreadSocketHandler()`
`1320`	`1320`	`if (pnode->hSocket == INVALID_SOCKET)`
`1321`	`1321`	`continue;`
`1322`	`1322`	`if (FD_ISSET(pnode->hSocket, &fdsetSend)) {`
`1323`		`- TRY_LOCK(pnode->cs_vSend, lockSend);`
`1324`		`- if (lockSend)`
`1325`		`- SocketSendData(pnode);`
	`1323`	`+ LOCK(pnode->cs_vSend);`
	`1324`	`+ SocketSendData(pnode);`
`1326`	`1325`	`}`
`1327`	`1326`
`1328`	`1327`	`//`
`@@ -1842,7 +1841,7 @@ void ThreadMessageHandler()`
`1842`	`1841`
`1843`	`1842`	`// Send messages`
`1844`	`1843`	`{`
`1845`		`- TRY_LOCK(pnode->cs_vSend, lockSend);`
	`1844`	`+ TRY_LOCK(pnode->cs_sendProcessing, lockSend);`
`1846`	`1845`	`if (lockSend)`
`1847`	`1846`	`GetNodeSignals().SendMessages(pnode);`
`1848`	`1847`	`}`