Address last bit of outstanding TCP cleanup

src/inet/TCPEndPoint.cpp

@@ -322,10 +322,8 @@ CHIP_ERROR TCPEndPoint::DriveSending()
     return err;
 }
 
-void TCPEndPoint::DriveReceiving()
+void TCPEndPoint::DriveReceiving(const TCPEndPointHandle & handle)
 {
-    TCPEndPointHandle handle(this);
-
     // If there's data in the receive queue and the app is ready to receive it then call the app's callback
     // with the entire receive queue.
     if (!mRcvQueue.IsNull() && mReceiveEnabled && OnDataReceived != nullptr)
@@ -438,7 +436,8 @@ void TCPEndPoint::DoClose(CHIP_ERROR err, bool suppressCallback)
                       oldState == State::kClosing) &&
                      OnConnectionClosed != nullptr)
             {
-                OnConnectionClosed(*this, err);
+                TCPEndPointHandle handle(this);
+                OnConnectionClosed(handle, err);
             }
         }
     }

src/inet/TCPEndPoint.h

@@ -199,7 +199,8 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis<TCPEndPoint>
     void EnableReceive()
     {
         mReceiveEnabled = true;
-        DriveReceiving();
+        TCPEndPointHandle handle(this);
+        DriveReceiving(handle);
     }
 
     /**
@@ -455,7 +456,7 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis<TCPEndPoint>
      *  member to process connection termination events on \c endPoint. The
      *  \c err argument distinguishes successful terminations from failures.
      */
-    typedef void (*OnConnectionClosedFunct)(TCPEndPoint & endPoint, CHIP_ERROR err);
+    typedef void (*OnConnectionClosedFunct)(const TCPEndPointHandle & endPoint, CHIP_ERROR err);
 
     /** The endpoint's close event handling function delegate. */
     OnConnectionClosedFunct OnConnectionClosed;
@@ -469,7 +470,7 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis<TCPEndPoint>
      *  Provide a function of this type to the \c OnPeerClose delegate member
      *  to process connection termination events on \c endPoint.
      */
-    typedef void (*OnPeerCloseFunct)(TCPEndPoint & endPoint);
+    typedef void (*OnPeerCloseFunct)(const TCPEndPointHandle & endPoint);
 
     /** The endpoint's half-close receive event handling function delegate. */
     OnPeerCloseFunct OnPeerClose;
@@ -497,15 +498,15 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis<TCPEndPoint>
     /**
      * @brief   Type of connection acceptance error event handling function.
      *
-     * @param[in]   endPoint    The TCP endpoint associated with the event.
-     * @param[in]   err         The reason for the error.
+     * @param[in]   listeningEndpoint    The listening TCP endpoint associated with the event.
+     * @param[in]   err                  The reason for the error.
      *
      * @details
      *  Provide a function of this type to the \c OnAcceptError delegate
-     *  member to process connection acceptance error events on \c endPoint. The
+     *  member to process connection acceptance error events on \c listeningEndpoint. The
      *  \c err argument provides specific detail about the type of the error.
      */
