TCP: Make HandleConnCloseTest[46] work reliably on Darwin and related fixes (project-chip#42136)

* TestTCP: Retry with different ports on EADDRINUSE

* TestTCP: Make HandleConnCloseTest[46] work reliably on Darwin

Wait until after HandleConnectionReceived() is called before closing the client
connection, otherwise we can't be sure the connection actually gets accepted at
the application level (as opposed to in the kernel) on the server side.

Also add some logging to the callbacks, and check that the callbacks are
actually being called with the expected handles. Remove some unused test code.

* TCPConnect base implementation should not return CHIP_NO_ERROR

* Darwin: Detect saLen == 0 from accept() and ignore the connection

* TCPTest: Add test for "late" failure (in TCP.DoHandleIncomingConnection)

* TCP: Remove the fallback for GetPeerAddress

The updated tests show this is no longer needed. I encountered no test failures
in 1000 repetitions on either Linux or Darwin. Certain test scenarios have a
chance of logging a "Failure accepting incoming connection" error, but these
are cases where the client-side socket is being closed as soon as it's
connected. Particularly on Darwin this may then cause the GetPeerAddress call
to fail (Linux doesn't seem to exhibit this behavior.)

* From review: make test macro docs less verbose

Author: ksperling-apple

src/inet/TCPEndPointImplSockets.cpp

@@ -1010,6 +1010,12 @@ CHIP_ERROR TCPEndPointImplSockets::HandleIncomingConnection()
         return CHIP_ERROR_POSIX(errno);
     }
 
+#ifdef __APPLE__
+    // On Darwin, if the client closes the connection just as it is being accepted, it is
+    // possible for accept() to return a socket but saLen is 0. Ignore the connection.
+    VerifyOrReturnError(saLen != 0, CHIP_NO_ERROR);
+#endif
+
     // If there's no callback available, fail with an error.
     VerifyOrReturnError(OnConnectionReceived != nullptr, CHIP_ERROR_NO_CONNECTION_HANDLER);
 

src/lib/support/tests/ExtraPwTestMacros.h

@@ -15,6 +15,7 @@
  *    See the License for the specific language governing permissions and
  *    limitations under the License.
  */
+
 #pragma once
 
 /**
@@ -61,7 +62,6 @@
  *
  * This macro does not define the body of the test function. It can be used to
  * run a single test case which needs to be run against different fixtures.
- *
  */
 #define TEST_F_FROM_FIXTURE_NO_BODY(test_fixture, test_name)                                                                       \
     TEST_F(test_fixture, test_name)                                                                                                \
@@ -70,33 +70,11 @@
     }
 
 /**
- *  @def EXPECT_SUCCESS(expr)
- *
- *  @brief
- *    Shorthand for EXPECT_EQ(expr, CHIP_NO_ERROR)
- *
- *  Example usage:
- *
- *  @code
- *    EXPECT_SUCCESS(channel->SendMsg(msg));
- *  @endcode
- *
- *  @param[in]  expr        A scalar expression to be evaluated against CHIP_NO_ERROR.
+ * Shorthand for EXPECT_EQ(expr, CHIP_NO_ERROR)
  */
 #define EXPECT_SUCCESS(expr) EXPECT_EQ((expr), CHIP_NO_ERROR)
 
 /**
- *  @def EXPECT_SUCCESS(expr)
- *
- *  @brief
- *    Shorthand for EXPECT_EQ(expr, CHIP_NO_ERROR)
- *
- *  Example usage:
- *
- *  @code
- *    EXPECT_SUCCESS(channel->SendMsg(msg));
- *  @endcode
- *
- *  @param[in]  expr        A scalar expression to be evaluated against CHIP_NO_ERROR.
+ * Shorthand for ASSERT_EQ(expr, CHIP_NO_ERROR)
  */
-#define EXPECT_SUCCESS(expr) EXPECT_EQ((expr), CHIP_NO_ERROR)
+#define ASSERT_SUCCESS(expr) ASSERT_EQ((expr), CHIP_NO_ERROR)

src/transport/raw/Base.h

@@ -97,12 +97,12 @@ class Base
 
 #if INET_CONFIG_ENABLE_TCP_ENDPOINT
     /**
-     * Connect to the specified peer.
+     * Connect to the specified peer. On success, outPeerConnHandle will be populated with a handle to the connection.
      */
     virtual CHIP_ERROR TCPConnect(const PeerAddress & address, Transport::AppTCPConnectionCallbackCtxt * appState,
-                                  ActiveTCPConnectionHandle & peerConnState)
+                                  ActiveTCPConnectionHandle & outPeerConnHandle)
     {
-        return CHIP_NO_ERROR;
+        return CHIP_ERROR_INTERNAL;
     }
 #endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
 

src/transport/raw/TCP.cpp

@@ -632,21 +632,19 @@ CHIP_ERROR TCPBase::DoHandleIncomingConnection(const Inet::TCPEndPointHandle & l
                                                const Inet::TCPEndPointHandle & endPoint, const Inet::IPAddress & peerAddress,
                                                uint16_t peerPort)
 {
-    PeerAddress addr;
-    CHIP_ERROR getPeerError = GetPeerAddress(*endPoint, addr);
-    // See https://github.com/project-chip/connectedhomeip/issues/41746
-    // Failures here must be handled carefully so that broken connections
-    // continue to propagate to failure callbacks to avoid flaky tests
-    if (getPeerError != CHIP_NO_ERROR)
+#if INET_CONFIG_TEST
+    if (sForceFailureInDoHandleIncomingConnection)
     {
-        ChipLogFailure(getPeerError, Inet, "Failure getting peer info, using fallback");
-        addr = PeerAddress::TCP(peerAddress, peerPort, Inet::InterfaceId::Null());
+        return CHIP_ERROR_INTERNAL;
     }
+#endif
+
+    // GetPeerAddress may fail if the client has already closed the connection, just drop it.
+    PeerAddress addr;
+    ReturnErrorOnFailure(GetPeerAddress(*endPoint, addr));
+
     ActiveTCPConnectionState * activeConnection = AllocateConnection(endPoint, addr);
-    if (activeConnection == nullptr)
-    {
-        return CHIP_ERROR_TOO_MANY_CONNECTIONS;
-    }
+    VerifyOrReturnError(activeConnection != nullptr, CHIP_ERROR_TOO_MANY_CONNECTIONS);
 
     auto connectionCleanup = ScopeExit([&]() { activeConnection->Free(); });
 
@@ -741,5 +739,9 @@ void TCPBase::InitEndpoint(const Inet::TCPEndPointHandle & endpoint)
     endpoint->SetConnectTimeout(mConnectTimeout);
 }
 
+#if INET_CONFIG_TEST
+bool TCPBase::sForceFailureInDoHandleIncomingConnection = false;
+#endif
+
 } // namespace Transport
 } // namespace chip

src/transport/raw/TCP.h

@@ -202,6 +202,10 @@ class DLL_EXPORT TCPBase : public Base
      */
     void CloseActiveConnections();
 
+#if INET_CONFIG_TEST
+    static bool sForceFailureInDoHandleIncomingConnection;
+#endif
+
 private:
     // Allow tests to access private members.
     template <size_t kActiveConnectionsSize, size_t kPendingPacketSize>