Winhttp flaky test

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)_)

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);

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<std::mutex> 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<std::mutex> 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<WinHttpAction*>(dwContext);
     try
     {
-      WinHttpAction* httpAction = reinterpret_cast<WinHttpAction*>(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)
     {

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)