From 166d95d97462c269c66ad7e4b7fa8bb75a78a89c Mon Sep 17 00:00:00 2001 From: "Julio C. Rocha" Date: Tue, 12 Jul 2022 19:12:19 -0700 Subject: [PATCH 1/6] Implement hard-coded timeout --- vnext/Shared/Networking/WinRTHttpResource.cpp | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/vnext/Shared/Networking/WinRTHttpResource.cpp b/vnext/Shared/Networking/WinRTHttpResource.cpp index a496186d769..7472fad01b7 100644 --- a/vnext/Shared/Networking/WinRTHttpResource.cpp +++ b/vnext/Shared/Networking/WinRTHttpResource.cpp @@ -297,7 +297,30 @@ fire_and_forget WinRTHttpResource::PerformSendRequest(HttpRequestMessage &&reque auto sendRequestOp = self->m_client.SendRequestAsync(coRequest); self->TrackResponse(coReqArgs->RequestId, sendRequestOp); - co_await lessthrow_await_adapter{sendRequestOp}; + // See https://devblogs.microsoft.com/oldnewthing/20220415-00/?p=106486 + using namespace std::chrono_literals; + using winrt::Windows::Foundation::IAsyncAction; + auto timedOut = std::make_shared(false); + //TODO: Look into using AsyncStatus? + // https://docs.microsoft.com/en-us/uwp/api/windows.foundation.iasyncoperationwithprogress-2?view=winrt-22621 + auto sendRequestTimeout = [](auto timedOut) -> ResponseOperation { + co_await winrt::resume_after(4s); + *timedOut = true; + co_return nullptr; + }(timedOut); + + auto sendRequestAny = winrt::when_any(sendRequestOp, sendRequestTimeout); + co_await lessthrow_await_adapter{sendRequestAny}; + + if (*timedOut) { + if (self->m_onError) { + //winrt::hresult_error(HRESULT_FROM_WIN32(ERROR_TIMEOUT)) + self->m_onError(coReqArgs->RequestId, Utilities::HResultToString(HRESULT_FROM_WIN32(ERROR_TIMEOUT))); + } + co_return self->UntrackResponse(coReqArgs->RequestId); + } + + //co_await lessthrow_await_adapter{sendRequestOp}; auto result = sendRequestOp.ErrorCode(); if (result < 0) { if (self->m_onError) { From 37cc8a4e8e5871203849cdf8013de02839d93ffa Mon Sep 17 00:00:00 2001 From: "Julio C. Rocha" Date: Wed, 13 Jul 2022 14:39:09 -0700 Subject: [PATCH 2/6] Create timeout from JS args --- vnext/Shared/Networking/WinRTHttpResource.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/vnext/Shared/Networking/WinRTHttpResource.cpp b/vnext/Shared/Networking/WinRTHttpResource.cpp index 7472fad01b7..359ecbc1100 100644 --- a/vnext/Shared/Networking/WinRTHttpResource.cpp +++ b/vnext/Shared/Networking/WinRTHttpResource.cpp @@ -303,23 +303,28 @@ fire_and_forget WinRTHttpResource::PerformSendRequest(HttpRequestMessage &&reque auto timedOut = std::make_shared(false); //TODO: Look into using AsyncStatus? // https://docs.microsoft.com/en-us/uwp/api/windows.foundation.iasyncoperationwithprogress-2?view=winrt-22621 - auto sendRequestTimeout = [](auto timedOut) -> ResponseOperation { - co_await winrt::resume_after(4s); + auto sendRequestTimeout = [](auto timedOut, auto milliseconds) -> ResponseOperation { + // Convert milliseconds to "ticks" (10^-7 seconds) + co_await winrt::resume_after(winrt::Windows::Foundation::TimeSpan{milliseconds * 10000}); *timedOut = true; co_return nullptr; - }(timedOut); + }(timedOut, coReqArgs->Timeout); auto sendRequestAny = winrt::when_any(sendRequestOp, sendRequestTimeout); co_await lessthrow_await_adapter{sendRequestAny}; + // Cancel either still unfinished coroutine. + sendRequestTimeout.Cancel(); + sendRequestOp.Cancel(); + if (*timedOut) { if (self->m_onError) { - //winrt::hresult_error(HRESULT_FROM_WIN32(ERROR_TIMEOUT)) self->m_onError(coReqArgs->RequestId, Utilities::HResultToString(HRESULT_FROM_WIN32(ERROR_TIMEOUT))); } co_return self->UntrackResponse(coReqArgs->RequestId); } + //TODO: REMOVE! //co_await lessthrow_await_adapter{sendRequestOp}; auto result = sendRequestOp.ErrorCode(); if (result < 0) { From ef8fc80739b419ca003a9f3c47401357e3aece59 Mon Sep 17 00:00:00 2001 From: "Julio C. Rocha" Date: Wed, 13 Jul 2022 17:02:20 -0700 Subject: [PATCH 3/6] Timeout only for values greater than 0 --- .../HttpOriginPolicyIntegrationTest.cpp | 6 +- .../HttpResourceIntegrationTests.cpp | 108 +++++++++++++++++- vnext/Shared/Networking/IHttpResource.h | 1 + vnext/Shared/Networking/WinRTHttpResource.cpp | 50 ++++---- 4 files changed, 131 insertions(+), 34 deletions(-) diff --git a/vnext/Desktop.IntegrationTests/HttpOriginPolicyIntegrationTest.cpp b/vnext/Desktop.IntegrationTests/HttpOriginPolicyIntegrationTest.cpp index 640383f71e6..40c4fde98da 100644 --- a/vnext/Desktop.IntegrationTests/HttpOriginPolicyIntegrationTest.cpp +++ b/vnext/Desktop.IntegrationTests/HttpOriginPolicyIntegrationTest.cpp @@ -147,7 +147,7 @@ TEST_CLASS(HttpOriginPolicyIntegrationTest) {}, /*data*/ "text", false, /*useIncrementalUpdates*/ - 1000, /*timeout*/ + 0, /*timeout*/ clientArgs.WithCredentials, /*withCredentials*/ [](int64_t){} /*reactCallback*/ ); @@ -201,7 +201,7 @@ TEST_CLASS(HttpOriginPolicyIntegrationTest) {}, /*data*/ "text", false, /*useIncrementalUpdates*/ - 1000, /*timeout*/ + 0, /*timeout*/ clientArgs.WithCredentials, /*withCredentials*/ [](int64_t) {} /*reactCallback*/ ); @@ -300,7 +300,7 @@ TEST_CLASS(HttpOriginPolicyIntegrationTest) //{} /*bodyData*/, "text", false /*useIncrementalUpdates*/, - 1000 /*timeout*/, + 0 /*timeout*/, false /*withCredentials*/, [](int64_t) {} /*callback*/ ); diff --git a/vnext/Desktop.IntegrationTests/HttpResourceIntegrationTests.cpp b/vnext/Desktop.IntegrationTests/HttpResourceIntegrationTests.cpp index 2d2e7dac901..f6b1679ce0b 100644 --- a/vnext/Desktop.IntegrationTests/HttpResourceIntegrationTests.cpp +++ b/vnext/Desktop.IntegrationTests/HttpResourceIntegrationTests.cpp @@ -78,7 +78,7 @@ TEST_CLASS (HttpResourceIntegrationTest) { {}, /*data*/ "text", false, - 1000 /*timeout*/, + 0 /*timeout*/, false /*withCredentials*/, [](int64_t) {}); @@ -139,7 +139,7 @@ TEST_CLASS (HttpResourceIntegrationTest) { {}, /*data*/ "text", false, - 1000 /*timeout*/, + 0 /*timeout*/, false /*withCredentials*/, [](int64_t) {}); //clang-format on @@ -172,7 +172,7 @@ TEST_CLASS (HttpResourceIntegrationTest) { promise.set_value(); }); - resource->SendRequest("GET", "http://nonexistinghost", 0, {}, {}, "text", false, 1000, false, [](int64_t) {}); + resource->SendRequest("GET", "http://nonexistinghost", 0, {}, {}, "text", false, 0, false, [](int64_t) {}); promise.get_future().wait(); @@ -256,7 +256,7 @@ TEST_CLASS (HttpResourceIntegrationTest) { {}, /*data*/ "text", false, - 1000 /*timeout*/, + 0 /*timeout*/, false /*withCredentials*/, [](int64_t) {}); //clang-format on @@ -339,7 +339,7 @@ TEST_CLASS (HttpResourceIntegrationTest) { {}, /*data*/ "text", false, /*useIncrementalUpdates*/ - 1000 /*timeout*/, + 0 /*timeout*/, false /*withCredentials*/, [](int64_t) {}); //clang-format on @@ -354,6 +354,104 @@ TEST_CLASS (HttpResourceIntegrationTest) { Assert::AreEqual(static_cast(200), getResponse.StatusCode); Assert::AreEqual({"Redirect Content"}, content); } + + TEST_METHOD(TimeoutSucceeds) { + auto port = s_port; + string url = "http://localhost:" + std::to_string(port); + + promise getPromise; + string error; + int statusCode = 0; + + auto server = std::make_shared(s_port); + server->Callbacks().OnGet = [&getPromise](const DynamicRequest &request) -> ResponseWrapper { + DynamicResponse response; + response.result(http::status::ok); + + // Hold response to test client timeout + promise timer; + timer.get_future().wait_for(std::chrono::milliseconds(2000)); + + return {std::move(response)}; + }; + server->Start(); + + auto resource = IHttpResource::Make(); + resource->SetOnResponse([&getPromise, &statusCode](int64_t, IHttpResource::Response response) { + statusCode = static_cast(response.StatusCode); + getPromise.set_value(); + }); + resource->SetOnError([&getPromise, &error, &server](int64_t, string &&errorMessage) { + error = std::move(errorMessage); + getPromise.set_value(); + }); + resource->SendRequest( + "GET", + std::move(url), + 0, /*requestId*/ + {}, /*headers*/ + {}, /*data*/ + "text", /*responseType*/ + false, /*useIncrementalUpdates*/ + 6000, /*timeout*/ + false, /*withCredentials*/ + [](int64_t) {} /*callback*/); + + getPromise.get_future().wait(); + server->Stop(); + + Assert::AreEqual({}, error); + Assert::AreEqual(200, statusCode); + } + + TEST_METHOD(TimeoutFails) { + auto port = s_port; + string url = "http://localhost:" + std::to_string(port); + + promise getPromise; + string error; + int statusCode = 0; + + auto server = std::make_shared(s_port); + server->Callbacks().OnGet = [&getPromise](const DynamicRequest &request) -> ResponseWrapper { + DynamicResponse response; + response.result(http::status::ok); + + // Hold response to test client timeout + promise timer; + timer.get_future().wait_for(std::chrono::milliseconds(4000)); + + return {std::move(response)}; + }; + server->Start(); + + auto resource = IHttpResource::Make(); + resource->SetOnResponse([&getPromise, &statusCode](int64_t, IHttpResource::Response response) { + statusCode = static_cast(response.StatusCode); + getPromise.set_value(); + }); + resource->SetOnError([&getPromise, &error, &server](int64_t, string &&errorMessage) { + error = std::move(errorMessage); + getPromise.set_value(); + }); + resource->SendRequest( + "GET", + std::move(url), + 0, /*requestId*/ + {}, /*headers*/ + {}, /*data*/ + "text", /*responseType*/ + false, /*useIncrementalUpdates*/ + 2000, /*timeout*/ + false, /*withCredentials*/ + [](int64_t) {} /*callback*/); + + getPromise.get_future().wait(); + server->Stop(); + + Assert::AreEqual({"[0x800705b4] This operation returned because the timeout period expired."}, error); + Assert::AreEqual(0, statusCode); + } }; /*static*/ uint16_t HttpResourceIntegrationTest::s_port = 4444; diff --git a/vnext/Shared/Networking/IHttpResource.h b/vnext/Shared/Networking/IHttpResource.h index 7a8434e9f6f..173c79cda6e 100644 --- a/vnext/Shared/Networking/IHttpResource.h +++ b/vnext/Shared/Networking/IHttpResource.h @@ -71,6 +71,7 @@ struct IHttpResource { /// /// /// Request timeout in miliseconds. + /// Note: A value of 0 means no timeout. The resource will await the response indefinitely. /// /// /// Allow including credentials in request. diff --git a/vnext/Shared/Networking/WinRTHttpResource.cpp b/vnext/Shared/Networking/WinRTHttpResource.cpp index 359ecbc1100..093a4c848b9 100644 --- a/vnext/Shared/Networking/WinRTHttpResource.cpp +++ b/vnext/Shared/Networking/WinRTHttpResource.cpp @@ -297,35 +297,33 @@ fire_and_forget WinRTHttpResource::PerformSendRequest(HttpRequestMessage &&reque auto sendRequestOp = self->m_client.SendRequestAsync(coRequest); self->TrackResponse(coReqArgs->RequestId, sendRequestOp); - // See https://devblogs.microsoft.com/oldnewthing/20220415-00/?p=106486 - using namespace std::chrono_literals; - using winrt::Windows::Foundation::IAsyncAction; - auto timedOut = std::make_shared(false); - //TODO: Look into using AsyncStatus? - // https://docs.microsoft.com/en-us/uwp/api/windows.foundation.iasyncoperationwithprogress-2?view=winrt-22621 - auto sendRequestTimeout = [](auto timedOut, auto milliseconds) -> ResponseOperation { - // Convert milliseconds to "ticks" (10^-7 seconds) - co_await winrt::resume_after(winrt::Windows::Foundation::TimeSpan{milliseconds * 10000}); - *timedOut = true; - co_return nullptr; - }(timedOut, coReqArgs->Timeout); - - auto sendRequestAny = winrt::when_any(sendRequestOp, sendRequestTimeout); - co_await lessthrow_await_adapter{sendRequestAny}; - - // Cancel either still unfinished coroutine. - sendRequestTimeout.Cancel(); - sendRequestOp.Cancel(); - - if (*timedOut) { - if (self->m_onError) { - self->m_onError(coReqArgs->RequestId, Utilities::HResultToString(HRESULT_FROM_WIN32(ERROR_TIMEOUT))); + if (coReqArgs->Timeout > 0) { + // See https://devblogs.microsoft.com/oldnewthing/20220415-00/?p=106486 + auto timedOut = std::make_shared(false); + auto sendRequestTimeout = [](auto timedOut, auto milliseconds) -> ResponseOperation { + // Convert milliseconds to "ticks" (10^-7 seconds) + co_await winrt::resume_after(winrt::Windows::Foundation::TimeSpan{milliseconds * 10000}); + *timedOut = true; + co_return nullptr; + }(timedOut, coReqArgs->Timeout); + + auto sendRequestAny = winrt::when_any(sendRequestOp, sendRequestTimeout); + co_await lessthrow_await_adapter{sendRequestAny}; + + // Cancel either still unfinished coroutine. + sendRequestTimeout.Cancel(); + sendRequestOp.Cancel(); + + if (*timedOut) { + if (self->m_onError) { + self->m_onError(coReqArgs->RequestId, Utilities::HResultToString(HRESULT_FROM_WIN32(ERROR_TIMEOUT))); + } + co_return self->UntrackResponse(coReqArgs->RequestId); } - co_return self->UntrackResponse(coReqArgs->RequestId); + } else { + co_await lessthrow_await_adapter{sendRequestOp}; } - //TODO: REMOVE! - //co_await lessthrow_await_adapter{sendRequestOp}; auto result = sendRequestOp.ErrorCode(); if (result < 0) { if (self->m_onError) { From 1133bab4626775be82209b208f2df30e034a7be5 Mon Sep 17 00:00:00 2001 From: "Julio C. Rocha" Date: Wed, 13 Jul 2022 17:03:04 -0700 Subject: [PATCH 4/6] Change files --- ...ative-windows-42dec614-af90-4b29-b8c7-f266415ea240.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/react-native-windows-42dec614-af90-4b29-b8c7-f266415ea240.json diff --git a/change/react-native-windows-42dec614-af90-4b29-b8c7-f266415ea240.json b/change/react-native-windows-42dec614-af90-4b29-b8c7-f266415ea240.json new file mode 100644 index 00000000000..5937254e3f5 --- /dev/null +++ b/change/react-native-windows-42dec614-af90-4b29-b8c7-f266415ea240.json @@ -0,0 +1,7 @@ +{ + "type": "prerelease", + "comment": "Implement HTTP client timeout", + "packageName": "react-native-windows", + "email": "julio.rocha@microsoft.com", + "dependentChangeType": "patch" +} From 803042a9531c5edd255ae65118e0c25693543d51 Mon Sep 17 00:00:00 2001 From: "Julio C. Rocha" Date: Wed, 13 Jul 2022 17:18:41 -0700 Subject: [PATCH 5/6] Remove variable sendRequestAny --- vnext/Shared/Networking/WinRTHttpResource.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/vnext/Shared/Networking/WinRTHttpResource.cpp b/vnext/Shared/Networking/WinRTHttpResource.cpp index 093a4c848b9..49b1d0e3875 100644 --- a/vnext/Shared/Networking/WinRTHttpResource.cpp +++ b/vnext/Shared/Networking/WinRTHttpResource.cpp @@ -307,8 +307,7 @@ fire_and_forget WinRTHttpResource::PerformSendRequest(HttpRequestMessage &&reque co_return nullptr; }(timedOut, coReqArgs->Timeout); - auto sendRequestAny = winrt::when_any(sendRequestOp, sendRequestTimeout); - co_await lessthrow_await_adapter{sendRequestAny}; + co_await lessthrow_await_adapter{winrt::when_any(sendRequestOp, sendRequestTimeout)}; // Cancel either still unfinished coroutine. sendRequestTimeout.Cancel(); From c87c2c5cfd8af43129587eca50dc4a0075e8154a Mon Sep 17 00:00:00 2001 From: "Julio C. Rocha" Date: Fri, 15 Jul 2022 15:54:30 -0700 Subject: [PATCH 6/6] Remove unused captures --- .../HttpResourceIntegrationTests.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/vnext/Desktop.IntegrationTests/HttpResourceIntegrationTests.cpp b/vnext/Desktop.IntegrationTests/HttpResourceIntegrationTests.cpp index f6b1679ce0b..8cafbade184 100644 --- a/vnext/Desktop.IntegrationTests/HttpResourceIntegrationTests.cpp +++ b/vnext/Desktop.IntegrationTests/HttpResourceIntegrationTests.cpp @@ -364,7 +364,7 @@ TEST_CLASS (HttpResourceIntegrationTest) { int statusCode = 0; auto server = std::make_shared(s_port); - server->Callbacks().OnGet = [&getPromise](const DynamicRequest &request) -> ResponseWrapper { + server->Callbacks().OnGet = [](const DynamicRequest &) -> ResponseWrapper { DynamicResponse response; response.result(http::status::ok); @@ -381,7 +381,7 @@ TEST_CLASS (HttpResourceIntegrationTest) { statusCode = static_cast(response.StatusCode); getPromise.set_value(); }); - resource->SetOnError([&getPromise, &error, &server](int64_t, string &&errorMessage) { + resource->SetOnError([&getPromise, &error](int64_t, string &&errorMessage) { error = std::move(errorMessage); getPromise.set_value(); }); @@ -413,7 +413,7 @@ TEST_CLASS (HttpResourceIntegrationTest) { int statusCode = 0; auto server = std::make_shared(s_port); - server->Callbacks().OnGet = [&getPromise](const DynamicRequest &request) -> ResponseWrapper { + server->Callbacks().OnGet = [](const DynamicRequest &) -> ResponseWrapper { DynamicResponse response; response.result(http::status::ok); @@ -430,7 +430,7 @@ TEST_CLASS (HttpResourceIntegrationTest) { statusCode = static_cast(response.StatusCode); getPromise.set_value(); }); - resource->SetOnError([&getPromise, &error, &server](int64_t, string &&errorMessage) { + resource->SetOnError([&getPromise, &error](int64_t, string &&errorMessage) { error = std::move(errorMessage); getPromise.set_value(); });