diff --git a/internal/network/BUILD b/internal/network/BUILD index 1d1f6d96..05eb3160 100644 --- a/internal/network/BUILD +++ b/internal/network/BUILD @@ -70,6 +70,7 @@ cc_library( "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", + "@com_google_absl//absl/time", ], ) @@ -93,11 +94,13 @@ cc_library( ":types", "//internal/platform:types", "//internal/platform/implementation:comm", + "//internal/platform/implementation:platform", "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/functional:any_invocable", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", + "@com_google_absl//absl/time", ], ) diff --git a/internal/network/http_client.h b/internal/network/http_client.h index 4e2bbedc..82334299 100644 --- a/internal/network/http_client.h +++ b/internal/network/http_client.h @@ -21,6 +21,7 @@ #include "absl/functional/any_invocable.h" #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "absl/time/time.h" #include "internal/network/http_request.h" #include "internal/network/http_response.h" #include "internal/platform/mutex.h" @@ -60,21 +61,21 @@ class HttpClient { virtual ~HttpClient() = default; - // Starts HTTP request in asynchronization mode. + // Starts HTTP request in asynchronous mode. virtual void StartRequest( - const HttpRequest& request, + const HttpRequest& request, absl::Duration timeout, absl::AnyInvocable&)> callback) = 0; - // Starts cancellable request in asynchronization mode. + // Starts cancellable request in asynchronous mode. virtual void StartCancellableRequest( - std::unique_ptr request, + std::unique_ptr request, absl::Duration timeout, absl::AnyInvocable&)> callback) = 0; - // Gets HTTP response in synchronization mode. + // Gets HTTP response in synchronous mode. virtual absl::StatusOr GetResponse( - const HttpRequest& request) = 0; + const HttpRequest& request, absl::Duration timeout) = 0; // The error may be corrected if retried at a later time. static bool IsRetryableHttpError(absl::Status status) { diff --git a/internal/network/http_client_impl.cc b/internal/network/http_client_impl.cc index 75878579..745eedfb 100644 --- a/internal/network/http_client_impl.cc +++ b/internal/network/http_client_impl.cc @@ -23,11 +23,13 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" +#include "absl/time/time.h" #include "internal/network/debug.h" #include "internal/network/http_request.h" #include "internal/network/http_response.h" #include "internal/network/http_status_code.h" #include "internal/platform/implementation/http_loader.h" +#include "internal/platform/implementation/platform.h" #include "internal/platform/logging.h" #include "internal/platform/mutex_lock.h" #include "internal/platform/single_thread_executor.h" @@ -36,20 +38,21 @@ namespace nearby { namespace network { void NearbyHttpClient::StartRequest( - const HttpRequest& request, + const HttpRequest& request, absl::Duration timeout, absl::AnyInvocable&)> callback) { MutexLock lock(&mutex_); executor_.Execute([request = std::move(request), - callback = std::move(callback)]() mutable { + callback = std::move(callback), timeout]() mutable { LOG(INFO) << __func__ << ": Start async request to url=" << request.GetUrl().GetUrlPath(); - absl::StatusOr response = InternalGetResponse(request); + absl::StatusOr response = + InternalGetResponse(request, timeout); if (response.ok()) { LOG(INFO) << __func__ << ": Got response from url=" << request.GetUrl().GetUrlPath(); } else { LOG(ERROR) << __func__ << ": Failed to get response from url=" - << request.GetUrl().GetUrlPath() << ", status" + << request.GetUrl().GetUrlPath() << ", status " << response.status(); } @@ -63,6 +66,7 @@ void NearbyHttpClient::StartRequest( void NearbyHttpClient::StartCancellableRequest( std::unique_ptr cancellable_request, + absl::Duration timeout, absl::AnyInvocable&)> callback) { MutexLock lock(&mutex_); if (cancellable_request == nullptr) { @@ -71,7 +75,7 @@ void NearbyHttpClient::StartCancellableRequest( return; } executor_.Execute([cancellable_request = std::move(cancellable_request), - callback = std::move(callback)]() mutable { + callback = std::move(callback), timeout]() mutable { LOG(INFO) << __func__ << ": Start async request to url=" << cancellable_request->http_request().GetUrl().GetUrlPath(); if (cancellable_request->is_cancelled()) { @@ -81,7 +85,7 @@ void NearbyHttpClient::StartCancellableRequest( return; } absl::StatusOr response = - InternalGetResponse(cancellable_request->http_request()); + InternalGetResponse(cancellable_request->http_request(), timeout); if (response.ok()) { LOG(INFO) << __func__ << ": Got response from url=" << cancellable_request->http_request().GetUrl().GetUrlPath(); @@ -107,11 +111,11 @@ void NearbyHttpClient::StartCancellableRequest( } absl::StatusOr NearbyHttpClient::GetResponse( - const HttpRequest& request) { + const HttpRequest& request, absl::Duration timeout) { LOG(INFO) << __func__ << ": Start request to url=" << request.GetUrl().GetUrlPath(); - absl::StatusOr response = InternalGetResponse(request); + absl::StatusOr response = InternalGetResponse(request, timeout); if (response.ok()) { LOG(INFO) << __func__ << ": Got response from url=" << request.GetUrl().GetUrlPath(); @@ -125,7 +129,7 @@ absl::StatusOr NearbyHttpClient::GetResponse( } absl::StatusOr NearbyHttpClient::InternalGetResponse( - const HttpRequest& request) { + const HttpRequest& request, absl::Duration timeout) { api::WebRequest web_request; web_request.url = request.GetUrl().GetUrlPath(); web_request.method = absl::StrCat(request.GetMethodString()); @@ -135,6 +139,7 @@ absl::StatusOr NearbyHttpClient::InternalGetResponse( } } web_request.body = absl::StrCat(request.GetBody().GetRawData()); + web_request.timeout = timeout; if (debug::kRequestEnabled) { std::stringstream request_stream; diff --git a/internal/network/http_client_impl.h b/internal/network/http_client_impl.h index f10f245b..4d6d493a 100644 --- a/internal/network/http_client_impl.h +++ b/internal/network/http_client_impl.h @@ -15,12 +15,15 @@ #ifndef THIRD_PARTY_NEARBY_INTERNAL_NETWORK_HTTP_CLIENT_IMPL_H_ #define THIRD_PARTY_NEARBY_INTERNAL_NETWORK_HTTP_CLIENT_IMPL_H_ -#include #include #include "absl/base/thread_annotations.h" +#include "absl/functional/any_invocable.h" +#include "absl/status/statusor.h" +#include "absl/time/time.h" #include "internal/network/http_client.h" #include "internal/network/http_request.h" +#include "internal/network/http_response.h" #include "internal/platform/mutex.h" #include "internal/platform/single_thread_executor.h" @@ -38,21 +41,22 @@ class NearbyHttpClient : public HttpClient { NearbyHttpClient& operator=(NearbyHttpClient&&) = default; void StartRequest( - const HttpRequest& request, + const HttpRequest& request, absl::Duration timeout, absl::AnyInvocable&)> callback) override ABSL_LOCKS_EXCLUDED(mutex_); void StartCancellableRequest( - std::unique_ptr request, + std::unique_ptr request, absl::Duration timeout, absl::AnyInvocable&)> callback) override ABSL_LOCKS_EXCLUDED(mutex_); // Gets HTTP response in synchronization mode. - absl::StatusOr GetResponse(const HttpRequest& request) override; + absl::StatusOr GetResponse(const HttpRequest& request, + absl::Duration timeout) override; private: static absl::StatusOr InternalGetResponse( - const HttpRequest& request); + const HttpRequest& request, absl::Duration timeout); Mutex mutex_; SingleThreadExecutor executor_; diff --git a/internal/network/http_client_impl_test.cc b/internal/network/http_client_impl_test.cc index 67a4550c..edbba3c7 100644 --- a/internal/network/http_client_impl_test.cc +++ b/internal/network/http_client_impl_test.cc @@ -38,6 +38,8 @@ namespace nearby { namespace api { namespace { +constexpr absl::Duration kRequestTimeout = absl::Seconds(30); + struct HttpTestContext { WebRequest web_request; WebResponse web_response; @@ -132,7 +134,7 @@ class NearbyHttpClientTest : public ::testing::Test { absl::Notification notification; client_.StartRequest( - *request, [&result, ¬ification]( + *request, api::kRequestTimeout, [&result, ¬ification]( const absl::StatusOr& http_response) { result = http_response; notification.Notify(); @@ -154,7 +156,7 @@ class NearbyHttpClientTest : public ::testing::Test { return request.status(); } - return client_.GetResponse(*request); + return client_.GetResponse(*request, api::kRequestTimeout); } void CheckHeader(const std::multimap& headers, @@ -191,6 +193,7 @@ TEST_F(NearbyHttpClientTest, TestGet) { api::WebRequest web_request = GetWebRequest(); EXPECT_EQ(web_request.url, "http://www.google.com"); EXPECT_EQ(web_request.method, "GET"); + EXPECT_EQ(web_request.timeout, api::kRequestTimeout); // Checks response. ASSERT_TRUE(result.ok()); @@ -210,6 +213,7 @@ TEST_F(NearbyHttpClientTest, TestGetWithQuery) { api::WebRequest web_request = GetWebRequest(); EXPECT_EQ(web_request.url, "http://www.google.com?name=name1&age=36"); EXPECT_EQ(web_request.method, "GET"); + EXPECT_EQ(web_request.timeout, api::kRequestTimeout); // Checks response. ASSERT_TRUE(result.ok()); @@ -225,6 +229,7 @@ TEST_F(NearbyHttpClientTest, TestGetWithErrorResult) { api::WebRequest web_request = GetWebRequest(); EXPECT_EQ(web_request.url, "http://www.google.com?name=name1&age=36"); EXPECT_EQ(web_request.method, "GET"); + EXPECT_EQ(web_request.timeout, api::kRequestTimeout); // Checks response. EXPECT_FALSE(result.ok()); @@ -240,6 +245,7 @@ TEST_F(NearbyHttpClientTest, TestPostAsync) { api::WebRequest web_request = GetWebRequest(); EXPECT_EQ(web_request.url, "http://www.google.com"); EXPECT_EQ(web_request.method, "POST"); + EXPECT_EQ(web_request.timeout, api::kRequestTimeout); // Checks response. ASSERT_TRUE(result.ok()); @@ -258,6 +264,7 @@ TEST_F(NearbyHttpClientTest, TestPost) { api::WebRequest web_request = GetWebRequest(); EXPECT_EQ(web_request.url, "http://www.google.com"); EXPECT_EQ(web_request.method, "POST"); + EXPECT_EQ(web_request.timeout, api::kRequestTimeout); // Checks response. ASSERT_TRUE(result.ok()); @@ -277,6 +284,7 @@ TEST_F(NearbyHttpClientTest, TestPostWithHeaderAsync) { api::WebRequest web_request = GetWebRequest(); EXPECT_EQ(web_request.url, "http://www.google.com"); EXPECT_EQ(web_request.method, "POST"); + EXPECT_EQ(web_request.timeout, api::kRequestTimeout); ASSERT_NO_FATAL_FAILURE( CheckHeader(web_request.headers, "Content_Type", "text/json")); ASSERT_NO_FATAL_FAILURE(CheckHeader(web_request.headers, "size", "596")); @@ -297,6 +305,7 @@ TEST_F(NearbyHttpClientTest, TestPostWithErrorResultAsync) { api::WebRequest web_request = GetWebRequest(); EXPECT_EQ(web_request.url, "http://www.google.com"); EXPECT_EQ(web_request.method, "POST"); + EXPECT_EQ(web_request.timeout, api::kRequestTimeout); ASSERT_NO_FATAL_FAILURE( CheckHeader(web_request.headers, "Content_Type", "text/json")); ASSERT_NO_FATAL_FAILURE(CheckHeader(web_request.headers, "size", "596")); @@ -315,6 +324,7 @@ TEST_F(NearbyHttpClientTest, TestRequestWithCleanThreadsAsync) { api::WebRequest web_request = GetWebRequest(); EXPECT_EQ(web_request.url, "http://www.google.com"); EXPECT_EQ(web_request.method, "GET"); + EXPECT_EQ(web_request.timeout, api::kRequestTimeout); // Checks response. ASSERT_TRUE(result.ok()); @@ -334,7 +344,7 @@ TEST_F(NearbyHttpClientTest, TestCancellableRequestAsync) { absl::StatusOr result; absl::Notification notification; client().StartCancellableRequest( - std::move(cancellable_request), + std::move(cancellable_request), api::kRequestTimeout, [&](const absl::StatusOr& response) { result = response; notification.Notify(); @@ -362,7 +372,7 @@ TEST_F(NearbyHttpClientTest, TestCancelCancellableRequestAsync) { absl::StatusOr result; absl::Notification notification; client().StartCancellableRequest( - std::move(cancellable_request), + std::move(cancellable_request), api::kRequestTimeout, [&](const absl::StatusOr& response) { result = response; notification.Notify(); @@ -386,7 +396,7 @@ TEST_F(NearbyHttpClientTest, absl::Notification notification; auto client = std::make_unique(); client->StartCancellableRequest( - std::move(cancellable_request), + std::move(cancellable_request), api::kRequestTimeout, [&](const absl::StatusOr& response) { result = response; notification.Notify(); diff --git a/internal/platform/implementation/BUILD b/internal/platform/implementation/BUILD index 3228fec4..032e6cd4 100644 --- a/internal/platform/implementation/BUILD +++ b/internal/platform/implementation/BUILD @@ -192,6 +192,7 @@ cc_library( "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:str_format", + "@com_google_absl//absl/time", "@com_google_absl//absl/types:optional", ], ) diff --git a/internal/platform/implementation/http_loader.h b/internal/platform/implementation/http_loader.h index ceca08fc..d82bb528 100644 --- a/internal/platform/implementation/http_loader.h +++ b/internal/platform/implementation/http_loader.h @@ -18,6 +18,8 @@ #include #include +#include "absl/time/time.h" + namespace nearby { namespace api { @@ -26,6 +28,7 @@ struct WebRequest { std::string method; std::multimap headers; std::string body; + absl::Duration timeout; }; struct WebResponse { diff --git a/internal/platform/implementation/windows/http_loader.cc b/internal/platform/implementation/windows/http_loader.cc index e3295931..619bd7d5 100644 --- a/internal/platform/implementation/windows/http_loader.cc +++ b/internal/platform/implementation/windows/http_loader.cc @@ -14,7 +14,6 @@ #include "internal/platform/implementation/windows/http_loader.h" -#include #include #include "absl/status/status.h" @@ -23,6 +22,7 @@ #include "absl/strings/numbers.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" +#include "absl/time/time.h" #include "internal/platform/implementation/http_loader.h" #include "internal/platform/logging.h" @@ -209,6 +209,21 @@ absl::Status HttpLoader::ConnectWebServer() { return absl::FailedPreconditionError(absl::StrCat(GetLastError())); } + // Set WinInet timeout if provided by caller. + if (request_.timeout > absl::ZeroDuration() && + request_.timeout < absl::InfiniteDuration()) { + DWORD timeout_ms = absl::ToInt64Milliseconds(request_.timeout); + ::InternetSetOptionA(internet_handle_, INTERNET_OPTION_CONNECT_TIMEOUT, + reinterpret_cast(&timeout_ms), + sizeof(timeout_ms)); + ::InternetSetOptionA(internet_handle_, INTERNET_OPTION_SEND_TIMEOUT, + reinterpret_cast(&timeout_ms), + sizeof(timeout_ms)); + ::InternetSetOptionA(internet_handle_, INTERNET_OPTION_RECEIVE_TIMEOUT, + reinterpret_cast(&timeout_ms), + sizeof(timeout_ms)); + } + connect_handle_ = InternetConnectA(internet_handle_, /*Internet*/ host_.c_str(), /*Server name*/ port_, /*Port*/ diff --git a/internal/test/BUILD b/internal/test/BUILD index 1f81b377..e72aa0ac 100644 --- a/internal/test/BUILD +++ b/internal/test/BUILD @@ -77,7 +77,6 @@ cc_library( "@com_google_absl//absl/strings", "@com_google_absl//absl/synchronization", "@com_google_absl//absl/time", - "@com_google_absl//absl/types:span", # fixdeps: keep ], ) @@ -103,8 +102,8 @@ cc_test( "//internal/network:types", "//internal/network:url", "//internal/platform:types", + "//internal/platform/implementation:platform_impl", "//internal/platform/implementation:types", - "//internal/platform/implementation/g3", # fixdeps: keep "@com_github_protobuf_matchers//protobuf-matchers", "@com_google_absl//absl/container:flat_hash_map", "@com_google_absl//absl/status", diff --git a/internal/test/fake_http_client.h b/internal/test/fake_http_client.h index 000143af..ecceee80 100644 --- a/internal/test/fake_http_client.h +++ b/internal/test/fake_http_client.h @@ -27,6 +27,7 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/string_view.h" +#include "absl/time/time.h" #include "internal/network/http_client.h" #include "internal/network/http_request.h" #include "internal/network/http_response.h" @@ -39,6 +40,7 @@ class FakeHttpClient : public HttpClient { public: struct RequestInfo { HttpRequest request; + absl::Duration timeout; absl::AnyInvocable&)> callback; }; @@ -52,27 +54,29 @@ class FakeHttpClient : public HttpClient { FakeHttpClient& operator=(FakeHttpClient&&) = default; void StartRequest( - const HttpRequest& request, + const HttpRequest& request, absl::Duration timeout, absl::AnyInvocable&)> callback) override { RequestInfo request_info; request_info.request = request; + request_info.timeout = timeout; request_info.callback = std::move(callback); request_infos_.push_back(std::move(request_info)); } void StartCancellableRequest( - std::unique_ptr request, + std::unique_ptr request, absl::Duration timeout, absl::AnyInvocable&)> callback) override { RequestInfo request_info; request_info.request = request->http_request(); + request_info.timeout = timeout; request_info.callback = std::move(callback); request_infos_.push_back(std::move(request_info)); } absl::StatusOr GetResponse( - const HttpRequest& request) override { + const HttpRequest& request, absl::Duration timeout) override { if (sync_responses_.empty()) { return absl::FailedPreconditionError("No response."); } diff --git a/internal/test/fake_http_client_test.cc b/internal/test/fake_http_client_test.cc index 2c498b52..98cba373 100644 --- a/internal/test/fake_http_client_test.cc +++ b/internal/test/fake_http_client_test.cc @@ -26,6 +26,7 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/string_view.h" +#include "absl/time/time.h" #include "internal/network/http_request.h" #include "internal/network/http_response.h" #include "internal/network/http_status_code.h" @@ -35,6 +36,8 @@ namespace nearby { namespace network { namespace { +constexpr absl::Duration kRequestTimeout = absl::Seconds(30); + class FakekHttpClientTest : public ::testing::Test { public: void StartRequest( @@ -46,7 +49,7 @@ class FakekHttpClientTest : public ::testing::Test { request.SetUrl(url); request.SetMethod(HttpRequestMethod::kPost); request.SetBody("request body"); - client_.StartRequest(request, callback); + client_.StartRequest(request, kRequestTimeout, callback); } void CompleteRequest( @@ -112,7 +115,8 @@ TEST_F(FakekHttpClientTest, TestGetResponse) { response.SetStatusCode(HttpStatusCode::kHttpOk); client_.SetResponseForSyncRequest(response); // Sync GetResponse - absl::StatusOr result = client_.GetResponse(request); + absl::StatusOr result = + client_.GetResponse(request, kRequestTimeout); ASSERT_OK(result); EXPECT_EQ(result->GetStatusCode(), HttpStatusCode::kHttpOk);