fix: release CurlRequest handles on error

coryan · coryan · commit a95ddf0ac2b2 · 2021-08-31T22:38:13.000Z
diff --git a/google/cloud/storage/internal/curl_request.cc b/google/cloud/storage/internal/curl_request.cc
@@ -64,6 +64,10 @@ extern "C" std::size_t CurlRequestOnReadData(char* ptr, std::size_t size,
   return v->OnRead(ptr, size, nitems);
 }
 
+CurlRequest::~CurlRequest() {
+  if (factory_) factory_->CleanupHandle(std::move(handle_));
+}
+
 StatusOr<HttpResponse> CurlRequest::MakeRequest(std::string const& payload) {
   handle_.SetOption(CURLOPT_UPLOAD, 0L);
   if (!payload.empty()) {
@@ -90,6 +94,14 @@ StatusOr<HttpResponse> CurlRequest::MakeUploadRequest(
   return MakeRequestImpl();
 }
 
+Status CurlRequest::OnError(Status status) {
+  // When there is a transfer error the handle is suspect. It could be pointing
+  // to an invalid host, a host that is slow and trickling data, or otherwise in
+  // a bad state. Release the handle, but do not return it to the pool.
+  auto handle = std::move(handle_);
+  return status;
+}
+
 StatusOr<HttpResponse> CurlRequest::MakeRequestImpl() {
   // We get better performance using a slightly larger buffer (128KiB) than the
   // default buffer size set by libcurl (16KiB)
@@ -111,7 +123,7 @@ StatusOr<HttpResponse> CurlRequest::MakeRequestImpl() {
   handle_.SetOption(CURLOPT_HEADERFUNCTION, &CurlRequestOnHeaderData);
   handle_.SetOption(CURLOPT_HEADERDATA, this);
   auto status = handle_.EasyPerform();
-  if (!status.ok()) return status;
+  if (!status.ok()) return OnError(std::move(status));
 
   if (logging_enabled_) handle_.FlushDebug(__func__);
   auto code = handle_.GetResponseCode();
diff --git a/google/cloud/storage/internal/curl_request.h b/google/cloud/storage/internal/curl_request.h
@@ -35,9 +35,7 @@ extern "C" size_t CurlRequestOnHeaderData(char* contents, size_t size,
 class CurlRequest {
  public:
   CurlRequest() = default;
-  ~CurlRequest() {
-    if (factory_) factory_->CleanupHandle(std::move(handle_));
-  }
+  ~CurlRequest();
 
   CurlRequest(CurlRequest&&) = default;
   CurlRequest& operator=(CurlRequest&&) = default;
@@ -55,6 +53,9 @@ class CurlRequest {
   StatusOr<HttpResponse> MakeUploadRequest(ConstBufferSequence payload);
 
  private:
+  /// Handle a libcurl error during the request.
+  Status OnError(Status status);
+
   StatusOr<HttpResponse> MakeRequestImpl();
 
   friend class CurlRequestBuilder;
diff --git a/google/cloud/storage/tests/curl_download_request_integration_test.cc b/google/cloud/storage/tests/curl_download_request_integration_test.cc
@@ -226,7 +226,7 @@ TEST(CurlDownloadRequestTest, HandlesReleasedOnError) {
 
   CurlRequestBuilder request("https://localhost:1/get", factory);
   auto download = std::move(request).BuildDownloadRequest();
-  // Perform a series of very small `.Read()` calls. This will force the
+  // This `.Read()` call fails as the endpoint is invalid.
   char buffer[4096];
   auto read = download->Read(buffer, sizeof(buffer));
   ASSERT_THAT(read, Not(IsOk()));
diff --git a/google/cloud/storage/tests/curl_request_integration_test.cc b/google/cloud/storage/tests/curl_request_integration_test.cc
@@ -565,6 +565,34 @@ TEST(CurlRequestTest, Logging) {
   EXPECT_THAT(log_messages, HasSubstr("curl(Recv Data)"));
 }
 
+TEST(CurlRequestTest, HandlesReleasedOnError) {
+  auto constexpr kTestPoolSize = 8;
+  auto factory =
+      std::make_shared<PooledCurlHandleFactory>(kTestPoolSize, Options{});
+  ASSERT_EQ(0, factory->CurrentHandleCount());
+  ASSERT_EQ(0, factory->CurrentMultiHandleCount());
+
+  CurlRequestBuilder builder("https://localhost:1/get", factory);
+  auto response = std::move(builder).BuildRequest().MakeRequest(std::string{});
+  ASSERT_THAT(response, Not(IsOk()));
+  // Assuming there was an error the CURL* handle should not be returned to the
+  // pool.
+  EXPECT_EQ(0, factory->CurrentHandleCount());
+}
+
+TEST(CurlRequestTest, HandlesReusedOnSuccess) {
+  auto constexpr kTestPoolSize = 8;
+  auto factory =
+      std::make_shared<PooledCurlHandleFactory>(kTestPoolSize, Options{});
+  ASSERT_EQ(0, factory->CurrentHandleCount());
+  ASSERT_EQ(0, factory->CurrentMultiHandleCount());
+
+  CurlRequestBuilder builder(HttpBinEndpoint() + "/get", factory);
+  auto response = std::move(builder).BuildRequest().MakeRequest(std::string{});
+  ASSERT_THAT(response, IsOk());
+  EXPECT_EQ(1, factory->CurrentHandleCount());
+}
+
 }  // namespace
 }  // namespace internal
 }  // namespace STORAGE_CLIENT_NS
