diff --git a/AampUtils.cpp b/AampUtils.cpp index 7e54aec70a..a39d6c8d3b 100644 --- a/AampUtils.cpp +++ b/AampUtils.cpp @@ -1546,3 +1546,27 @@ bool IsCurlTimeoutFailure( int httpResponseCode ) return false; } } + +/** + * @brief Check if a curl failure is retryable based on transient transport errors + * @param[in] curlCode curl error code + * @retval true if the error is retryable + * @retval false otherwise + */ +bool IsRetryableCurlFailure( CURLcode curlCode ) +{ + switch( curlCode ) + { + case CURLE_OPERATION_TIMEDOUT: + case CURLE_PARTIAL_FILE: + case CURLE_COULDNT_RESOLVE_PROXY: + case CURLE_COULDNT_RESOLVE_HOST: + case CURLE_COULDNT_CONNECT: + case CURLE_RECV_ERROR: + case CURLE_SEND_ERROR: + case CURLE_GOT_NOTHING: + return true; + default: + return false; + } +} diff --git a/AampUtils.h b/AampUtils.h index 9928523fe2..878142f260 100644 --- a/AampUtils.h +++ b/AampUtils.h @@ -469,4 +469,6 @@ CurlTimeoutFailureReason GetCurlTimeoutFailureReason(CURL* curl); bool IsCurlTimeoutFailure( int httpResponseCode ); +bool IsRetryableCurlFailure( CURLcode curlCode ); + #endif /* __AAMP_UTILS_H__ */ diff --git a/downloader/AampCurlDownloader.cpp b/downloader/AampCurlDownloader.cpp index 4931e8dd09..f707e16bb3 100644 --- a/downloader/AampCurlDownloader.cpp +++ b/downloader/AampCurlDownloader.cpp @@ -115,7 +115,6 @@ long aamp_CurlEasyGetinfoLong( CURL *handle, CURLINFO info ) } return rc; } - char *aamp_CurlEasyGetinfoString( CURL *handle, CURLINFO info ) { char *rc = NULL; @@ -368,7 +367,8 @@ int AampCurlDownloader::Download(const std::string &urlStr, std::shared_ptr(httpRetVal)) || + IsCurlTimeoutFailure(httpRetVal)) { AAMPLOG_WARN("Download failed due to curl error %d numDownloadAttempts %d numRetriesAllowed %d", httpRetVal, numDownloadAttempts, numRetriesAllowed); loopAgain = true; diff --git a/priv_aamp.cpp b/priv_aamp.cpp index 2e986f39c1..b11f05f15a 100644 --- a/priv_aamp.cpp +++ b/priv_aamp.cpp @@ -4990,10 +4990,11 @@ bool PrivateInstanceAAMP::GetFile( std::string remoteUrl, AampMediaType mediaTyp print_headerResponse(context.allResponseHeaders, mediaType); } - if (res == CURLE_COULDNT_CONNECT || IsCurlTimeoutFailure(res) || + const bool isRetryableStall = (isDownloadStalled && - (eCURL_ABORT_REASON_LOW_BANDWIDTH_TIMEDOUT != abortReason)) || - res == CURLE_SEND_ERROR) + (eCURL_ABORT_REASON_LOW_BANDWIDTH_TIMEDOUT != abortReason)); + if (IsRetryableCurlFailure(res) || IsCurlTimeoutFailure(res) || + isRetryableStall) { if(mpStreamAbstractionAAMP) diff --git a/test/utests/fakes/FakeAampUtils.cpp b/test/utests/fakes/FakeAampUtils.cpp index a2c7f89856..b00c81b7f2 100644 --- a/test/utests/fakes/FakeAampUtils.cpp +++ b/test/utests/fakes/FakeAampUtils.cpp @@ -652,5 +652,9 @@ bool IsCurlTimeoutFailure( int httpResponseCode ) { return true; } +bool IsRetryableCurlFailure( CURLcode curlCode ) +{ + return true; +} // aamp_ApplyPageHttpHeaders not actually part of AampUtils.cpp, but fake declared here for convenience extern "C" void aamp_ApplyPageHttpHeaders(PlayerInstanceAAMP *aamp){} diff --git a/test/utests/tests/AampCurlDownloader/FunctionalTests.cpp b/test/utests/tests/AampCurlDownloader/FunctionalTests.cpp index 010fbef860..a9301ec11f 100644 --- a/test/utests/tests/AampCurlDownloader/FunctionalTests.cpp +++ b/test/utests/tests/AampCurlDownloader/FunctionalTests.cpp @@ -590,6 +590,74 @@ TEST_F(FunctionalTests, AampCurlDownloader_Retry_SendError) EXPECT_FALSE(mAampCurlDownloader->IsDownloadActive()); } +class RetryableCurlFailureTests : public FunctionalTests, + public ::testing::WithParamInterface +{ +}; + +/** + * @brief Verify that newly retryable curl transport failures are retried. + */ +TEST_P(RetryableCurlFailureTests, + RetryableCurlFailure_RetryAndSucceed) +{ + DownloadResponsePtr respData = std::make_shared(); + DownloadConfigPtr inpData = std::make_shared(); + inpData->bNeedDownloadMetrics = true; + inpData->bIgnoreResponseHeader = true; + inpData->iDownloadRetryCount = 1; + + EXPECT_CALL(*g_mockCurl, curl_easy_init()).WillOnce(Return(mCurlEasyHandle)); + /* The curl easy handle will be cleaned when AampCurlDownloader is destroyed. */ + EXPECT_CALL(*g_mockCurl, curl_easy_cleanup(mCurlEasyHandle)); + EXPECT_CALL(*g_mockCurl, + curl_easy_setopt_ptr(mCurlEasyHandle, CURLOPT_PROGRESSDATA, + mAampCurlDownloader)).WillOnce(Return(CURLE_OK)); + EXPECT_CALL(*g_mockCurl, + curl_easy_setopt_func_xferinfo(mCurlEasyHandle, + CURLOPT_XFERINFOFUNCTION, NotNull())) + .WillOnce(DoAll(SaveArgPointee<2>(&mCurlProgressCallback), + Return(CURLE_OK))); + EXPECT_CALL(*g_mockCurl, + curl_easy_setopt_ptr(mCurlEasyHandle, CURLOPT_WRITEDATA, + mAampCurlDownloader)).WillOnce(Return(CURLE_OK)); + EXPECT_CALL(*g_mockCurl, + curl_easy_setopt_func_write(mCurlEasyHandle, + CURLOPT_WRITEFUNCTION, NotNull())) + .WillOnce(DoAll(SaveArgPointee<2>(&mCurlWriteFunc), + Return(CURLE_OK))); + mAampCurlDownloader->Initialize(inpData); + + ASSERT_NE(mCurlProgressCallback, nullptr); + ASSERT_NE(mCurlWriteFunc, nullptr); + + EXPECT_CALL(*g_mockCurl, + curl_easy_setopt_str(mCurlEasyHandle, CURLOPT_URL, mUrl.c_str())) + .WillOnce(Return(CURLE_OK)); + EXPECT_CALL(*g_mockCurl, curl_easy_perform(mCurlEasyHandle)) + .WillOnce(Return(GetParam())) + .WillOnce(Return(CURLE_OK)); + EXPECT_CALL(*g_mockCurl, + curl_easy_getinfo_int(mCurlEasyHandle, CURLINFO_RESPONSE_CODE, + NotNull())) + .WillOnce(DoAll(SetArgPointee<2>(200), Return(CURLE_OK))); + + mAampCurlDownloader->Download(mUrl, respData); + + EXPECT_EQ(200, respData->iHttpRetValue); + EXPECT_FALSE(mAampCurlDownloader->IsDownloadActive()); +} + +INSTANTIATE_TEST_SUITE_P( + RetryableTransportFailures, + RetryableCurlFailureTests, + ::testing::Values( + CURLE_PARTIAL_FILE, + CURLE_COULDNT_RESOLVE_PROXY, + CURLE_COULDNT_RESOLVE_HOST, + CURLE_RECV_ERROR, + CURLE_GOT_NOTHING)); + /** * @brief Verify that downloadCompleteMetrics.total uses cumulative wall-clock time, not CURLINFO_TOTAL_TIME. *