diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index 4ee0ccff97..166dc7477c 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -8,6 +8,8 @@ ### Bugs Fixed +- [[5244]](https://github.com/Azure/azure-sdk-for-cpp/issues/5244) WinHTTP transport not closing correctly in case of a request timeout. + ### Other Changes - Move the connection back to the connection pool when HTTP error 404 was received. This may improve the performance of a multithreaded application when libcurl transport adapter is being used. (A community contribution, courtesy of _[mchelnokov](https://github.com/mchelnokov)_) diff --git a/sdk/core/azure-core/src/http/winhttp/win_http_request.hpp b/sdk/core/azure-core/src/http/winhttp/win_http_request.hpp index 5ae4b43471..2e2c34075c 100644 --- a/sdk/core/azure-core/src/http/winhttp/win_http_request.hpp +++ b/sdk/core/azure-core/src/http/winhttp/win_http_request.hpp @@ -178,7 +178,7 @@ namespace Azure { namespace Core { namespace Http { namespace _detail { WinHttpTransportOptions const& options); ~WinHttpRequest(); - + void MarkRequestHandleClosed() { m_requestHandleClosed = true; }; void Upload(Azure::Core::Http::Request& request, Azure::Core::Context const& context); void SendRequest(Azure::Core::Http::Request& request, Azure::Core::Context const& context); void ReceiveResponse(Azure::Core::Context const& context); diff --git a/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp b/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp index 2bbcb7c03b..e15fffb0c0 100644 --- a/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp +++ b/sdk/core/azure-core/src/http/winhttp/win_http_transport.cpp @@ -522,13 +522,21 @@ namespace Azure { namespace Core { namespace Http { namespace _detail { } void WinHttpAction::CompleteActionWithError(DWORD_PTR stowedErrorInformation, DWORD stowedError) { - // Note that the order of scope_exit and lock is important - this ensures that scope_exit is - // destroyed *after* lock is destroyed, ensuring that the event is not set to the signalled - // state before the lock is released. - auto scope_exit{m_actionCompleteEvent.SetEvent_scope_exit()}; - std::unique_lock lock(m_actionCompleteMutex); - m_stowedErrorInformation = stowedErrorInformation; - m_stowedError = stowedError; + if (m_expectedStatus != WINHTTP_CALLBACK_STATUS_HANDLE_CLOSING) + { + // Note that the order of scope_exit and lock is important - this ensures that scope_exit is + // destroyed *after* lock is destroyed, ensuring that the event is not set to the signalled + // state before the lock is released. + auto scope_exit{m_actionCompleteEvent.SetEvent_scope_exit()}; + std::unique_lock lock(m_actionCompleteMutex); + m_stowedErrorInformation = stowedErrorInformation; + m_stowedError = stowedError; + } + else + { + Log::Write( + Logger::Level::Verbose, "Received error while closing: " + std::to_string(stowedError)); + } } DWORD WinHttpAction::GetStowedError() @@ -564,10 +572,9 @@ namespace Azure { namespace Core { namespace Http { namespace _detail { { return; } - + WinHttpAction* httpAction = reinterpret_cast(dwContext); try { - WinHttpAction* httpAction = reinterpret_cast(dwContext); httpAction->OnHttpStatusOperation( hInternet, internetStatus, statusInformation, statusInformationLength); } @@ -578,6 +585,7 @@ namespace Azure { namespace Core { namespace Http { namespace _detail { Logger::Level::Error, "Request Failed Exception Thrown: " + std::string(rfe.what()) + rfe.Message); WinHttpCloseHandle(hInternet); + httpAction->m_httpRequest->MarkRequestHandleClosed(); } catch (std::exception const& ex) { diff --git a/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp b/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp index fe61dacd87..7bd380567c 100644 --- a/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp +++ b/sdk/core/azure-core/test/ut/transport_adapter_base_test.cpp @@ -383,14 +383,17 @@ namespace Azure { namespace Core { namespace Test { TEST_P(TransportAdapter, cancelRequest) { - Azure::Core::Url hostPath(AzureSdkHttpbinServer::Delay() + "/10"); // 10 seconds delay on server - Azure::Core::Context cancelThis = Azure::Core::Context::ApplicationContext.WithDeadline( - std::chrono::system_clock::now() + std::chrono::seconds(3)); + Azure::Core::Url hostPath(AzureSdkHttpbinServer::Delay() + "/2"); // 2 seconds delay on server + for (int i = 0; i < 10; i++) + { + Azure::Core::Context cancelThis = Azure::Core::Context::ApplicationContext.WithDeadline( + std::chrono::system_clock::now() + std::chrono::milliseconds(500)); - auto request = Azure::Core::Http::Request(Azure::Core::Http::HttpMethod::Get, hostPath); + auto request = Azure::Core::Http::Request(Azure::Core::Http::HttpMethod::Get, hostPath); - // Request will be cancelled 3 seconds after sending the request. - EXPECT_THROW(m_pipeline->Send(request, cancelThis), Azure::Core::OperationCancelledException); + // Request will be canceled 500ms after sending the request. + EXPECT_THROW(m_pipeline->Send(request, cancelThis), Azure::Core::OperationCancelledException); + } } TEST_P(TransportAdapter, cancelTransferDownload)