Move cs_vSend lock into SocketSendData

troygiorshev · troygiorshev · commit 9d47d6da524b · 2020-11-03T11:57:51.000-05:00
SocketSendData requires cs_vSend to be locked, but it places this
responsibility with the caller.  This is unnecessary, unclear, and has
caused us to accidentally add RecordBytesSent under the lock.

This commit fixes both problems.

Justification is needed in the case of PushMessage.  It is ok that we
let go of cs_vSend for a moment before locking it again and calling
SocketSendData.  In the case where optimistic_send == false, this lock
is let go of for a long time between adding messages to vSendMsg and the
call to SocketSendData (in SocketHandler).  As corrected in the comment
change, optimistic_send represents whether vSendMsg *was* empty, not
*is* empty (as, of course, we've just added to it).

If SocketHandler grabs cs_vSend in the middle then SocketSendData will
be empty when we call it in PushMessage.  As suggested by the "if
(bytes_sent)" check, this is fine.
diff --git a/src/net.cpp b/src/net.cpp
@@ -747,8 +747,9 @@ void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vec
     CVectorWriter{SER_NETWORK, INIT_PROTO_VERSION, header, 0, hdr};
 }
 
-size_t CConnman::SocketSendData(CNode *pnode) const EXCLUSIVE_LOCKS_REQUIRED(pnode->cs_vSend)
+size_t CConnman::SocketSendData(CNode *pnode) const
 {
+    LOCK(pnode->cs_vSend);
     auto it = pnode->vSendMsg.begin();
     size_t nSentSize = 0;
 
@@ -1445,11 +1446,8 @@ void CConnman::SocketHandler()
         //
         if (sendSet)
         {
-            LOCK(pnode->cs_vSend);
-            size_t nBytes = SocketSendData(pnode);
-            if (nBytes) {
-                RecordBytesSent(nBytes);
-            }
+            const size_t bytes_sent{SocketSendData(pnode)};
+            if (bytes_sent) RecordBytesSent(bytes_sent);
         }
 
         InactivityCheck(pnode);
@@ -2821,10 +2819,10 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
     pnode->m_serializer->prepareForTransport(msg, serialized_header);
     const size_t total_size{message_size + serialized_header.size()};
 
-    size_t bytes_sent{0};
+    bool optimistic_send{false};
     {
         LOCK(pnode->cs_vSend);
-        bool optimistic_send{pnode->vSendMsg.empty()};
+        optimistic_send = pnode->vSendMsg.empty();
 
         //log total amount of bytes per message type
         pnode->mapSendBytesPerMsgCmd[msg.m_type] += total_size;
@@ -2837,13 +2835,12 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
         if (message_size) {
             pnode->vSendMsg.push_back(std::move(msg.data));
         }
-
-        // If write queue empty, attempt "optimistic write"
-        if (optimistic_send) {
-            bytes_sent = SocketSendData(pnode);
-        }
     }
-    if (bytes_sent) RecordBytesSent(bytes_sent);
+    // If write queue was empty, attempt "optimistic write"
+    if (optimistic_send) {
+        const size_t bytes_sent{SocketSendData(pnode)};
+        if (bytes_sent) RecordBytesSent(bytes_sent);
+    }
 }
 
 bool CConnman::ForNode(NodeId id, std::function<bool(CNode* pnode)> func)
