From a2d07f224a8dd40d3d0da16f8d700661b1d4c93b Mon Sep 17 00:00:00 2001 From: Divya Sampath Kumar Date: Thu, 12 Oct 2023 12:11:28 -0700 Subject: [PATCH] Handle situation where callback is invoked before hostname is populated --- .../kinesis/video/webrtcclient/Include.h | 2 +- src/source/Ice/IceUtils.c | 3 +- src/source/PeerConnection/PeerConnection.c | 72 +++++++++++-------- tst/MetricsApiTest.cpp | 11 ++- 4 files changed, 52 insertions(+), 36 deletions(-) diff --git a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h index 34a558c45b..1de9326c39 100644 --- 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 index f6fee583ab..60f0bcb6e9 100644 --- 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 index 67b8316eb5..c79eb24634 100644 --- 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 index 0e014421e3..7484d2db94 100644 --- 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;