Merge pull request #18926 from jakesmith/HPCC-32315-MP-GC-fix

HPCC-32315 Fix MP graceful close follow on [traced] errors Reviewed-by: Mark Kelly [email protected] Reviewed-by: Gavin Halliday <[email protected]> Merged-by: Gavin Halliday <[email protected]>
hpcc-systems · Jul 31, 2024 · 027c58f · 027c58f
2 parents 9a2ea8c + 739334f
commit 027c58f
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 19 deletions.
diff --git a/system/jlib/jsocket.cpp b/system/jlib/jsocket.cpp
@@ -2768,7 +2768,9 @@ void CSocket::shutdown(unsigned mode)
 
 void CSocket::shutdownNoThrow(unsigned mode)
 {
-    if (state == ss_open) {
+    if (state == ss_open)
+    {
+        state = ss_shutdown;
 #ifdef SOCKTRACE
         PROGLOG("SOCKTRACE: shutdown(%d) socket %x %d (%p)", mode, sock, sock, this);
 #endif

diff --git a/system/mp/mpcomm.cpp b/system/mp/mpcomm.cpp
@@ -1846,6 +1846,7 @@ class CMPPacketReader: public ISocketSelectNotify, public CInterface
     {
         if (!parent)
             return false;
+        bool gc = false; // if a gc is hit, then will fall through to close socket
         try
         {
             while (true) // NB: breaks out if blocked (if (remaining) ..)
@@ -1870,11 +1871,11 @@ class CMPPacketReader: public ISocketSelectNotify, public CInterface
                 if (!gotPacketHdr)
                 {
                     CCycleTimer timer;
-                    sock->readtms(activeptr, 0, remaining, szRead, timer.remainingMs(60000));
+                    gc = readtmsAllowClose(sock, activeptr, 0, remaining, szRead, timer.remainingMs(60000));
                     remaining -= szRead;
                     activeptr += szRead;
                     if (remaining) // only possible if blocked.
-                        return false; // wait for next notification
+                        break; // wait for next notification
 
                     PacketHeader &hdr = *(PacketHeader *)activemsg->bufferBase();
                     if (hdr.version/0x100 != MP_PROTOCOL_VERSION/0x100)
@@ -1896,14 +1897,14 @@ class CMPPacketReader: public ISocketSelectNotify, public CInterface
                     gotPacketHdr = true;
                 }
 
-                if (remaining)
+                if (!gc && remaining)
                 {
-                    sock->readtms(activeptr, 0, remaining, szRead, WAIT_FOREVER);
+                    gc = readtmsAllowClose(sock, activeptr, 0, remaining, szRead, WAIT_FOREVER);
                     remaining -= szRead;
                     activeptr += szRead;
                 }
                 if (remaining) // only possible if blocked.
-                    return false; // wait for next notification
+                    break; // wait for next notification
 
 #ifdef _FULLTRACE
                 LOG(MCdebugInfo, "MP: ReadPacket(timetaken = %d,select iterations=%d)",msTick()-parent->startxfer,parent->numiter);
@@ -1937,6 +1938,8 @@ class CMPPacketReader: public ISocketSelectNotify, public CInterface
                     }
                 }
                 while (activemsg);
+                if (gc)
+                    break;
             }
         }
         catch (IException *e)
@@ -1947,24 +1950,27 @@ class CMPPacketReader: public ISocketSelectNotify, public CInterface
             gotPacketHdr = false;
         }
 
-        // here due to error or graceful close, so close socket (ignore error as may be closed already)
-        try
+        if (gc)
         {
-            Linked<CMPChannel> pc;
+            // here due to error or graceful close, so close socket (ignore error as may be closed already)
+            try
             {
-                CriticalBlock block(sect);
-                if (parent)
+                Linked<CMPChannel> pc;
                 {
-                    pc.set(parent);     // don't want channel to disappear during call
-                    parent = NULL;
+                    CriticalBlock block(sect);
+                    if (parent)
+                    {
+                        pc.set(parent);     // don't want channel to disappear during call
+                        parent = NULL;
+                    }
                 }
+                if (pc)
+                    pc->closeSocket(false, true);
+            }
+            catch (IException *e)
+            {
+                e->Release();
             }
-            if (pc)
-                pc->closeSocket(false, true);
-        }
-        catch (IException *e)
-        {
-            e->Release();
         }
         return false;
     }

diff --git a/system/security/securesocket/securesocket.cpp b/system/security/securesocket/securesocket.cpp
@@ -774,6 +774,7 @@ void CSecureSocket::handleError(int ssl_err, bool writing, bool wait, unsigned t
     {
         case SSL_ERROR_ZERO_RETURN:
         {
+            m_socket->shutdownNoThrow();
             THROWJSOCKEXCEPTION(JSOCKERR_graceful_close);
         }
         case SSL_ERROR_WANT_READ: // NB: SSL_write can provoke SSL_ERROR_WANT_READ
@@ -928,6 +929,7 @@ void CSecureSocket::readtms(void* buf, size32_t min_size, size32_t max_size, siz
         }
         else if (0 == rc)
         {
+            m_socket->shutdownNoThrow();
             if (suppresGCIfMinSize && (sizeRead >= min_size))
                 break;
             THROWJSOCKEXCEPTION(JSOCKERR_graceful_close);