From 1f911e388885ae61afc0a55a03fb58f3e7abd5e4 Mon Sep 17 00:00:00 2001 From: Scott Hart Date: Tue, 6 Aug 2024 15:36:56 -0400 Subject: [PATCH 1/3] impl(rest): set ReadFunctionAbort via RAII --- google/cloud/internal/curl_impl.cc | 60 ++++++++++++++++++------------ google/cloud/internal/curl_impl.h | 1 + 2 files changed, 37 insertions(+), 24 deletions(-) diff --git a/google/cloud/internal/curl_impl.cc b/google/cloud/internal/curl_impl.cc index 107210aa0c25c..aecfd56f2e9a1 100644 --- a/google/cloud/internal/curl_impl.cc +++ b/google/cloud/internal/curl_impl.cc @@ -487,6 +487,22 @@ std::size_t CurlImpl::HeaderCallback(absl::Span response) { response.size()); } +class CurlImpl::ReadFunctionAbortGuard { + public: + explicit ReadFunctionAbortGuard(CurlImpl& impl) : impl_(impl) {} + ~ReadFunctionAbortGuard() { + // If curl_closed_ is true, then the handle has already been recycled and + // attempting to set an option on it will error. + if (impl_.curl_closed_) { + impl_.handle_.SetOptionUnchecked(CURLOPT_READFUNCTION, + &ReadFunctionAbort); + } + } + + private: + CurlImpl& impl_; +}; + Status CurlImpl::MakeRequestImpl(RestContext& context) { TRACE_STATE() << ", url_=" << url_; @@ -509,33 +525,29 @@ Status CurlImpl::MakeRequestImpl(RestContext& context) { handle_.SetOptionUnchecked(CURLOPT_HTTP_VERSION, VersionToCurlCode(http_version_)); - auto error = curl_multi_add_handle(multi_.get(), handle_.handle_.get()); - - // This indicates that we are using the API incorrectly. The application - // can not recover from these problems, so terminating is the right thing - // to do. - if (error != CURLM_OK) { - GCP_LOG(FATAL) << ", status=" << AsStatus(error, __func__); - } - - in_multi_ = true; + // All data in the WriteVector should be written after ReadImpl returns unless + // an error, typically a timeout, has occurred. Use ReadFunctionAbortGuard to + // instruct curl to not attempt to send anymore data on this handle regardless + // if an error or exception is encountered. + { + ReadFunctionAbortGuard guard(*this); + auto error = curl_multi_add_handle(multi_.get(), handle_.handle_.get()); + + // This indicates that we are using the API incorrectly. The application + // can not recover from these problems, so terminating is the right thing + // to do. + if (error != CURLM_OK) { + GCP_LOG(FATAL) << ", status=" << AsStatus(error, __func__); + } - // This call to Read() should send the request, get the response, and - // thus make available the status_code and headers. Any response data - // should be put into the spill buffer, which makes them available for - // subsequent calls to Read() after the headers have been extracted. - auto read_status = ReadImpl(context, {}).status(); + in_multi_ = true; - // All data in the WriteVector should have been written by now, unless an - // error, typically a timeout, has occurred. Instruct curl to not attempt to - // send anymore data on this handle. If curl_closed_ is true, then the handle - // has already been recycled and attempting to set an option on it will error. - if (!curl_closed_) { - status = handle_.SetOption(CURLOPT_READFUNCTION, &ReadFunctionAbort); - if (!status.ok()) return OnTransferError(context, std::move(status)); + // This call to Read() should send the request, get the response, and + // thus make available the status_code and headers. Any response data + // should be put into the spill buffer, which makes them available for + // subsequent calls to Read() after the headers have been extracted. + return ReadImpl(context, {}).status(); } - - return read_status; } StatusOr CurlImpl::ReadImpl(RestContext& context, diff --git a/google/cloud/internal/curl_impl.h b/google/cloud/internal/curl_impl.h index 59added888618..ce9c8a18a994b 100644 --- a/google/cloud/internal/curl_impl.h +++ b/google/cloud/internal/curl_impl.h @@ -107,6 +107,7 @@ class CurlImpl { std::size_t HeaderCallback(absl::Span response); private: + class ReadFunctionAbortGuard; Status MakeRequestImpl(RestContext& context); StatusOr ReadImpl(RestContext& context, absl::Span output); From dfb996a77c9aa50efab18136053db419e9a44dd9 Mon Sep 17 00:00:00 2001 From: Scott Hart Date: Wed, 7 Aug 2024 11:27:51 -0400 Subject: [PATCH 2/3] fix logic inversion --- google/cloud/internal/curl_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google/cloud/internal/curl_impl.cc b/google/cloud/internal/curl_impl.cc index aecfd56f2e9a1..7767a5839dc37 100644 --- a/google/cloud/internal/curl_impl.cc +++ b/google/cloud/internal/curl_impl.cc @@ -493,7 +493,7 @@ class CurlImpl::ReadFunctionAbortGuard { ~ReadFunctionAbortGuard() { // If curl_closed_ is true, then the handle has already been recycled and // attempting to set an option on it will error. - if (impl_.curl_closed_) { + if (!impl_.curl_closed_) { impl_.handle_.SetOptionUnchecked(CURLOPT_READFUNCTION, &ReadFunctionAbort); } From 3c2a0b554a2b5690c50e4738a34c0db315859895 Mon Sep 17 00:00:00 2001 From: Scott Hart Date: Wed, 7 Aug 2024 16:26:03 -0400 Subject: [PATCH 3/3] remove scope; more comments --- google/cloud/internal/curl_impl.cc | 36 ++++++++++++++---------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/google/cloud/internal/curl_impl.cc b/google/cloud/internal/curl_impl.cc index 7767a5839dc37..b1db29c57ad9b 100644 --- a/google/cloud/internal/curl_impl.cc +++ b/google/cloud/internal/curl_impl.cc @@ -527,27 +527,25 @@ Status CurlImpl::MakeRequestImpl(RestContext& context) { // All data in the WriteVector should be written after ReadImpl returns unless // an error, typically a timeout, has occurred. Use ReadFunctionAbortGuard to - // instruct curl to not attempt to send anymore data on this handle regardless - // if an error or exception is encountered. - { - ReadFunctionAbortGuard guard(*this); - auto error = curl_multi_add_handle(multi_.get(), handle_.handle_.get()); - - // This indicates that we are using the API incorrectly. The application - // can not recover from these problems, so terminating is the right thing - // to do. - if (error != CURLM_OK) { - GCP_LOG(FATAL) << ", status=" << AsStatus(error, __func__); - } - - in_multi_ = true; + // leverage RAII to instruct curl to not attempt to send anymore data on this + // handle regardless if an error or exception is encountered. + ReadFunctionAbortGuard guard(*this); + auto error = curl_multi_add_handle(multi_.get(), handle_.handle_.get()); - // This call to Read() should send the request, get the response, and - // thus make available the status_code and headers. Any response data - // should be put into the spill buffer, which makes them available for - // subsequent calls to Read() after the headers have been extracted. - return ReadImpl(context, {}).status(); + // This indicates that we are using the API incorrectly. The application + // can not recover from these problems, so terminating is the right thing + // to do. + if (error != CURLM_OK) { + GCP_LOG(FATAL) << ", status=" << AsStatus(error, __func__); } + + in_multi_ = true; + + // This call to Read() should send the request, get the response, and + // thus make available the status_code and headers. Any response data + // should be put into the spill buffer, which makes them available for + // subsequent calls to Read() after the headers have been extracted. + return ReadImpl(context, {}).status(); } StatusOr CurlImpl::ReadImpl(RestContext& context,