From fb03127d05045a9dafc64f1f84cb58406b5b9a41 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Wed, 3 Nov 2021 16:08:35 -0700 Subject: [PATCH] Use ms for connection timeout (#3001) * move the static transport adapter to private impl * use MS for connection timeout * update * fix the value --- .../inc/azure/core/http/curl_transport.hpp | 24 +++++++++++-------- sdk/core/azure-core/src/http/curl/curl.cpp | 13 ++++++---- .../azure-core/src/http/curl/static_curl.cpp | 11 ++++----- .../test/ut/curl_connection_pool_test.cpp | 3 ++- 4 files changed, 30 insertions(+), 21 deletions(-) diff --git a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp index ec526e4a2..2bfb7d172 100644 --- a/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/curl_transport.hpp @@ -14,6 +14,16 @@ namespace Azure { namespace Core { namespace Http { + namespace _detail { + /** + * @brief Default maximum time in milliseconds that you allow the connection phase to the server + * to take. + * + */ + AZ_CORE_DLLEXPORT static const std::chrono::milliseconds DefaultConnectionTimeout + = std::chrono::milliseconds(300000); + } // namespace _detail + /** * @brief The available options to set libcurl SSL options. * @@ -39,13 +49,6 @@ namespace Azure { namespace Core { namespace Http { */ struct CurlTransportOptions final { - /** - * @brief Default Maximum time in seconds that you allow the connection phase to the server to - * take. - * - */ - AZ_CORE_DLLEXPORT static const long DefaultConnectionTimeout = 300; - /** * @brief The string for the proxy is passed directly to the libcurl handle without any parsing * @@ -107,13 +110,14 @@ namespace Azure { namespace Core { namespace Http { bool NoSignal = false; /** - * @brief Contain the maximum time in seconds that you allow the connection phase to the server - * to take. + * @brief Contain the maximum time that you allow the connection phase to the server to take. * * @details This only limits the connection phase, it has no impact once it has connected. * + * @remarks The default timeout is 300 seconds and using `0` would set this default value. + * */ - long ConnectionTimeout = DefaultConnectionTimeout; + std::chrono::milliseconds ConnectionTimeout = _detail::DefaultConnectionTimeout; }; /** diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index 0f76e65a9..06ac64e55 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -1231,8 +1231,12 @@ inline std::string GetConnectionKey(std::string const& host, CurlTransportOption key.append(!options.SslOptions.EnableCertificateRevocationListCheck ? "1" : "0"); key.append(options.SslVerifyPeer ? "1" : "0"); key.append(options.NoSignal ? "1" : "0"); + // using DefaultConnectionTimeout or 0 result in the same setting key.append( - options.ConnectionTimeout == CurlTransportOptions::DefaultConnectionTimeout ? "0" : "1"); + (options.ConnectionTimeout == Azure::Core::Http::_detail::DefaultConnectionTimeout + || options.ConnectionTimeout == std::chrono::milliseconds(0)) + ? "0" + : std::to_string(options.ConnectionTimeout.count())); return key; } @@ -1338,13 +1342,14 @@ std::unique_ptr CurlConnectionPool::ExtractOrCreateCurlCo + std::string(curl_easy_strerror(result))); } - if (options.ConnectionTimeout != CurlTransportOptions::DefaultConnectionTimeout) + if (options.ConnectionTimeout != Azure::Core::Http::_detail::DefaultConnectionTimeout) { - if (!SetLibcurlOption(newHandle, CURLOPT_CONNECTTIMEOUT, options.ConnectionTimeout, &result)) + if (!SetLibcurlOption(newHandle, CURLOPT_CONNECTTIMEOUT_MS, options.ConnectionTimeout, &result)) { throw Azure::Core::Http::TransportException( _detail::DefaultFailedToGetNewConnectionTemplate + host - + ". Fail setting connect timeout to: " + std::to_string(options.ConnectionTimeout) + ". " + + ". Fail setting connect timeout to: " + + std::to_string(options.ConnectionTimeout.count()) + " ms. " + std::string(curl_easy_strerror(result))); } } diff --git a/sdk/core/azure-core/src/http/curl/static_curl.cpp b/sdk/core/azure-core/src/http/curl/static_curl.cpp index 069527156..a2dbb0dd6 100644 --- a/sdk/core/azure-core/src/http/curl/static_curl.cpp +++ b/sdk/core/azure-core/src/http/curl/static_curl.cpp @@ -301,16 +301,15 @@ public: } } - if (m_options.ConnectionTimeout - != Azure::Core::Http::CurlTransportOptions::DefaultConnectionTimeout) + if (m_options.ConnectionTimeout != Azure::Core::Http::_detail::DefaultConnectionTimeout) { if (!SetStaticLibcurlOption( - m_libcurlHandle, CURLOPT_CONNECTTIMEOUT, m_options.ConnectionTimeout, &result)) + m_libcurlHandle, CURLOPT_CONNECTTIMEOUT_MS, m_options.ConnectionTimeout, &result)) { throw Azure::Core::Http::TransportException( - FailedToGetNewConnectionTemplate + host - + ". Fail setting connect timeout to: " + std::to_string(m_options.ConnectionTimeout) - + ". " + std::string(curl_easy_strerror(result))); + FailedToGetNewConnectionTemplate + host + ". Fail setting connect timeout to: " + + std::to_string(m_options.ConnectionTimeout.count()) + ". " + + std::string(curl_easy_strerror(result))); } } diff --git a/sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp b/sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp index 444d9be5a..40c93ad1f 100644 --- a/sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp +++ b/sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp @@ -114,11 +114,12 @@ namespace Azure { namespace Core { namespace Test { // Now test that using a different connection config won't re-use the same connection std::string const secondExpectedKey - = AzureSdkHttpbinServer::Schema() + AzureSdkHttpbinServer::Host() + "001000"; + = AzureSdkHttpbinServer::Schema() + AzureSdkHttpbinServer::Host() + "00100200000"; { // Creating a new connection with options Azure::Core::Http::CurlTransportOptions options; options.SslVerifyPeer = false; + options.ConnectionTimeout = std::chrono::seconds(200); auto connection = CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options); EXPECT_EQ(connection->GetConnectionKey(), secondExpectedKey);