From 5b611c08900cba72d9770bb45ca8055175b2938d Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Tue, 23 May 2023 16:52:33 +0000 Subject: [PATCH 1/2] fix(rest): support rewinds in libcurl This fixes problems I introduced in the first PR. --- .../cloud/google_cloud_cpp_rest_internal.bzl | 2 + .../google_cloud_cpp_rest_internal.cmake | 3 + ...gle_cloud_cpp_rest_internal_unit_tests.bzl | 1 + google/cloud/internal/curl_impl.cc | 79 +++---------------- google/cloud/internal/curl_writev.cc | 43 ++++++++++ google/cloud/internal/curl_writev.h | 75 ++++++++++++++++++ google/cloud/internal/curl_writev_test.cc | 65 +++++++++++++++ 7 files changed, 199 insertions(+), 69 deletions(-) create mode 100644 google/cloud/internal/curl_writev.cc create mode 100644 google/cloud/internal/curl_writev.h create mode 100644 google/cloud/internal/curl_writev_test.cc diff --git a/google/cloud/google_cloud_cpp_rest_internal.bzl b/google/cloud/google_cloud_cpp_rest_internal.bzl index 9176859c47a8b..bbc1fe90881f6 100644 --- a/google/cloud/google_cloud_cpp_rest_internal.bzl +++ b/google/cloud/google_cloud_cpp_rest_internal.bzl @@ -26,6 +26,7 @@ google_cloud_cpp_rest_internal_hdrs = [ "internal/curl_rest_client.h", "internal/curl_rest_response.h", "internal/curl_wrappers.h", + "internal/curl_writev.h", "internal/external_account_source_format.h", "internal/external_account_token_source_aws.h", "internal/external_account_token_source_file.h", @@ -77,6 +78,7 @@ google_cloud_cpp_rest_internal_srcs = [ "internal/curl_rest_client.cc", "internal/curl_rest_response.cc", "internal/curl_wrappers.cc", + "internal/curl_writev.cc", "internal/external_account_source_format.cc", "internal/external_account_token_source_aws.cc", "internal/external_account_token_source_file.cc", diff --git a/google/cloud/google_cloud_cpp_rest_internal.cmake b/google/cloud/google_cloud_cpp_rest_internal.cmake index fc485e0f99ea6..7e3ea2f2edfe9 100644 --- a/google/cloud/google_cloud_cpp_rest_internal.cmake +++ b/google/cloud/google_cloud_cpp_rest_internal.cmake @@ -38,6 +38,8 @@ add_library( internal/curl_rest_response.h internal/curl_wrappers.cc internal/curl_wrappers.h + internal/curl_writev.cc + internal/curl_writev.h internal/external_account_source_format.cc internal/external_account_source_format.h internal/external_account_token_source_aws.cc @@ -221,6 +223,7 @@ if (BUILD_TESTING) internal/curl_wrappers_locking_disabled_test.cc internal/curl_wrappers_locking_enabled_test.cc internal/curl_wrappers_test.cc + internal/curl_writev_test.cc internal/external_account_source_format_test.cc internal/external_account_token_source_aws_test.cc internal/external_account_token_source_file_test.cc diff --git a/google/cloud/google_cloud_cpp_rest_internal_unit_tests.bzl b/google/cloud/google_cloud_cpp_rest_internal_unit_tests.bzl index 8aba0bdf23797..7b4dfdd65f72b 100644 --- a/google/cloud/google_cloud_cpp_rest_internal_unit_tests.bzl +++ b/google/cloud/google_cloud_cpp_rest_internal_unit_tests.bzl @@ -29,6 +29,7 @@ google_cloud_cpp_rest_internal_unit_tests = [ "internal/curl_wrappers_locking_disabled_test.cc", "internal/curl_wrappers_locking_enabled_test.cc", "internal/curl_wrappers_test.cc", + "internal/curl_writev_test.cc", "internal/external_account_source_format_test.cc", "internal/external_account_token_source_aws_test.cc", "internal/external_account_token_source_file_test.cc", diff --git a/google/cloud/internal/curl_impl.cc b/google/cloud/internal/curl_impl.cc index 0468ea85a3092..5d676a9e5c2b8 100644 --- a/google/cloud/internal/curl_impl.cc +++ b/google/cloud/internal/curl_impl.cc @@ -18,6 +18,7 @@ #include "google/cloud/internal/absl_str_join_quiet.h" #include "google/cloud/internal/algorithm.h" #include "google/cloud/internal/curl_options.h" +#include "google/cloud/internal/curl_writev.h" #include "google/cloud/internal/make_status.h" #include "google/cloud/internal/rest_options.h" #include "google/cloud/internal/user_agent_prefix.h" @@ -25,8 +26,6 @@ #include "absl/strings/match.h" #include "absl/strings/strip.h" #include -#include -#include #include #include @@ -87,64 +86,7 @@ Status AsStatus(CURLMcode result, char const* where) { return internal::UnknownError(std::move(os).str()); } -// Vector of data chunks to satisfy requests from libcurl. -class WriteVector { - public: - explicit WriteVector(std::vector> v) - : original_(std::move(v)), writev_(original_.begin(), original_.end()) {} - - std::size_t size() const { - std::size_t size = 0; - for (auto const& s : writev_) { - size += s.size(); - } - return size; - } - - std::size_t MoveTo(absl::Span dst) { - auto const avail = dst.size(); - while (!writev_.empty()) { - auto& src = writev_.front(); - if (src.size() > dst.size()) { - std::copy(src.begin(), src.begin() + dst.size(), dst.begin()); - src.remove_prefix(dst.size()); - dst.remove_prefix(dst.size()); - break; - } - std::copy(src.begin(), src.end(), dst.begin()); - dst.remove_prefix(src.size()); - writev_.pop_front(); - } - return avail - dst.size(); - } - - /// Implements a CURLOPT_SEEKFUNCTION callback. - /// - /// @see https://curl.se/libcurl/c/CURLOPT_SEEKFUNCTION.html - /// @returns true if the seek operation was successful. - bool Seek(std::size_t offset, int origin) { - // libcurl claims to only req - if (origin != SEEK_SET) return false; - writev_.assign(original_.begin(), original_.end()); - // Reverse the vector so the first chunk is at the end. - std::reverse(writev_.begin(), writev_.end()); - while (!writev_.empty()) { - auto& src = writev_.front(); - if (src.size() >= offset) { - src.remove_prefix(offset); - offset = 0; - break; - } - offset -= src.size(); - writev_.pop_front(); - } - return offset == 0; - } - - private: - std::vector> original_; - std::deque> writev_; -}; +} // namespace extern "C" { // libcurl callbacks @@ -152,27 +94,28 @@ extern "C" { // libcurl callbacks // our own buffers (i.e., without an extra copy). But, there is no such API. // Receive response data from peer. -std::size_t WriteFunction(char* ptr, size_t size, size_t nmemb, - void* userdata) { +static std::size_t WriteFunction( // NOLINT(misc-use-anonymous-namespace) + char* ptr, size_t size, size_t nmemb, void* userdata) { auto* const request = reinterpret_cast(userdata); return request->WriteCallback(absl::MakeSpan(ptr, size * nmemb)); } // Receive a response header from peer. -std::size_t HeaderFunction(char* buffer, std::size_t size, std::size_t nitems, - void* userdata) { +static std::size_t HeaderFunction( // NOLINT(misc-use-anonymous-namespace) + char* buffer, std::size_t size, std::size_t nitems, void* userdata) { auto* const request = reinterpret_cast(userdata); return request->HeaderCallback(absl::MakeSpan(buffer, size * nitems)); } // Fill buffer to send data to peer (POST/PUT). -std::size_t ReadFunction(char* buffer, std::size_t size, std::size_t nitems, - void* userdata) { +static std::size_t ReadFunction( // NOLINT(misc-use-anonymous-namespace) + char* buffer, std::size_t size, std::size_t nitems, void* userdata) { auto* const writev = reinterpret_cast(userdata); return writev->MoveTo(absl::MakeSpan(buffer, size * nitems)); } -int SeekFunction(void* userdata, curl_off_t offset, int origin) { +static int SeekFunction( // NOLINT(misc-use-anonymous-namespace) + void* userdata, curl_off_t offset, int origin) { auto* const writev = reinterpret_cast(userdata); return writev->Seek(static_cast(offset), origin) ? CURL_SEEKFUNC_OK @@ -181,8 +124,6 @@ int SeekFunction(void* userdata, curl_off_t offset, int origin) { } // extern "C" -} // namespace - std::size_t SpillBuffer::CopyFrom(absl::Span src) { // capacity() is CURL_MAX_WRITE_SIZE, the maximum amount of data that // libcurl will pass to CurlImpl::WriteCallback(). However, it can give diff --git a/google/cloud/internal/curl_writev.cc b/google/cloud/internal/curl_writev.cc new file mode 100644 index 0000000000000..dba4a21a95229 --- /dev/null +++ b/google/cloud/internal/curl_writev.cc @@ -0,0 +1,43 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "google/cloud/internal/curl_writev.h" +#include + +namespace google { +namespace cloud { +namespace rest_internal { +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN + +bool WriteVector::Seek(std::size_t offset, int origin) { + // libcurl claims to only req + if (origin != SEEK_SET) return false; + writev_.assign(original_.begin(), original_.end()); + while (!writev_.empty()) { + auto& src = writev_.front(); + if (src.size() >= offset) { + src.remove_prefix(offset); + offset = 0; + break; + } + offset -= src.size(); + writev_.pop_front(); + } + return offset == 0; +} + +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END +} // namespace rest_internal +} // namespace cloud +} // namespace google diff --git a/google/cloud/internal/curl_writev.h b/google/cloud/internal/curl_writev.h new file mode 100644 index 0000000000000..03264b71f3b84 --- /dev/null +++ b/google/cloud/internal/curl_writev.h @@ -0,0 +1,75 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_CURL_WRITEV_H +#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_CURL_WRITEV_H + +#include "google/cloud/version.h" +#include "absl/types/span.h" +#include +#include + +namespace google { +namespace cloud { +namespace rest_internal { +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN + +// Vector of data chunks to satisfy requests from libcurl. +class WriteVector { + public: + explicit WriteVector(std::vector> v) + : original_(std::move(v)), writev_(original_.begin(), original_.end()) {} + + std::size_t size() const { + std::size_t size = 0; + for (auto const& s : writev_) { + size += s.size(); + } + return size; + } + + std::size_t MoveTo(absl::Span dst) { + auto const avail = dst.size(); + while (!writev_.empty()) { + auto& src = writev_.front(); + if (src.size() > dst.size()) { + std::copy(src.begin(), src.begin() + dst.size(), dst.begin()); + src.remove_prefix(dst.size()); + dst.remove_prefix(dst.size()); + break; + } + std::copy(src.begin(), src.end(), dst.begin()); + dst.remove_prefix(src.size()); + writev_.pop_front(); + } + return avail - dst.size(); + } + + /// Implements a CURLOPT_SEEKFUNCTION callback. + /// + /// @see https://curl.se/libcurl/c/CURLOPT_SEEKFUNCTION.html + /// @returns true if the seek operation was successful. + bool Seek(std::size_t offset, int origin); + + private: + std::vector> original_; + std::deque> writev_; +}; + +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END +} // namespace rest_internal +} // namespace cloud +} // namespace google + +#endif // GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_INTERNAL_CURL_WRITEV_H diff --git a/google/cloud/internal/curl_writev_test.cc b/google/cloud/internal/curl_writev_test.cc new file mode 100644 index 0000000000000..c3f57a98d797c --- /dev/null +++ b/google/cloud/internal/curl_writev_test.cc @@ -0,0 +1,65 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "google/cloud/internal/curl_writev.h" +#include +#include +#include + +namespace google { +namespace cloud { +namespace rest_internal { +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN + +TEST(WriteVector, Simple) { + auto const a = std::string(32, 'a'); + auto const b = std::string(32, 'b'); + auto tested = + WriteVector({absl::Span(a), absl::Span(b)}); + + auto buffer = std::vector(a.size() + b.size(), 'c'); + auto offset = std::size_t{0}; + ASSERT_EQ(4, tested.MoveTo(absl::MakeSpan(buffer.data() + offset, 4))); + offset += 4; + ASSERT_EQ(32, tested.MoveTo(absl::MakeSpan(buffer.data() + offset, 32))); + offset += 32; + ASSERT_EQ(28, tested.MoveTo(absl::MakeSpan(buffer.data() + offset, 64))); + offset += 28; + EXPECT_EQ(std::string(buffer.begin(), buffer.end()), a + b); +} + +TEST(WriteVector, Rewind) { + auto const a = std::string(32, 'a'); + auto const b = std::string(32, 'b'); + auto tested = + WriteVector({absl::Span(a), absl::Span(b)}); + + auto buffer = std::vector(a.size() + b.size(), 'c'); + auto offset = std::size_t{0}; + ASSERT_EQ(4, tested.MoveTo(absl::MakeSpan(buffer.data() + offset, 4))); + offset += 4; + ASSERT_EQ(32, tested.MoveTo(absl::MakeSpan(buffer.data() + offset, 32))); + offset += 32; + tested.Seek(16, SEEK_SET); + offset = 16; + ASSERT_EQ(32, tested.MoveTo(absl::MakeSpan(buffer.data() + offset, 32))); + offset += 32; + ASSERT_EQ(16, tested.MoveTo(absl::MakeSpan(buffer.data() + offset, 32))); + EXPECT_EQ(std::string(buffer.begin(), buffer.end()), a + b); +} + +GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END +} // namespace rest_internal +} // namespace cloud +} // namespace google From 3abc61296268ee6afb02fac56eced81efbfd5a52 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Tue, 23 May 2023 19:52:04 +0000 Subject: [PATCH 2/2] Address review comments --- google/cloud/internal/curl_writev.cc | 31 +++++++++++++++++++++++++++- google/cloud/internal/curl_writev.h | 29 ++++++-------------------- 2 files changed, 36 insertions(+), 24 deletions(-) diff --git a/google/cloud/internal/curl_writev.cc b/google/cloud/internal/curl_writev.cc index dba4a21a95229..06ba2a685b235 100644 --- a/google/cloud/internal/curl_writev.cc +++ b/google/cloud/internal/curl_writev.cc @@ -20,8 +20,37 @@ namespace cloud { namespace rest_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN +std::size_t WriteVector::size() const { + std::size_t size = 0; + for (auto const& s : writev_) { + size += s.size(); + } + return size; +} + +std::size_t WriteVector::MoveTo(absl::Span dst) { + auto const avail = dst.size(); + while (!writev_.empty()) { + auto& src = writev_.front(); + if (src.size() > dst.size()) { + std::copy(src.begin(), src.begin() + dst.size(), dst.begin()); + src.remove_prefix(dst.size()); + dst.remove_prefix(dst.size()); + break; + } + std::copy(src.begin(), src.end(), dst.begin()); + dst.remove_prefix(src.size()); + writev_.pop_front(); + } + return avail - dst.size(); +} + bool WriteVector::Seek(std::size_t offset, int origin) { - // libcurl claims to only req + // libcurl claims to only require support for `SEEK_SET`, so we only support + // that. If libcurl ever uses any other `origin` the seek operation will + // fail, causing the libcurl request to fail with CURLE_SEND_FAIL_REWIND. + // These errors are treated as `StatusCode::kUnavailable` and thus retryable + // for most operations. if (origin != SEEK_SET) return false; writev_.assign(original_.begin(), original_.end()); while (!writev_.empty()) { diff --git a/google/cloud/internal/curl_writev.h b/google/cloud/internal/curl_writev.h index 03264b71f3b84..00adba2853173 100644 --- a/google/cloud/internal/curl_writev.h +++ b/google/cloud/internal/curl_writev.h @@ -31,30 +31,13 @@ class WriteVector { explicit WriteVector(std::vector> v) : original_(std::move(v)), writev_(original_.begin(), original_.end()) {} - std::size_t size() const { - std::size_t size = 0; - for (auto const& s : writev_) { - size += s.size(); - } - return size; - } + /// Returns the number of bytes available in the write vector. + std::size_t size() const; - std::size_t MoveTo(absl::Span dst) { - auto const avail = dst.size(); - while (!writev_.empty()) { - auto& src = writev_.front(); - if (src.size() > dst.size()) { - std::copy(src.begin(), src.begin() + dst.size(), dst.begin()); - src.remove_prefix(dst.size()); - dst.remove_prefix(dst.size()); - break; - } - std::copy(src.begin(), src.end(), dst.begin()); - dst.remove_prefix(src.size()); - writev_.pop_front(); - } - return avail - dst.size(); - } + /// Copies as much data as possible from the internal vector to @p dst. + /// + /// @return the number of bytes copied. + std::size_t MoveTo(absl::Span dst); /// Implements a CURLOPT_SEEKFUNCTION callback. ///