From d692cde146aade3643a25ed404660f61c8a4a4a2 Mon Sep 17 00:00:00 2001 From: Pip Potter <61438964+lmbr-pip@users.noreply.github.com> Date: Mon, 18 Oct 2021 13:12:20 -0700 Subject: [PATCH] LYN-7473: Ensure Requestor background thread is shutdown correctly (#4744) Signed-off-by: rppotter --- .../Code/Source/HttpRequestManager.cpp | 44 ++++++++++++------- .../Source/HttpRequestorSystemComponent.cpp | 15 +++---- .../Code/Tests/HttpRequestorTest.cpp | 21 ++++----- 3 files changed, 43 insertions(+), 37 deletions(-) diff --git a/Gems/HttpRequestor/Code/Source/HttpRequestManager.cpp b/Gems/HttpRequestor/Code/Source/HttpRequestManager.cpp index 06cf018f76..d1bf3bab4d 100644 --- a/Gems/HttpRequestor/Code/Source/HttpRequestManager.cpp +++ b/Gems/HttpRequestor/Code/Source/HttpRequestManager.cpp @@ -36,22 +36,27 @@ namespace HttpRequestor desc.m_cpuId = AFFINITY_MASK_USERTHREADS; m_runThread = true; AWSNativeSDKInit::InitializationManager::InitAwsApi(); - auto function = AZStd::bind(&Manager::ThreadFunction, this); + auto function = [this] + { + ThreadFunction(); + }; m_thread = AZStd::thread(desc, function); } Manager::~Manager() { - AWSNativeSDKInit::InitializationManager::Shutdown(); m_runThread = false; m_requestConditionVar.notify_all(); if (m_thread.joinable()) { m_thread.join(); } + + // Shutdown after background thread has closed. + AWSNativeSDKInit::InitializationManager::Shutdown(); } - void Manager::AddRequest(Parameters && httpRequestParameters) + void Manager::AddRequest(Parameters&& httpRequestParameters) { { AZStd::lock_guard lock(m_requestMutex); @@ -60,7 +65,7 @@ namespace HttpRequestor m_requestConditionVar.notify_all(); } - void Manager::AddTextRequest(TextParameters && httpTextRequestParameters) + void Manager::AddTextRequest(TextParameters&& httpTextRequestParameters) { { AZStd::lock_guard lock(m_requestMutex); @@ -80,9 +85,14 @@ namespace HttpRequestor void Manager::HandleRequestBatch() { - // Lock mutex and wait for work to be signalled via the condition variable + // Lock mutex and wait for work to be signaled via the condition variable AZStd::unique_lock lock(m_requestMutex); - m_requestConditionVar.wait(lock, [&] { return !m_runThread || !m_requestsToHandle.empty() || !m_textRequestsToHandle.empty(); }); + m_requestConditionVar.wait( + lock, + [&] + { + return !m_runThread || !m_requestsToHandle.empty() || !m_textRequestsToHandle.empty(); + }); // Swap queues AZStd::queue requestsToHandle; @@ -114,21 +124,22 @@ namespace HttpRequestor config.enableTcpKeepAlive = AZ_TRAIT_AZFRAMEWORK_AWS_ENABLE_TCP_KEEP_ALIVE_SUPPORTED; std::shared_ptr httpClient = Aws::Http::CreateHttpClient(config); - auto httpRequest = Aws::Http::CreateHttpRequest(httpRequestParameters.GetURI(), httpRequestParameters.GetMethod(), Aws::Utils::Stream::DefaultResponseStreamFactoryMethod); + auto httpRequest = Aws::Http::CreateHttpRequest( + httpRequestParameters.GetURI(), httpRequestParameters.GetMethod(), Aws::Utils::Stream::DefaultResponseStreamFactoryMethod); AZ_Assert(httpRequest, "HttpRequest not created!"); - for (const auto & it : httpRequestParameters.GetHeaders()) + for (const auto& it : httpRequestParameters.GetHeaders()) { httpRequest->SetHeaderValue(it.first.c_str(), it.second.c_str()); } - if( httpRequestParameters.GetBodyStream() != nullptr) + if (httpRequestParameters.GetBodyStream() != nullptr) { httpRequest->AddContentBody(httpRequestParameters.GetBodyStream()); httpRequest->SetContentLength(AZStd::to_string(httpRequestParameters.GetBodyStream()->str().length()).c_str()); } - + auto httpResponse = httpClient->MakeRequest(httpRequest); if (!httpResponse) @@ -154,15 +165,16 @@ namespace HttpRequestor } } - void Manager::HandleTextRequest(const TextParameters & httpRequestParameters) + void Manager::HandleTextRequest(const TextParameters& httpRequestParameters) { Aws::Client::ClientConfiguration config; config.enableTcpKeepAlive = AZ_TRAIT_AZFRAMEWORK_AWS_ENABLE_TCP_KEEP_ALIVE_SUPPORTED; std::shared_ptr httpClient = Aws::Http::CreateHttpClient(config); - auto httpRequest = Aws::Http::CreateHttpRequest(httpRequestParameters.GetURI(), httpRequestParameters.GetMethod(), Aws::Utils::Stream::DefaultResponseStreamFactoryMethod); - - for (const auto & it : httpRequestParameters.GetHeaders()) + auto httpRequest = Aws::Http::CreateHttpRequest( + httpRequestParameters.GetURI(), httpRequestParameters.GetMethod(), Aws::Utils::Stream::DefaultResponseStreamFactoryMethod); + + for (const auto& it : httpRequestParameters.GetHeaders()) { httpRequest->SetHeaderValue(it.first.c_str(), it.second.c_str()); } @@ -172,7 +184,7 @@ namespace HttpRequestor httpRequest->AddContentBody(httpRequestParameters.GetBodyStream()); } - auto httpResponse = httpClient->MakeRequest(httpRequest); + const auto httpResponse = httpClient->MakeRequest(httpRequest); if (!httpResponse) { @@ -192,4 +204,4 @@ namespace HttpRequestor AZStd::string data(std::istreambuf_iterator(httpResponse->GetResponseBody()), eos); httpRequestParameters.GetCallback()(AZStd::move(data), httpResponse->GetResponseCode()); } -} +} // namespace HttpRequestor diff --git a/Gems/HttpRequestor/Code/Source/HttpRequestorSystemComponent.cpp b/Gems/HttpRequestor/Code/Source/HttpRequestorSystemComponent.cpp index 9df015ce36..89cf2d19e2 100644 --- a/Gems/HttpRequestor/Code/Source/HttpRequestorSystemComponent.cpp +++ b/Gems/HttpRequestor/Code/Source/HttpRequestorSystemComponent.cpp @@ -63,20 +63,17 @@ namespace HttpRequestor void HttpRequestorSystemComponent::Reflect(AZ::ReflectContext* context) { - if (AZ::SerializeContext* serialize = azrtti_cast(context)) + if (auto serialize = azrtti_cast(context)) { - serialize->Class() - ->Version(1); - ; - + serialize->Class()->Version(1); + if (AZ::EditContext* ec = serialize->GetEditContext()) { ec->Class("HttpRequestor", "Will make HTTP Rest calls") ->ClassElement(AZ::Edit::ClassElements::EditorData, "") - // ->Attribute(AZ::Edit::Attributes::Category, "") Set a category - ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("System")) - ->Attribute(AZ::Edit::Attributes::AutoExpand, true) - ; + // ->Attribute(AZ::Edit::Attributes::Category, "") Set a category + ->Attribute(AZ::Edit::Attributes::AppearsInAddComponentMenu, AZ_CRC("System")) + ->Attribute(AZ::Edit::Attributes::AutoExpand, true); } } } diff --git a/Gems/HttpRequestor/Code/Tests/HttpRequestorTest.cpp b/Gems/HttpRequestor/Code/Tests/HttpRequestorTest.cpp index 48826fcf10..b6d1822537 100644 --- a/Gems/HttpRequestor/Code/Tests/HttpRequestorTest.cpp +++ b/Gems/HttpRequestor/Code/Tests/HttpRequestorTest.cpp @@ -19,7 +19,7 @@ class HttpTest { }; -TEST_F(HttpTest, DISABLED_HttpRequesterTest) +TEST_F(HttpTest, HttpRequesterTest) { HttpRequestor::Manager httpRequestManager; @@ -35,17 +35,14 @@ TEST_F(HttpTest, DISABLED_HttpRequesterTest) requestConditionVar.wait_for(lock, AZStd::chrono::milliseconds(10)); } - httpRequestManager.AddTextRequest( - HttpRequestor::TextParameters("https://httpbin.org/ip", - Aws::Http::HttpMethod::HTTP_GET, - [&resultData, &resultCode, &requestConditionVar](const AZStd::string& data, Aws::Http::HttpResponseCode code) - { - resultData = data; - resultCode = code; - requestConditionVar.notify_all(); - } - ) - ); + httpRequestManager.AddTextRequest(HttpRequestor::TextParameters( + "https://httpbin.org/ip", Aws::Http::HttpMethod::HTTP_GET, + [&resultData, &resultCode, &requestConditionVar](const AZStd::string& data, Aws::Http::HttpResponseCode code) + { + resultData = data; + resultCode = code; + requestConditionVar.notify_all(); + })); { AZStd::unique_lock lock(requestMutex);