StreamSocket::checkRemoval: Signal shutdown only if appropriate, allo…

…wing to drain bytes instead of hard disconnect Signal shutdown only if appropriate, allowing to drain bytes instead of hard disconnect. Only in case of SigUtil::getTerminationFlag(), we force disconnection. SocketPoll::poll() hence checks isClosed() after checkRemoval() for forced Socket removal. A pending signaled shutdown may occure via handlePoll() after processing more I/O data. Further, remove useless setClosed() in checkRemoval() after asserting !isOpen(). Signed-off-by: Sven Göthel <[email protected]> Change-Id: I4e3ba962880b77017443236bc33b03ed20dcdd34
CollaboraOnline · Oct 25, 2024 · f81ec5c · f81ec5c
1 parent 105e6ea
commit f81ec5c
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 15 deletions.
diff --git a/net/Socket.cpp b/net/Socket.cpp
@@ -686,10 +686,10 @@ int SocketPoll::poll(int64_t timeoutMaxMicroS)
                          << " of " << _pollSockets.size() << ") from " << _name);
                 _pollSockets[i] = nullptr;
             }
-            else if( _pollSockets[i]->checkRemoval(newNow) )
+            else if( _pollSockets[i]->checkRemoval(newNow) && _pollSockets[i]->isClosed() )
             {
-                // timed out socket .. also checks
-                // ProtocolHandlerInterface
+                // Socket to be removed (timeout) w/ immediate shutdown/disconnect,
+                /// also checks ProtocolHandlerInterface
                 // - http::Session::checkTimeout() OK
                 // - WebSocketHandler::checkTimeout() OK
                 ++itemsErased;
@@ -1518,26 +1518,32 @@ bool StreamSocket::checkRemoval(std::chrono::steady_clock::time_point now)
             if (_socketHandler)
             {
                 _socketHandler->onDisconnect();
-                if( isOpen() ) {
+                if( isOpen() )
+                {
                     // Note: Ensure proper semantics of onDisconnect()
-                    LOG_WRN("Socket still open post onDisconnect(), forced shutdown.");
-                    shutdown(); // signal
-                    closeConnection(); // real -> setClosed()
+                    if( isTermination )
+                    {
+                        LOG_WRN("Socket still open post onDisconnect(), forced disconnect.");
+                        closeConnection();
+                    }
+                    else if( !isShutdownSignalled() )
+                    {
+                        LOG_WRN("Socket still open post onDisconnect(), forced shutdown-signal.");
+                        shutdown(); // signal shutdown only, allowing to drain remaining bytes
+                    }
                 }
             }
+            else if( isTermination )
+                closeConnection();
             else
-            {
-                shutdown(); // signal
-                closeConnection(); // real -> setClosed()
-            }
-            assert(isOpen() == false); // should have issued shutdown
+                shutdown(); // signal shutdown only, allowing to drain remaining bytes
+
             return true;
         }
     }
     if (_socketHandler && _socketHandler->checkTimeout(now))
     {
         assert(isOpen() == false); // should have issued shutdown
-        setClosed();
         LOG_WRN("CheckRemoval: Timeout: " << getStatsString(now) << ", " << *this);
         return true;
     }

diff --git a/net/Socket.hpp b/net/Socket.hpp
@@ -211,8 +211,11 @@ class Socket
         recv = _bytesRcvd;
     }
 
-    /// Checks whether socket is due for forced removal, e.g. by internal timeout or small throughput. Method will shutdown connection and socket on forced removal.
-    /// Returns true in case of forced removal, caller shall stop processing
+    /// Checks whether socket is due for forced removal, e.g. by internal timeout or small throughput.
+    /// Method will either shutdown connection and socket immediately for forced removal,
+    /// signal a pending shutdown or keeping the socket alive.
+    /// Returns true in case of immediate disconnect or signaled pending shutdown, otherwise false.
+    /// See isClosed()
     virtual bool checkRemoval(std::chrono::steady_clock::time_point /* now */) { return false; }
 
     /// Shutdown the socket.