From e2c3e2570f40d160fe592fa0a271b365943f6516 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Thu, 4 Nov 2021 13:34:22 -0400 Subject: [PATCH 1/3] for non retriable failures, set the terminal exit status for state in state machine --- src/source/Signaling/LwsApiCalls.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/source/Signaling/LwsApiCalls.c b/src/source/Signaling/LwsApiCalls.c index c05862ecfd..9f72ceb23b 100644 --- a/src/source/Signaling/LwsApiCalls.c +++ b/src/source/Signaling/LwsApiCalls.c @@ -643,6 +643,10 @@ STATUS lwsCompleteSync(PLwsCallInfo pCallInfo) return retStatus; } +BOOL isLwsCallResultFailureRetryable(PCallInfo pCallInfo) { + return (STRSTR(pCallInfo->responseData, "Signature expired") == NULL); +} + ////////////////////////////////////////////////////////////////////////// // API calls ////////////////////////////////////////////////////////////////////////// @@ -689,6 +693,11 @@ STATUS describeChannelLws(PSignalingClient pSignalingClient, UINT64 time) pResponseStr = pLwsCallInfo->callInfo.responseData; resultLen = pLwsCallInfo->callInfo.responseDataLen; + if (!isLwsCallResultFailureRetryable(&pLwsCallInfo->callInfo)) { + // Return early with terminal status code so there will be no lower level retries + CHK(FALSE, STATUS_SIGNALING_DESCRIBE_CALL_FAILED); + } + // Early return if we have a non-success result CHK((SERVICE_CALL_RESULT) ATOMIC_LOAD(&pSignalingClient->result) == SERVICE_CALL_RESULT_OK && resultLen != 0 && pResponseStr != NULL, STATUS_SIGNALING_LWS_CALL_FAILED); @@ -838,6 +847,12 @@ STATUS createChannelLws(PSignalingClient pSignalingClient, UINT64 time) pResponseStr = pLwsCallInfo->callInfo.responseData; resultLen = pLwsCallInfo->callInfo.responseDataLen; + + if (!isLwsCallResultFailureRetryable(&pLwsCallInfo->callInfo)) { + // Return early with terminal status code so there will be no lower level retries + CHK(FALSE, STATUS_SIGNALING_CREATE_CALL_FAILED); + } + // Early return if we have a non-success result CHK((SERVICE_CALL_RESULT) ATOMIC_LOAD(&pSignalingClient->result) == SERVICE_CALL_RESULT_OK && resultLen != 0 && pResponseStr != NULL, STATUS_SIGNALING_LWS_CALL_FAILED); @@ -906,6 +921,8 @@ STATUS getChannelEndpointLws(PSignalingClient pSignalingClient, UINT64 time) SIGNALING_SERVICE_API_CALL_CONNECTION_TIMEOUT, SIGNALING_SERVICE_API_CALL_COMPLETION_TIMEOUT, DEFAULT_LOW_SPEED_LIMIT, DEFAULT_LOW_SPEED_TIME_LIMIT, pSignalingClient->pAwsCredentials, &pRequestInfo)); + pRequestInfo->currentTime -= (10 * HUNDREDS_OF_NANOS_IN_A_MINUTE); + CHK_STATUS(createLwsCallInfo(pSignalingClient, pRequestInfo, PROTOCOL_INDEX_HTTPS, &pLwsCallInfo)); // Make a blocking call @@ -916,6 +933,11 @@ STATUS getChannelEndpointLws(PSignalingClient pSignalingClient, UINT64 time) pResponseStr = pLwsCallInfo->callInfo.responseData; resultLen = pLwsCallInfo->callInfo.responseDataLen; + if (!isLwsCallResultFailureRetryable(&pLwsCallInfo->callInfo)) { + // Return early with terminal status code so there will be no lower level retries + CHK(FALSE, STATUS_SIGNALING_GET_ENDPOINT_CALL_FAILED); + } + // Early return if we have a non-success result CHK((SERVICE_CALL_RESULT) ATOMIC_LOAD(&pSignalingClient->result) == SERVICE_CALL_RESULT_OK && resultLen != 0 && pResponseStr != NULL, STATUS_SIGNALING_LWS_CALL_FAILED); @@ -1051,6 +1073,11 @@ STATUS getIceConfigLws(PSignalingClient pSignalingClient, UINT64 time) pResponseStr = pLwsCallInfo->callInfo.responseData; resultLen = pLwsCallInfo->callInfo.responseDataLen; + if (!isLwsCallResultFailureRetryable(&pLwsCallInfo->callInfo)) { + // Return early with terminal status code so there will be no lower level retries + CHK(FALSE, STATUS_SIGNALING_GET_ICE_CONFIG_CALL_FAILED); + } + // Early return if we have a non-success result CHK((SERVICE_CALL_RESULT) ATOMIC_LOAD(&pSignalingClient->result) == SERVICE_CALL_RESULT_OK && resultLen != 0 && pResponseStr != NULL, STATUS_SIGNALING_LWS_CALL_FAILED); From 1b6f1cf7bff4679fc887331668c5f1372608d905 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Thu, 4 Nov 2021 15:16:17 -0400 Subject: [PATCH 2/3] address comments --- src/source/Signaling/LwsApiCalls.c | 33 ++++++++++-------------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/src/source/Signaling/LwsApiCalls.c b/src/source/Signaling/LwsApiCalls.c index 9f72ceb23b..b19fac9a34 100644 --- a/src/source/Signaling/LwsApiCalls.c +++ b/src/source/Signaling/LwsApiCalls.c @@ -643,8 +643,8 @@ STATUS lwsCompleteSync(PLwsCallInfo pCallInfo) return retStatus; } -BOOL isLwsCallResultFailureRetryable(PCallInfo pCallInfo) { - return (STRSTR(pCallInfo->responseData, "Signature expired") == NULL); +BOOL isCallResultFailureRetryable(PCallInfo pCallInfo) { + return (STRNSTR(pCallInfo->responseData, "Signature expired", pCallInfo->responseDataLen) == NULL); } ////////////////////////////////////////////////////////////////////////// @@ -693,10 +693,8 @@ STATUS describeChannelLws(PSignalingClient pSignalingClient, UINT64 time) pResponseStr = pLwsCallInfo->callInfo.responseData; resultLen = pLwsCallInfo->callInfo.responseDataLen; - if (!isLwsCallResultFailureRetryable(&pLwsCallInfo->callInfo)) { - // Return early with terminal status code so there will be no lower level retries - CHK(FALSE, STATUS_SIGNALING_DESCRIBE_CALL_FAILED); - } + CHK_ERR(isCallResultFailureRetryable(&pLwsCallInfo->callInfo), STATUS_SIGNALING_DESCRIBE_CALL_FAILED, + "DescribeChannel API call failed with: %s and will not be retried.", pResponseStr); // Early return if we have a non-success result CHK((SERVICE_CALL_RESULT) ATOMIC_LOAD(&pSignalingClient->result) == SERVICE_CALL_RESULT_OK && resultLen != 0 && pResponseStr != NULL, STATUS_SIGNALING_LWS_CALL_FAILED); @@ -847,11 +845,8 @@ STATUS createChannelLws(PSignalingClient pSignalingClient, UINT64 time) pResponseStr = pLwsCallInfo->callInfo.responseData; resultLen = pLwsCallInfo->callInfo.responseDataLen; - - if (!isLwsCallResultFailureRetryable(&pLwsCallInfo->callInfo)) { - // Return early with terminal status code so there will be no lower level retries - CHK(FALSE, STATUS_SIGNALING_CREATE_CALL_FAILED); - } + CHK_ERR(isCallResultFailureRetryable(&pLwsCallInfo->callInfo), STATUS_SIGNALING_CREATE_CALL_FAILED, + "CreateChannel API call failed with: %s and will not be retried.", pResponseStr); // Early return if we have a non-success result CHK((SERVICE_CALL_RESULT) ATOMIC_LOAD(&pSignalingClient->result) == SERVICE_CALL_RESULT_OK && resultLen != 0 && pResponseStr != NULL, STATUS_SIGNALING_LWS_CALL_FAILED); @@ -921,8 +916,6 @@ STATUS getChannelEndpointLws(PSignalingClient pSignalingClient, UINT64 time) SIGNALING_SERVICE_API_CALL_CONNECTION_TIMEOUT, SIGNALING_SERVICE_API_CALL_COMPLETION_TIMEOUT, DEFAULT_LOW_SPEED_LIMIT, DEFAULT_LOW_SPEED_TIME_LIMIT, pSignalingClient->pAwsCredentials, &pRequestInfo)); - pRequestInfo->currentTime -= (10 * HUNDREDS_OF_NANOS_IN_A_MINUTE); - CHK_STATUS(createLwsCallInfo(pSignalingClient, pRequestInfo, PROTOCOL_INDEX_HTTPS, &pLwsCallInfo)); // Make a blocking call @@ -933,10 +926,8 @@ STATUS getChannelEndpointLws(PSignalingClient pSignalingClient, UINT64 time) pResponseStr = pLwsCallInfo->callInfo.responseData; resultLen = pLwsCallInfo->callInfo.responseDataLen; - if (!isLwsCallResultFailureRetryable(&pLwsCallInfo->callInfo)) { - // Return early with terminal status code so there will be no lower level retries - CHK(FALSE, STATUS_SIGNALING_GET_ENDPOINT_CALL_FAILED); - } + CHK_ERR(isCallResultFailureRetryable(&pLwsCallInfo->callInfo), STATUS_SIGNALING_GET_ENDPOINT_CALL_FAILED, + "GetChannelEndpoint API call failed with: %s and will not be retried.", pResponseStr); // Early return if we have a non-success result CHK((SERVICE_CALL_RESULT) ATOMIC_LOAD(&pSignalingClient->result) == SERVICE_CALL_RESULT_OK && resultLen != 0 && pResponseStr != NULL, STATUS_SIGNALING_LWS_CALL_FAILED); @@ -1072,11 +1063,9 @@ STATUS getIceConfigLws(PSignalingClient pSignalingClient, UINT64 time) ATOMIC_STORE(&pSignalingClient->result, (SIZE_T) pLwsCallInfo->callInfo.callResult); pResponseStr = pLwsCallInfo->callInfo.responseData; resultLen = pLwsCallInfo->callInfo.responseDataLen; - - if (!isLwsCallResultFailureRetryable(&pLwsCallInfo->callInfo)) { - // Return early with terminal status code so there will be no lower level retries - CHK(FALSE, STATUS_SIGNALING_GET_ICE_CONFIG_CALL_FAILED); - } + + CHK_ERR(isCallResultFailureRetryable(&pLwsCallInfo->callInfo), STATUS_SIGNALING_GET_ICE_CONFIG_CALL_FAILED, + "GetIceConfig API call failed with: %s and will not be retried.", pResponseStr); // Early return if we have a non-success result CHK((SERVICE_CALL_RESULT) ATOMIC_LOAD(&pSignalingClient->result) == SERVICE_CALL_RESULT_OK && resultLen != 0 && pResponseStr != NULL, STATUS_SIGNALING_LWS_CALL_FAILED); From 392734dd6b2f1d54722b1250a7d2c81db02e1db6 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Tue, 9 Nov 2021 14:14:12 -0500 Subject: [PATCH 3/3] adjust tests set retry max to 1 --- .../kinesis/video/webrtcclient/Include.h | 2 +- src/source/Signaling/LwsApiCalls.c | 2 +- tst/SignalingApiFunctionalityTest.cpp | 22 +++++++++---------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h index 38f5ec39a3..363f95c88f 100644 --- a/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h +++ b/src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h @@ -629,7 +629,7 @@ extern "C" { /** * Signaling states default retry count. This will evaluate to the last call being made 20 seconds in which will hit a timeout first. */ -#define SIGNALING_STATES_DEFAULT_RETRY_COUNT 10 +#define SIGNALING_STATES_DEFAULT_RETRY_COUNT 1 /** * Signaling caching policy default TTL period diff --git a/src/source/Signaling/LwsApiCalls.c b/src/source/Signaling/LwsApiCalls.c index b19fac9a34..0ef4a269bc 100644 --- a/src/source/Signaling/LwsApiCalls.c +++ b/src/source/Signaling/LwsApiCalls.c @@ -1063,7 +1063,7 @@ STATUS getIceConfigLws(PSignalingClient pSignalingClient, UINT64 time) ATOMIC_STORE(&pSignalingClient->result, (SIZE_T) pLwsCallInfo->callInfo.callResult); pResponseStr = pLwsCallInfo->callInfo.responseData; resultLen = pLwsCallInfo->callInfo.responseDataLen; - + CHK_ERR(isCallResultFailureRetryable(&pLwsCallInfo->callInfo), STATUS_SIGNALING_GET_ICE_CONFIG_CALL_FAILED, "GetIceConfig API call failed with: %s and will not be retried.", pResponseStr); diff --git a/tst/SignalingApiFunctionalityTest.cpp b/tst/SignalingApiFunctionalityTest.cpp index 9bd44acb6a..fe5d4b3fa1 100644 --- a/tst/SignalingApiFunctionalityTest.cpp +++ b/tst/SignalingApiFunctionalityTest.cpp @@ -1449,7 +1449,7 @@ TEST_F(SignalingApiFunctionalityTest, iceServerConfigRefreshNotConnectedAuthExpi // Check the states - we should have failed on get credentials EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_NEW]); - EXPECT_EQ(23, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_CREDENTIALS]); + EXPECT_EQ(5, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_CREDENTIALS]); EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_DESCRIBE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_CREATE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ENDPOINT]); @@ -1466,7 +1466,7 @@ TEST_F(SignalingApiFunctionalityTest, iceServerConfigRefreshNotConnectedAuthExpi EXPECT_NE((UINT64) NULL, (UINT64) pIceConfigInfo); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_NEW]); - EXPECT_EQ(23, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_CREDENTIALS]); + EXPECT_EQ(5, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_CREDENTIALS]); EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_DESCRIBE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_CREATE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ENDPOINT]); @@ -1572,7 +1572,7 @@ TEST_F(SignalingApiFunctionalityTest, iceServerConfigRefreshConnectedAuthExpirat // Check the states - we should have failed on get credentials EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_NEW]); - EXPECT_EQ(23, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_CREDENTIALS]); + EXPECT_EQ(5, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_CREDENTIALS]); EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_DESCRIBE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_CREATE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ENDPOINT]); @@ -1589,7 +1589,7 @@ TEST_F(SignalingApiFunctionalityTest, iceServerConfigRefreshConnectedAuthExpirat EXPECT_NE((UINT64) NULL, (UINT64) pIceConfigInfo); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_NEW]); - EXPECT_EQ(23, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_CREDENTIALS]); + EXPECT_EQ(5, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_CREDENTIALS]); EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_DESCRIBE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_CREATE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ENDPOINT]); @@ -1641,7 +1641,7 @@ TEST_F(SignalingApiFunctionalityTest, iceServerConfigRefreshNotConnectedWithFaul // Make it fail after the first call and recover after two failures on the 3rd call getIceConfigResult = STATUS_INVALID_OPERATION; getIceConfigFail = 1; - getIceConfigRecover = 3; + getIceConfigRecover = 1; MEMSET(&channelInfo, 0x00, SIZEOF(ChannelInfo)); channelInfo.version = CHANNEL_INFO_CURRENT_VERSION; @@ -1688,7 +1688,7 @@ TEST_F(SignalingApiFunctionalityTest, iceServerConfigRefreshNotConnectedWithFaul EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_DESCRIBE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_CREATE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ENDPOINT]); - EXPECT_EQ(4, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ICE_CONFIG]); + EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ICE_CONFIG]); EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_READY]); EXPECT_EQ(0, signalingStatesCounts[SIGNALING_CLIENT_STATE_CONNECTING]); EXPECT_EQ(0, signalingStatesCounts[SIGNALING_CLIENT_STATE_CONNECTED]); @@ -1701,7 +1701,7 @@ TEST_F(SignalingApiFunctionalityTest, iceServerConfigRefreshNotConnectedWithFaul EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_DESCRIBE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_CREATE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ENDPOINT]); - EXPECT_EQ(4, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ICE_CONFIG]); + EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ICE_CONFIG]); EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_READY]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_CONNECTING]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_CONNECTED]); @@ -1755,7 +1755,7 @@ TEST_F(SignalingApiFunctionalityTest, iceServerConfigRefreshConnectedWithFaultIn // Make it fail after the first call and recover after two failures on the 3rd call getIceConfigResult = STATUS_INVALID_OPERATION; getIceConfigFail = 1; - getIceConfigRecover = 3; + getIceConfigRecover = 1; MEMSET(&channelInfo, 0x00, SIZEOF(ChannelInfo)); channelInfo.version = CHANNEL_INFO_CURRENT_VERSION; @@ -1817,7 +1817,7 @@ TEST_F(SignalingApiFunctionalityTest, iceServerConfigRefreshConnectedWithFaultIn EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_DESCRIBE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_CREATE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ENDPOINT]); - EXPECT_EQ(4, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ICE_CONFIG]); + EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ICE_CONFIG]); EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_READY]); EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_CONNECTING]); EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_CONNECTED]); @@ -2894,7 +2894,7 @@ TEST_F(SignalingApiFunctionalityTest, deleteChannelCreatedAuthExpiration) // Check the states - we should have failed on get credentials EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_NEW]); - EXPECT_EQ(12, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_CREDENTIALS]); + EXPECT_EQ(3, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_CREDENTIALS]); EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_DESCRIBE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_CREATE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ENDPOINT]); @@ -2914,7 +2914,7 @@ TEST_F(SignalingApiFunctionalityTest, deleteChannelCreatedAuthExpiration) // Check the states - we should have got the credentials now and directly moved to delete EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_NEW]); - EXPECT_EQ(13, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_CREDENTIALS]); + EXPECT_EQ(4, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_CREDENTIALS]); EXPECT_EQ(2, signalingStatesCounts[SIGNALING_CLIENT_STATE_DESCRIBE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_CREATE]); EXPECT_EQ(1, signalingStatesCounts[SIGNALING_CLIENT_STATE_GET_ENDPOINT]);