From e17ce3ce06988c9ea5234be0d5d87e260d19815a Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Fri, 29 Jul 2022 14:30:06 -0700 Subject: [PATCH] feat(storage): experimental options to tune stall timeouts (#9593) Some applications need more progress than just "at least 1 byte per second for K seconds". These new options allow them to require "at least N bytes per second for K seconds". --- google/cloud/internal/curl_impl.cc | 10 +++++-- google/cloud/internal/curl_impl.h | 2 ++ google/cloud/internal/rest_options.h | 29 ++++++++++++++++--- google/cloud/storage/client_options.cc | 6 ++++ google/cloud/storage/client_options_test.cc | 10 +++++++ .../storage/internal/curl_download_request.cc | 8 +++-- .../storage/internal/curl_download_request.h | 1 + google/cloud/storage/internal/curl_request.cc | 4 ++- google/cloud/storage/internal/curl_request.h | 1 + .../storage/internal/curl_request_builder.cc | 6 ++++ .../storage/internal/curl_request_builder.h | 2 ++ google/cloud/storage/internal/grpc_client.cc | 23 ++++++++++++--- google/cloud/storage/options.h | 22 ++++++++++++++ 13 files changed, 110 insertions(+), 14 deletions(-) diff --git a/google/cloud/internal/curl_impl.cc b/google/cloud/internal/curl_impl.cc index b03cd0e5ecc1a..bc3f2e7ccdc92 100644 --- a/google/cloud/internal/curl_impl.cc +++ b/google/cloud/internal/curl_impl.cc @@ -178,7 +178,9 @@ void CurlImpl::ApplyOptions(Options const& options) { user_agent_ = absl::StrCat(absl::StrJoin(agents, " "), UserAgentSuffix()); http_version_ = std::move(options.get()); transfer_stall_timeout_ = options.get(); + transfer_stall_minimum_rate_ = options.get(); download_stall_timeout_ = options.get(); + transfer_stall_minimum_rate_ = options.get(); ignored_http_error_codes_ = options.get(); } @@ -726,11 +728,13 @@ Status CurlImpl::MakeRequest(CurlImpl::HttpMethod method, if (download_stall_timeout_.count() != 0) { // NOLINTNEXTLINE(google-runtime-int) - libcurl *requires* `long` auto const timeout = static_cast(download_stall_timeout_.count()); + // NOLINTNEXTLINE(google-runtime-int) - libcurl *requires* `long` + auto const limit = static_cast(download_stall_minimum_rate_); status = handle_.SetOption(CURLOPT_CONNECTTIMEOUT, timeout); if (!status.ok()) return OnTransferError(std::move(status)); // Timeout if the request sends or receives less than 1 byte/second (i.e. // effectively no bytes) for `download_stall_timeout_` seconds. - status = handle_.SetOption(CURLOPT_LOW_SPEED_LIMIT, 1L); + status = handle_.SetOption(CURLOPT_LOW_SPEED_LIMIT, limit); if (!status.ok()) return OnTransferError(std::move(status)); status = handle_.SetOption(CURLOPT_LOW_SPEED_TIME, timeout); if (!status.ok()) return OnTransferError(std::move(status)); @@ -741,11 +745,13 @@ Status CurlImpl::MakeRequest(CurlImpl::HttpMethod method, if (transfer_stall_timeout_.count() != 0) { // NOLINTNEXTLINE(google-runtime-int) - libcurl *requires* `long` auto const timeout = static_cast(transfer_stall_timeout_.count()); + // NOLINTNEXTLINE(google-runtime-int) - libcurl *requires* `long` + auto const limit = static_cast(transfer_stall_minimum_rate_); status = handle_.SetOption(CURLOPT_CONNECTTIMEOUT, timeout); if (!status.ok()) return OnTransferError(std::move(status)); // Timeout if the request sends or receives less than 1 byte/second (i.e. // effectively no bytes) for `transfer_stall_timeout_` seconds. - status = handle_.SetOption(CURLOPT_LOW_SPEED_LIMIT, 1L); + status = handle_.SetOption(CURLOPT_LOW_SPEED_LIMIT, limit); if (!status.ok()) return OnTransferError(std::move(status)); status = handle_.SetOption(CURLOPT_LOW_SPEED_TIME, timeout); if (!status.ok()) return OnTransferError(std::move(status)); diff --git a/google/cloud/internal/curl_impl.h b/google/cloud/internal/curl_impl.h index 681cd5c4f6c66..84e4ac963c98d 100644 --- a/google/cloud/internal/curl_impl.h +++ b/google/cloud/internal/curl_impl.h @@ -127,7 +127,9 @@ class CurlImpl { bool logging_enabled_ = false; CurlHandle::SocketOptions socket_options_; std::chrono::seconds transfer_stall_timeout_; + std::uint32_t transfer_stall_minimum_rate_ = 1; std::chrono::seconds download_stall_timeout_; + std::uint32_t download_stall_minimum_rate_ = 1; std::string http_version_; std::int32_t http_code_; std::set ignored_http_error_codes_; diff --git a/google/cloud/internal/rest_options.h b/google/cloud/internal/rest_options.h index ef92ed1f119c3..f6df271cf0549 100644 --- a/google/cloud/internal/rest_options.h +++ b/google/cloud/internal/rest_options.h @@ -18,6 +18,7 @@ #include "google/cloud/options.h" #include "google/cloud/version.h" #include +#include #include #include @@ -56,6 +57,16 @@ struct TransferStallTimeoutOption { using Type = std::chrono::seconds; }; +/** + * The minimum accepted bytes/second transfer rate. + * + * If the average rate is below this value for the `TransferStallTimeoutOption` + * then the transfer is aborted. + */ +struct TransferStallMinimumRateOption { + using Type = std::int32_t; +}; + /** * Sets the download stall timeout. * @@ -74,6 +85,16 @@ struct DownloadStallTimeoutOption { using Type = std::chrono::seconds; }; +/** + * The minimum accepted bytes/second download rate. + * + * If the average rate is below this value for the `DownloadStallTimeoutOption` + * then the download is aborted. + */ +struct DownloadStallMinimumRateOption { + using Type = std::int32_t; +}; + /** * Some services appropriate Http error codes for their own use. If any such * error codes need to be treated as non-failures, this option can indicate @@ -84,10 +105,10 @@ struct IgnoredHttpErrorCodes { }; /// The complete list of options accepted by `CurlRestClient` -using RestOptionList = - ::google::cloud::OptionList; +using RestOptionList = ::google::cloud::OptionList< + UserIpOption, TransferStallTimeoutOption, TransferStallMinimumRateOption, + DownloadStallTimeoutOption, DownloadStallMinimumRateOption, + IgnoredHttpErrorCodes>; GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace rest_internal diff --git a/google/cloud/storage/client_options.cc b/google/cloud/storage/client_options.cc index 59342f212dfea..679f2580e38c9 100644 --- a/google/cloud/storage/client_options.cc +++ b/google/cloud/storage/client_options.cc @@ -187,6 +187,8 @@ Options DefaultOptions(std::shared_ptr credentials, .set(0) .set(std::chrono::seconds( GOOGLE_CLOUD_CPP_STORAGE_DEFAULT_DOWNLOAD_STALL_TIMEOUT)) + .set(1) + .set(1) .set( LimitedTimeRetryPolicy( STORAGE_CLIENT_DEFAULT_MAXIMUM_RETRY_PERIOD) @@ -238,8 +240,12 @@ Options DefaultOptions(std::shared_ptr credentials, Options{} .set( o.get()) + .set( + o.get()) .set( o.get()) + .set( + o.get()) .set( o.get()) .set( diff --git a/google/cloud/storage/client_options_test.cc b/google/cloud/storage/client_options_test.cc index 12f146ec08148..e51eafdd4d68f 100644 --- a/google/cloud/storage/client_options_test.cc +++ b/google/cloud/storage/client_options_test.cc @@ -325,6 +325,8 @@ TEST_F(ClientOptionsTest, MakeOptionsFromDefault) { EXPECT_EQ("test-project-id", opts.get()); EXPECT_LT(0, opts.get()); EXPECT_LT(0, opts.get()); + EXPECT_LT(0, + opts.get()); EXPECT_LT(0, opts.get()); EXPECT_LT(0, opts.get()); EXPECT_TRUE(opts.has()); @@ -332,6 +334,8 @@ TEST_F(ClientOptionsTest, MakeOptionsFromDefault) { EXPECT_EQ(0, opts.get()); EXPECT_EQ(0, opts.get()); EXPECT_LT(0, opts.get().count()); + EXPECT_LT(0, + opts.get()); EXPECT_THAT(opts.get(), IsEmpty()); } @@ -360,13 +364,19 @@ TEST_F(ClientOptionsTest, DefaultOptions) { EXPECT_EQ(0, o.get()); EXPECT_EQ(0, o.get()); EXPECT_LT(std::chrono::seconds(0), o.get()); + EXPECT_LT(0, o.get()); EXPECT_LT(std::chrono::seconds(0), o.get()); + EXPECT_LT(0, o.get()); namespace rest = ::google::cloud::rest_internal; EXPECT_EQ(o.get(), o.get()); + EXPECT_EQ(o.get(), + o.get()); EXPECT_EQ(o.get(), o.get()); + EXPECT_EQ(o.get(), + o.get()); EXPECT_EQ(o.get(), o.get()); EXPECT_EQ(o.get(), diff --git a/google/cloud/storage/internal/curl_download_request.cc b/google/cloud/storage/internal/curl_download_request.cc index 376499c8394c2..209e066fe3a6e 100644 --- a/google/cloud/storage/internal/curl_download_request.cc +++ b/google/cloud/storage/internal/curl_download_request.cc @@ -261,11 +261,13 @@ Status CurlDownloadRequest::SetOptions() { if (download_stall_timeout_.count() != 0) { // NOLINTNEXTLINE(google-runtime-int) - libcurl *requires* `long` auto const timeout = static_cast(download_stall_timeout_.count()); + // NOLINTNEXTLINE(google-runtime-int) - libcurl *requires* `long` + auto const limit = static_cast(download_stall_minimum_rate_); status = handle_.SetOption(CURLOPT_CONNECTTIMEOUT, timeout); if (!status.ok()) return OnTransferError(std::move(status)); - // Timeout if the download receives less than 1 byte/second (i.e. - // effectively no bytes) for `transfer_stall_timeout_` seconds. - status = handle_.SetOption(CURLOPT_LOW_SPEED_LIMIT, 1L); + // Timeout if the download receives less than `limit` bytes in `timeout` + // seconds for `transfer_stall_timeout_` seconds. + status = handle_.SetOption(CURLOPT_LOW_SPEED_LIMIT, limit); if (!status.ok()) return OnTransferError(std::move(status)); status = handle_.SetOption(CURLOPT_LOW_SPEED_TIME, timeout); if (!status.ok()) return OnTransferError(std::move(status)); diff --git a/google/cloud/storage/internal/curl_download_request.h b/google/cloud/storage/internal/curl_download_request.h index 43d32226e7423..e074fdbde1093 100644 --- a/google/cloud/storage/internal/curl_download_request.h +++ b/google/cloud/storage/internal/curl_download_request.h @@ -135,6 +135,7 @@ class CurlDownloadRequest : public ObjectReadSource { bool logging_enabled_ = false; CurlHandle::SocketOptions socket_options_; std::chrono::seconds download_stall_timeout_; + std::uint32_t download_stall_minimum_rate_ = 0; CurlHandle handle_; rest_internal::CurlMulti multi_; std::shared_ptr factory_; diff --git a/google/cloud/storage/internal/curl_request.cc b/google/cloud/storage/internal/curl_request.cc index a205fa3ab0bf3..7cb083734168a 100644 --- a/google/cloud/storage/internal/curl_request.cc +++ b/google/cloud/storage/internal/curl_request.cc @@ -120,10 +120,12 @@ StatusOr CurlRequest::MakeRequestImpl() { if (transfer_stall_timeout_.count() != 0) { // NOLINTNEXTLINE(google-runtime-int) - libcurl *requires* `long` auto const timeout = static_cast(transfer_stall_timeout_.count()); + // NOLINTNEXTLINE(google-runtime-int) - libcurl *requires* `long` + auto const limit = static_cast(transfer_stall_minimum_rate_); handle_.SetOption(CURLOPT_CONNECTTIMEOUT, timeout); // Timeout if the request sends or receives less than 1 byte/second (i.e. // effectively no bytes) for `transfer_stall_timeout_` seconds. - handle_.SetOption(CURLOPT_LOW_SPEED_LIMIT, 1L); + handle_.SetOption(CURLOPT_LOW_SPEED_LIMIT, limit); handle_.SetOption(CURLOPT_LOW_SPEED_TIME, timeout); } auto status = handle_.EasyPerform(); diff --git a/google/cloud/storage/internal/curl_request.h b/google/cloud/storage/internal/curl_request.h index b3324764571da..2b93c04a99749 100644 --- a/google/cloud/storage/internal/curl_request.h +++ b/google/cloud/storage/internal/curl_request.h @@ -78,6 +78,7 @@ class CurlRequest { bool logging_enabled_ = false; CurlHandle::SocketOptions socket_options_; std::chrono::seconds transfer_stall_timeout_; + std::uint32_t transfer_stall_minimum_rate_; CurlHandle handle_; std::shared_ptr factory_; }; diff --git a/google/cloud/storage/internal/curl_request_builder.cc b/google/cloud/storage/internal/curl_request_builder.cc index 7e60476ec21e7..c23d47612d423 100644 --- a/google/cloud/storage/internal/curl_request_builder.cc +++ b/google/cloud/storage/internal/curl_request_builder.cc @@ -59,6 +59,7 @@ CurlRequest CurlRequestBuilder::BuildRequest() && { request.logging_enabled_ = logging_enabled_; request.socket_options_ = socket_options_; request.transfer_stall_timeout_ = transfer_stall_timeout_; + request.transfer_stall_minimum_rate_ = transfer_stall_minimum_rate_; return request; } @@ -75,6 +76,7 @@ CurlRequestBuilder::BuildDownloadRequest() && { request->logging_enabled_ = logging_enabled_; request->socket_options_ = socket_options_; request->download_stall_timeout_ = download_stall_timeout_; + request->download_stall_minimum_rate_ = download_stall_minimum_rate_; auto status = request->SetOptions(); if (!status.ok()) return status; return request; @@ -95,7 +97,11 @@ CurlRequestBuilder& CurlRequestBuilder::ApplyClientOptions( http_version_ = std::move(options.get()); transfer_stall_timeout_ = options.get(); + transfer_stall_minimum_rate_ = + options.get(); download_stall_timeout_ = options.get(); + download_stall_minimum_rate_ = + options.get(); return *this; } diff --git a/google/cloud/storage/internal/curl_request_builder.h b/google/cloud/storage/internal/curl_request_builder.h index 2f352d952e932..6c2a6f47d3626 100644 --- a/google/cloud/storage/internal/curl_request_builder.h +++ b/google/cloud/storage/internal/curl_request_builder.h @@ -197,7 +197,9 @@ class CurlRequestBuilder { bool logging_enabled_ = false; CurlHandle::SocketOptions socket_options_; std::chrono::seconds transfer_stall_timeout_; + std::uint32_t transfer_stall_minimum_rate_ = 0; std::chrono::seconds download_stall_timeout_; + std::uint32_t download_stall_minimum_rate_ = 0; std::string http_version_; }; diff --git a/google/cloud/storage/internal/grpc_client.cc b/google/cloud/storage/internal/grpc_client.cc index 11286ed3c6308..aca2d7ae1f4e8 100644 --- a/google/cloud/storage/internal/grpc_client.cc +++ b/google/cloud/storage/internal/grpc_client.cc @@ -114,6 +114,14 @@ void MaybeFinalize(google::storage::v2::WriteObjectRequest& write_request, } } +std::chrono::milliseconds ScaleStallTimeout(std::chrono::milliseconds timeout, + std::uint32_t size, + std::uint32_t quantum) { + if (timeout == std::chrono::milliseconds(0)) return timeout; + if (quantum <= size || size == 0) return timeout; + return timeout * quantum / size; +} + Status TimeoutError(std::chrono::milliseconds timeout, std::string const& op) { return Status(StatusCode::kDeadlineExceeded, "timeout [" + absl::FormatDuration(absl::FromChrono(timeout)) + @@ -346,8 +354,11 @@ StatusOr GrpcClient::InsertObjectMedia( if (!r) return std::move(r).status(); auto proto_request = *r; - auto timeout = google::cloud::internal::CurrentOptions() - .get(); + auto const& current = google::cloud::internal::CurrentOptions(); + auto timeout = ScaleStallTimeout( + current.get(), + current.get(), + google::storage::v2::ServiceConstants::MAX_WRITE_CHUNK_BYTES); auto create_watchdog = [cq = background_->cq(), timeout]() mutable { if (timeout == std::chrono::seconds(0)) { return make_ready_future(false); @@ -606,8 +617,12 @@ StatusOr GrpcClient::DeleteResumableUpload( StatusOr GrpcClient::UploadChunk( UploadChunkRequest const& request) { - auto const timeout = google::cloud::internal::CurrentOptions() - .get(); + auto const& current = google::cloud::internal::CurrentOptions(); + auto const timeout = ScaleStallTimeout( + current.get(), + current.get(), + google::storage::v2::ServiceConstants::MAX_WRITE_CHUNK_BYTES); + auto create_watchdog = [cq = background_->cq(), timeout]() mutable { if (timeout == std::chrono::seconds(0)) { return make_ready_future(false); diff --git a/google/cloud/storage/options.h b/google/cloud/storage/options.h index 8cebfc7490d9e..e80e1d4672ff0 100644 --- a/google/cloud/storage/options.h +++ b/google/cloud/storage/options.h @@ -23,6 +23,7 @@ #include "google/cloud/credentials.h" #include "google/cloud/options.h" #include +#include #include #include @@ -48,6 +49,27 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN struct HttpVersionOption { using Type = std::string; }; + +/** + * The minimum accepted bytes/second transfer rate. + * + * If the average rate is below this value for the `TransferStallTimeoutOption` + * then the transfer is aborted. + */ +struct TransferStallMinimumRateOption { + using Type = std::int32_t; +}; + +/** + * The minimum accepted bytes/second download rate. + * + * If the average rate is below this value for the `DownloadStallTimeoutOption` + * then the download is aborted. + */ +struct DownloadStallMinimumRateOption { + using Type = std::int32_t; +}; + GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END } // namespace storage_experimental