-    typedef void (*OnAcceptErrorFunct)(const TCPEndPointHandle & endPoint, CHIP_ERROR err);
+    typedef void (*OnAcceptErrorFunct)(const TCPEndPointHandle & listeningEndpoint, CHIP_ERROR err);
 
     /**
      * The endpoint's connection acceptance event handling function delegate.
@@ -593,7 +594,8 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis<TCPEndPoint>
     TCPEndPoint(const TCPEndPoint &) = delete;
 
     CHIP_ERROR DriveSending();
-    void DriveReceiving();
+    // Take in a handle to self to guarantee this sticks around until we're done
+    void DriveReceiving(const TCPEndPointHandle & handle);
     void HandleConnectComplete(CHIP_ERROR err);
     void HandleAcceptError(CHIP_ERROR err);
     void DoClose(CHIP_ERROR err, bool suppressCallback);

src/inet/TCPEndPointImplLwIP.cpp

@@ -653,53 +653,56 @@ void TCPEndPointImplLwIP::HandleDataSent(uint16_t lenSent)
 void TCPEndPointImplLwIP::HandleDataReceived(System::PacketBufferHandle && buf)
 {
     // Only receive new data while in the Connected or SendShutdown states.
-    if (mState == State::kConnected || mState == State::kSendShutdown)
+    if (mState != State::kConnected && mState != State::kSendShutdown)
     {
-        // Mark the connection as being active.
-        MarkActive();
+        return;
+    }
+
+    TCPEndPointHandle handle(this);
+    // Mark the connection as being active.
+    MarkActive();
 
-        // If we received a data buffer, queue it on the receive queue.  If there's already data in
-        // the queue, compact the data into the head buffer.
-        if (!buf.IsNull())
+    // If we received a data buffer, queue it on the receive queue.  If there's already data in
+    // the queue, compact the data into the head buffer.
+    if (!buf.IsNull())
+    {
+        if (mRcvQueue.IsNull())
         {
-            if (mRcvQueue.IsNull())
-            {
-                mRcvQueue = std::move(buf);
-            }
-            else
-            {
-                mRcvQueue->AddToEnd(std::move(buf));
-                mRcvQueue->CompactHead();
-            }
+            mRcvQueue = std::move(buf);
         }
-
-        // Otherwise buf == NULL means the other side closed the connection, so ...
         else
         {
-
-            // If in the Connected state and the app has provided an OnPeerClose callback,
-            // enter the ReceiveShutdown state.  Providing an OnPeerClose callback allows
-            // the app to decide whether to keep the send side of the connection open after
-            // the peer has closed. If no OnPeerClose is provided, we assume that the app
-            // wants to close both directions and automatically enter the Closing state.
-            if (mState == State::kConnected && OnPeerClose != nullptr)
-            {
-                mState = State::kReceiveShutdown;
-            }
-            else
-            {
-                mState = State::kClosing;
-            }
-            // Call the app's OnPeerClose.
-            if (OnPeerClose != NULL)
-            {
-                OnPeerClose(*this);
-            }
+            mRcvQueue->AddToEnd(std::move(buf));
+            mRcvQueue->CompactHead();
         }
+    }
 
-        // Drive the received data into the app.
-        DriveReceiving();
+    // Otherwise buf == NULL means the other side closed the connection, so ...
+    else
+    {
+
+        // If in the Connected state and the app has provided an OnPeerClose callback,
+        // enter the ReceiveShutdown state.  Providing an OnPeerClose callback allows
+        // the app to decide whether to keep the send side of the connection open after
+        // the peer has closed. If no OnPeerClose is provided, we assume that the app
+        // wants to close both directions and automatically enter the Closing state.
+        if (mState == State::kConnected && OnPeerClose != nullptr)
+        {
+            mState = State::kReceiveShutdown;
+        }
+        else
+        {
+            mState = State::kClosing;
+        }
+        // Call the app's OnPeerClose.
+        if (OnPeerClose != NULL)
+        {
+            OnPeerClose(handle);
+        }
     }
+
+    // Drive the received data into the app.
+    DriveReceiving(handle);
 }
 
 void TCPEndPointImplLwIP::HandleIncomingConnection(const TCPEndPointHandle & conEP)

src/inet/TCPEndPointImplSockets.cpp

@@ -904,6 +904,7 @@ void TCPEndPointImplSockets::ReceiveData()
         RestartTCPUserTimeoutTimer();
     }
 #endif // INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT
+    TCPEndPointHandle handle(this);
     // If an error occurred, abort the connection.
     if (rcvLen < 0)
     {
@@ -921,7 +922,6 @@ void TCPEndPointImplSockets::ReceiveData()
 
         DoClose(CHIP_ERROR_POSIX(systemErrno), false);
     }
-
     else
     {
         // Mark the connection as being active.
@@ -948,7 +948,7 @@ void TCPEndPointImplSockets::ReceiveData()
             // Call the app's OnPeerClose.
             if (OnPeerClose != nullptr)
             {
-                OnPeerClose(*this);
+                OnPeerClose(handle);
             }
         }
 
@@ -978,7 +978,7 @@ void TCPEndPointImplSockets::ReceiveData()
     }
 
     // Drive any received data into the app.
-    DriveReceiving();
+    DriveReceiving(handle);
 }
 
 CHIP_ERROR TCPEndPointImplSockets::HandleIncomingConnection()

src/inet/tests/inet-layer-test-tool.cpp

@@ -547,7 +547,7 @@ void HandleTCPConnectionComplete(const TCPEndPointHandle & aEndPoint, CHIP_ERROR
     }
 }
 
-static void HandleTCPConnectionClosed(TCPEndPoint & aEndPoint, CHIP_ERROR aError)
+static void HandleTCPConnectionClosed(const TCPEndPointHandle & aEndPoint, CHIP_ERROR aError)
 {
     if (aError == CHIP_NO_ERROR)
     {

src/transport/raw/TCP.cpp

@@ -594,10 +594,10 @@ void TCPBase::HandleTCPEndPointConnectComplete(const Inet::TCPEndPointHandle & e
     tcp->HandleConnectionAttemptComplete(activeConnection, CHIP_NO_ERROR);
 }
 
-void TCPBase::HandleTCPEndPointConnectionClosed(Inet::TCPEndPoint & endPoint, CHIP_ERROR err)
+void TCPBase::HandleTCPEndPointConnectionClosed(const Inet::TCPEndPointHandle & endPoint, CHIP_ERROR err)
 {
-    TCPBase * tcp                              = reinterpret_cast<TCPBase *>(endPoint.mAppState);
-    ActiveTCPConnectionHandle activeConnection = tcp->FindInUseConnection(endPoint);
+    TCPBase * tcp                              = reinterpret_cast<TCPBase *>(endPoint->mAppState);
+    ActiveTCPConnectionHandle activeConnection = tcp->FindInUseConnection(*endPoint);
 
     if (activeConnection.IsNull())
     {
@@ -617,56 +617,53 @@ void TCPBase::HandleIncomingConnection(const Inet::TCPEndPointHandle & listenEnd
                                        const Inet::IPAddress & peerAddress, uint16_t peerPort)
 {
     TCPBase * tcp = reinterpret_cast<TCPBase *>(listenEndPoint->mAppState);
-    // To handle errors around endpoint in the calls to HandleAcceptError, we must initialize mAppState
-    endPoint->mAppState = tcp;
+    ReturnAndLogOnFailure(tcp->DoHandleIncomingConnection(listenEndPoint, endPoint, peerAddress, peerPort), Inet,
+                          "Failure accepting incoming connection");
+}
 
+CHIP_ERROR TCPBase::DoHandleIncomingConnection(const Inet::TCPEndPointHandle & listenEndPoint,
+                                               const Inet::TCPEndPointHandle & endPoint, const Inet::IPAddress & peerAddress,
+                                               uint16_t peerPort)
+{
     PeerAddress addr;
-    CHIP_ERROR err = GetPeerAddress(*endPoint, addr);
-    VerifyOrReturn(err == CHIP_NO_ERROR, HandleAcceptError(endPoint, err));
-    ActiveTCPConnectionState * activeConnection = tcp->AllocateConnection(endPoint, addr);
-    if (activeConnection != nullptr)
+    ReturnErrorOnFailure(GetPeerAddress(*endPoint, addr));
+    ActiveTCPConnectionState * activeConnection = AllocateConnection(endPoint, addr);
+    if (activeConnection == nullptr)
     {
-        auto connectionCleanup = ScopeExit([&]() { activeConnection->Free(); });
+        return CHIP_ERROR_TOO_MANY_CONNECTIONS;
+    }
+
+    auto connectionCleanup = ScopeExit([&]() { activeConnection->Free(); });
+
+    endPoint->mAppState          = this;
+    endPoint->OnDataReceived     = HandleTCPEndPointDataReceived;
+    endPoint->OnDataSent         = nullptr;
+    endPoint->OnConnectionClosed = HandleTCPEndPointConnectionClosed;
 
-        endPoint->OnDataReceived     = HandleTCPEndPointDataReceived;
-        endPoint->OnDataSent         = nullptr;
-        endPoint->OnConnectionClosed = HandleTCPEndPointConnectionClosed;
+    // By default, disable TCP Nagle buffering by setting TCP_NODELAY socket option to true
+    endPoint->EnableNoDelay();
 
-        // By default, disable TCP Nagle buffering by setting TCP_NODELAY socket option to true
-        endPoint->EnableNoDelay();
+    mUsedEndPointCount++;
+    activeConnection->mConnectionState = TCPState::kConnected;
 
-        tcp->mUsedEndPointCount++;
-        activeConnection->mConnectionState = TCPState::kConnected;
+    // Set the TCPKeepalive configurations on the received connection
+    endPoint->EnableKeepAlive(activeConnection->mTCPKeepAliveIntervalSecs, activeConnection->mTCPMaxNumKeepAliveProbes);
 
-        // Set the TCPKeepalive configurations on the received connection
-        endPoint->EnableKeepAlive(activeConnection->mTCPKeepAliveIntervalSecs, activeConnection->mTCPMaxNumKeepAliveProbes);
+    char addrStr[Transport::PeerAddress::kMaxToStringSize];
+    peerAddress.ToString(addrStr);
+    ChipLogProgress(Inet, "Incoming connection established with peer at %s.", addrStr);
 
-        char addrStr[Transport::PeerAddress::kMaxToStringSize];
-        peerAddress.ToString(addrStr);
-        ChipLogProgress(Inet, "Incoming connection established with peer at %s.", addrStr);
+    // Call the upper layer handler for incoming connection received.
+    HandleConnectionReceived(*activeConnection);
 
-        // Call the upper layer handler for incoming connection received.
-        tcp->HandleConnectionReceived(*activeConnection);
+    connectionCleanup.release();
 
-        connectionCleanup.release();
-    }
-    else
-    {
-        ChipLogError(Inet, "Insufficient connection space to accept new connections.");
-        HandleAcceptError(endPoint, CHIP_ERROR_TOO_MANY_CONNECTIONS);
-    }
+    return CHIP_NO_ERROR;
 }
 
-void TCPBase::HandleAcceptError(const Inet::TCPEndPointHandle & endPoint, CHIP_ERROR err)
+void TCPBase::HandleAcceptError(const Inet::TCPEndPointHandle & listeningEndpoint, CHIP_ERROR err)
 {
     ChipLogError(Inet, "Accept error: %" CHIP_ERROR_FORMAT, err.Format());
-    TCPBase * tcp = reinterpret_cast<TCPBase *>(endPoint->mAppState);
-
-    ActiveTCPConnectionState * conn = tcp->FindActiveConnection(endPoint);
-    if (conn != nullptr)
-    {
-        tcp->CloseConnectionInternal(*conn, err, SuppressCallback::No);
-    }
 }
 
 CHIP_ERROR TCPBase::TCPConnect(const PeerAddress & address, Transport::AppTCPConnectionCallbackCtxt * appState,

src/transport/raw/TCP.h

@@ -290,13 +290,16 @@ class DLL_EXPORT TCPBase : public Base
 
     // Callback handler for TCPEndPoint. Called when a connection has been closed.
     // @see TCPEndpoint::OnConnectionClosedFunct
-    static void HandleTCPEndPointConnectionClosed(Inet::TCPEndPoint & endPoint, CHIP_ERROR err);
+    static void HandleTCPEndPointConnectionClosed(const Inet::TCPEndPointHandle & endPoint, CHIP_ERROR err);
 
     // Callback handler for TCPEndPoint. Called when a connection is received on the listening port.
     // @see TCPEndpoint::OnConnectionReceivedFunct
     static void HandleIncomingConnection(const Inet::TCPEndPointHandle & listenEndPoint, const Inet::TCPEndPointHandle & endPoint,
                                          const Inet::IPAddress & peerAddress, uint16_t peerPort);
 
+    CHIP_ERROR DoHandleIncomingConnection(const Inet::TCPEndPointHandle & listenEndPoint, const Inet::TCPEndPointHandle & endPoint,
+                                          const Inet::IPAddress & peerAddress, uint16_t peerPort);
+
     // Callback handler for handling accept error
     // @see TCPEndpoint::OnAcceptErrorFunct
     static void HandleAcceptError(const Inet::TCPEndPointHandle & endPoint, CHIP_ERROR err);