Handle situation where callback is invoked before hostname is populat…

…ed (#1829)
awslabs · Oct 12, 2023 · b82753f · b82753f
1 parent 8e28d52
commit b82753f
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 36 deletions.
diff --git a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h
@@ -348,7 +348,7 @@ extern "C" {
 #define STATUS_PEERCONNECTION_CREATE_ANSWER_WITHOUT_REMOTE_DESCRIPTION STATUS_PEERCONNECTION_BASE + 0x00000001
 #define STATUS_PEERCONNECTION_CODEC_INVALID                            STATUS_PEERCONNECTION_BASE + 0x00000002
 #define STATUS_PEERCONNECTION_CODEC_MAX_EXCEEDED                       STATUS_PEERCONNECTION_BASE + 0x00000003
-#define STATUS_PEERCONNECTION_UNSUPPORTED_HOSTNAME                     STATUS_PEERCONNECTION_BASE + 0x00000004
+#define STATUS_PEERCONNECTION_EARLY_DNS_RESOLUTION_FAILED              STATUS_PEERCONNECTION_BASE + 0x00000004
 /*!@} */
 
 /////////////////////////////////////////////////////

diff --git a/src/source/Ice/IceUtils.c b/src/source/Ice/IceUtils.c
@@ -256,7 +256,8 @@ STATUS parseIceServer(PIceServer pIceServer, PCHAR url, PCHAR username, PCHAR cr
 
     // Adding a NULL_ARG check specifically to cover for the case where early STUN
     // resolution might not be enabled
-    if (retStatus == STATUS_NULL_ARG || pIceServer->setIpFn == NULL) {
+    // Also cover the case where hostname is not resolved because the request was made too soon
+    if (retStatus == STATUS_NULL_ARG || retStatus == STATUS_PEERCONNECTION_EARLY_DNS_RESOLUTION_FAILED || pIceServer->setIpFn == NULL) {
         // Reset the retStatus to ensure the appropriate status code is returned from
         // getIpWithHostName
         retStatus = STATUS_SUCCESS;

diff --git a/src/source/PeerConnection/PeerConnection.c b/src/source/PeerConnection/PeerConnection.c
@@ -787,19 +787,26 @@ STATUS onSetStunServerIp(UINT64 customData, PCHAR url, PKvsIpAddress pIpAddr)
     MUTEX_LOCK(pWebRtcClientContext->stunCtxlock);
     locked = TRUE;
 
-    CHK(STRCMP(url, pWebRtcClientContext->pStunIpAddrCtx->hostname) == 0, STATUS_PEERCONNECTION_UNSUPPORTED_HOSTNAME);
-
-    if (pWebRtcClientContext->pStunIpAddrCtx->isIpInitialized) {
-        DLOGI("Initialized successfully");
-        if (currentTime > (pWebRtcClientContext->pStunIpAddrCtx->startTime + pWebRtcClientContext->pStunIpAddrCtx->expirationDuration)) {
-            DLOGI("Expired...need to refresh STUN address");
-            // Reset start time
-            pWebRtcClientContext->pStunIpAddrCtx->startTime = 0;
-            CHK_ERR(getStunAddr(pWebRtcClientContext->pStunIpAddrCtx) == STATUS_SUCCESS, retStatus, "Failed to resolve after cache expiry");
-        }
-        MEMCPY(pIpAddr, &pWebRtcClientContext->pStunIpAddrCtx->kvsIpAddr, SIZEOF(pWebRtcClientContext->pStunIpAddrCtx->kvsIpAddr));
+    // This covers a situation where say we receive a URL that is not the default STUN or the hostname is not populated
+    // pWebRtcClientContext->pStunIpAddrCtx->status needs to be set to ensure we do not go ahead with resolution on thread
+    // in case we receive the request early on
+    if (STRCMP(url, pWebRtcClientContext->pStunIpAddrCtx->hostname) != 0) {
+        retStatus = STATUS_PEERCONNECTION_EARLY_DNS_RESOLUTION_FAILED;
+        // This is to ensure we do not go ahead with STUN resolution if this call is already made
+        pWebRtcClientContext->pStunIpAddrCtx->status = STATUS_PEERCONNECTION_EARLY_DNS_RESOLUTION_FAILED;
     } else {
-        DLOGE("Initialization failed");
+        if (pWebRtcClientContext->pStunIpAddrCtx->isIpInitialized) {
+            DLOGI("Initialized successfully");
+            if (currentTime > (pWebRtcClientContext->pStunIpAddrCtx->startTime + pWebRtcClientContext->pStunIpAddrCtx->expirationDuration)) {
+                DLOGI("Expired...need to refresh STUN address");
+                // Reset start time
+                pWebRtcClientContext->pStunIpAddrCtx->startTime = 0;
+                CHK_ERR(getStunAddr(pWebRtcClientContext->pStunIpAddrCtx) == STATUS_SUCCESS, retStatus, "Failed to resolve after cache expiry");
+            }
+            MEMCPY(pIpAddr, &pWebRtcClientContext->pStunIpAddrCtx->kvsIpAddr, SIZEOF(pWebRtcClientContext->pStunIpAddrCtx->kvsIpAddr));
+        } else {
+            DLOGE("Initialization failed");
+        }
     }
 CleanUp:
     if (locked) {
@@ -822,30 +829,33 @@ PVOID resolveStunIceServerIp(PVOID args)
     if (ATOMIC_LOAD_BOOL(&pWebRtcClientContext->isContextInitialized)) {
         MUTEX_LOCK(pWebRtcClientContext->stunCtxlock);
         locked = TRUE;
-
         if (pWebRtcClientContext->pStunIpAddrCtx == NULL) {
             DLOGE("Failed to resolve STUN IP address because webrtc client instance was not created");
         } else {
-            if ((pRegion = GETENV(DEFAULT_REGION_ENV_VAR)) == NULL) {
-                pRegion = DEFAULT_AWS_REGION;
-            }
-
-            pHostnamePostfix = KINESIS_VIDEO_STUN_URL_POSTFIX;
-            // If region is in CN, add CN region uri postfix
-            if (STRSTR(pRegion, "cn-")) {
-                pHostnamePostfix = KINESIS_VIDEO_STUN_URL_POSTFIX_CN;
-            }
-
-            SNPRINTF(pWebRtcClientContext->pStunIpAddrCtx->hostname, SIZEOF(pWebRtcClientContext->pStunIpAddrCtx->hostname),
-                     KINESIS_VIDEO_STUN_URL_WITHOUT_PORT, pRegion, pHostnamePostfix);
-            if (getStunAddr(pWebRtcClientContext->pStunIpAddrCtx) == STATUS_SUCCESS) {
-                getIpAddrStr(&pWebRtcClientContext->pStunIpAddrCtx->kvsIpAddr, addressResolved, ARRAY_SIZE(addressResolved));
-                DLOGI("ICE Server address for %s with getaddrinfo: %s", pWebRtcClientContext->pStunIpAddrCtx->hostname, addressResolved);
-                pWebRtcClientContext->pStunIpAddrCtx->isIpInitialized = TRUE;
+            if (pWebRtcClientContext->pStunIpAddrCtx->status != STATUS_PEERCONNECTION_EARLY_DNS_RESOLUTION_FAILED) {
+                if ((pRegion = GETENV(DEFAULT_REGION_ENV_VAR)) == NULL) {
+                    pRegion = DEFAULT_AWS_REGION;
+                }
+
+                pHostnamePostfix = KINESIS_VIDEO_STUN_URL_POSTFIX;
+                // If region is in CN, add CN region uri postfix
+                if (STRSTR(pRegion, "cn-")) {
+                    pHostnamePostfix = KINESIS_VIDEO_STUN_URL_POSTFIX_CN;
+                }
+
+                SNPRINTF(pWebRtcClientContext->pStunIpAddrCtx->hostname, SIZEOF(pWebRtcClientContext->pStunIpAddrCtx->hostname),
+                         KINESIS_VIDEO_STUN_URL_WITHOUT_PORT, pRegion, pHostnamePostfix);
+                if (getStunAddr(pWebRtcClientContext->pStunIpAddrCtx) == STATUS_SUCCESS) {
+                    getIpAddrStr(&pWebRtcClientContext->pStunIpAddrCtx->kvsIpAddr, addressResolved, ARRAY_SIZE(addressResolved));
+                    DLOGI("ICE Server address for %s with getaddrinfo: %s", pWebRtcClientContext->pStunIpAddrCtx->hostname, addressResolved);
+                    pWebRtcClientContext->pStunIpAddrCtx->isIpInitialized = TRUE;
+                } else {
+                    DLOGE("Failed to resolve %s", pWebRtcClientContext->pStunIpAddrCtx->hostname);
+                }
+                pWebRtcClientContext->pStunIpAddrCtx->startTime = GETTIME();
             } else {
-                DLOGE("Failed to resolve %s", pWebRtcClientContext->pStunIpAddrCtx->hostname);
+                DLOGW("Request already received to get the URL before resolution could even start...allowing higher layers to handle resolution");
             }
-            pWebRtcClientContext->pStunIpAddrCtx->startTime = GETTIME();
         }
         if (locked) {
             MUTEX_UNLOCK(pWebRtcClientContext->stunCtxlock);

diff --git a/tst/MetricsApiTest.cpp b/tst/MetricsApiTest.cpp
@@ -22,7 +22,7 @@ TEST_F(MetricsApiTest, webRtcGetMetrics)
 
     MEMSET(&configuration, 0x00, SIZEOF(RtcConfiguration));
 
-    EXPECT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection));
+    ASSERT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection));
 
     EXPECT_EQ(STATUS_NULL_ARG, rtcPeerConnectionGetMetrics(pRtcPeerConnection, NULL, NULL));
 
@@ -57,7 +57,7 @@ TEST_F(MetricsApiTest, webRtcIceServerGetMetrics)
     STRNCPY(configuration.iceServers[1].credential, (PCHAR) "username", MAX_ICE_CONFIG_CREDENTIAL_LEN);
     STRNCPY(configuration.iceServers[1].username, (PCHAR) "password", MAX_ICE_CONFIG_USER_NAME_LEN);
 
-    EXPECT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection));
+    ASSERT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection));
 
     EXPECT_EQ(STATUS_ICE_SERVER_INDEX_INVALID, rtcPeerConnectionGetMetrics(pRtcPeerConnection, NULL, &rtcIceMetrics));
 
@@ -90,10 +90,15 @@ TEST_F(MetricsApiTest, webRtcIceCandidateGetMetrics)
     STRNCPY(configuration.iceServers[0].credential, (PCHAR) "", MAX_ICE_CONFIG_CREDENTIAL_LEN);
     STRNCPY(configuration.iceServers[0].username, (PCHAR) "", MAX_ICE_CONFIG_USER_NAME_LEN);
 
-    EXPECT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection));
+    ASSERT_EQ(STATUS_SUCCESS, createPeerConnection(&configuration, &pRtcPeerConnection));
 
     pIceAgent = ((PKvsPeerConnection) pRtcPeerConnection)->pIceAgent;
 
+    if(pIceAgent == NULL) {
+        DLOGI("ICE Agent null");
+    } else {
+        DLOGI("ICE agent not null");
+    }
     IceCandidate localCandidate;
     IceCandidate remoteCandidate;
     IceCandidatePair iceCandidatePair;