AU: When server dies, don't retry forever

libcurl_http_fetcher.cc

 // Copyright (c) 2009 The Chromium OS Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "update_engine/libcurl_http_fetcher.h"
 #include <algorithm>
 #include "chromeos/obsolete_logging.h"
 
 using std::max;
 using std::make_pair;
 
 // This is a concrete implementation of HttpFetcher that uses libcurl to do the
 // http work.
 
 namespace chromeos_update_engine {
 
+namespace {
+const int kMaxRetriesCount = 20;
+}
+
 LibcurlHttpFetcher::~LibcurlHttpFetcher() {
   CleanUp();
 }
 
 void LibcurlHttpFetcher::ResumeTransfer(const std::string& url) {
   LOG(INFO) << "Starting/Resuming transfer";
   CHECK(!transfer_in_progress_);
   url_ = url;
   curl_multi_handle_ = curl_multi_init();
   CHECK(curl_multi_handle_);
@@ skipping 32 matching lines @@
 
   CHECK_EQ(curl_multi_add_handle(curl_multi_handle_, curl_handle_), CURLM_OK);
   transfer_in_progress_ = true;
 }
 
 // Begins the transfer, which must not have already been started.
 void LibcurlHttpFetcher::BeginTransfer(const std::string& url) {
   transfer_size_ = -1;
   bytes_downloaded_ = 0;
   resume_offset_ = 0;
-  do {
-    ResumeTransfer(url);
-  } while (CurlPerformOnce());
+  retry_count_ = 0;
+  ResumeTransfer(url);
+  CurlPerformOnce();
 }
 
 void LibcurlHttpFetcher::TerminateTransfer() {
   CleanUp();
 }
 
 bool LibcurlHttpFetcher::CurlPerformOnce() {

[petkov, 2010/07/16 21:47:45]

You could make this method void.

   CHECK(transfer_in_progress_);
   int running_handles = 0;
   CURLMcode retcode = CURLM_CALL_MULTI_PERFORM;
 
   // libcurl may request that we immediately call curl_multi_perform after it
   // returns, so we do. libcurl promises that curl_multi_perform will not block.
   while (CURLM_CALL_MULTI_PERFORM == retcode) {
     retcode = curl_multi_perform(curl_multi_handle_, &running_handles);
   }
   if (0 == running_handles) {
+    long http_response_code = 0;
+    if (curl_easy_getinfo(curl_handle_,

[petkov, 2010/07/16 21:47:45]

This is definitely better than the CHECK_EQ...

+                          CURLINFO_RESPONSE_CODE,
+                          &http_response_code) == CURLE_OK) {
+      LOG(INFO) << "HTTP response code: " << http_response_code;
+    } else {
+      LOG(ERROR) << "Unable to get http response code.";
+    }
+    
     // we're done!
     CleanUp();
 
     if ((transfer_size_ >= 0) && (bytes_downloaded_ < transfer_size_)) {
       // Need to restart transfer
-      return true;
+      retry_count_++;
+      LOG(INFO) << "Restarting transfer b/c we finished, had downloaded "
+                << bytes_downloaded_ << " bytes, but transfer_size_ is "
+                << transfer_size_ << ". retry_count: " << retry_count_;
+      if (retry_count_ > kMaxRetriesCount) {
+        if (delegate_)
+          delegate_->TransferComplete(this, false);  // success
+      } else {
+        g_timeout_add(5 * 1000,
+                      &LibcurlHttpFetcher::StaticRetryTimeoutCallback,
+                      this);
+      }
+      return false;
     } else {
       if (delegate_) {
-        delegate_->TransferComplete(this, true);  // success
+        // success is when http_response_code is 200
+        delegate_->TransferComplete(this, http_response_code == 200);
       }
     }
   } else {
     // set up callback
     SetupMainloopSources();
   }
   return false;
 }
 
 size_t LibcurlHttpFetcher::LibcurlWrite(void *ptr, size_t size, size_t nmemb) {
@@ skipping 102 matching lines @@
     static int counter = 0;
     counter++;
     if (counter % 50 == 0) {
       LOG(INFO) << "counter = " << counter;
     }
   }
 }
 
 bool LibcurlHttpFetcher::FDCallback(GIOChannel *source,
                                     GIOCondition condition) {
-  while (CurlPerformOnce()) {
-    ResumeTransfer(url_);
-  }
+  CurlPerformOnce();
   // We handle removing of this source elsewhere, so we always return true.
   // The docs say, "the function should return FALSE if the event source
   // should be removed."
   // http://www.gtk.org/api/2.6/glib/glib-IO-Channels.html#GIOFunc
   return true;
 }
 
+gboolean LibcurlHttpFetcher::RetryTimeoutCallback() {
+  ResumeTransfer(url_);
+  CurlPerformOnce();
+  return FALSE;  // Don't have glib auto call this callback again
+}
+
 gboolean LibcurlHttpFetcher::TimeoutCallback() {
   if (!transfer_in_progress_)
     return TRUE;
   // Since we will return false from this function, which tells glib to

[petkov, 2010/07/16 21:47:45]

Actually, it seems we're returning TRUE from this function? Is this a bug? I
guess this is not part of this CL...

   // destroy the timeout callback, we must NULL it out here. This way, when
   // setting up callback sources again, we won't try to delete this (doomed)
   // timeout callback then.
   // TODO(adlr): optimize by checking if we can keep this timeout callback.
   //timeout_source_ = NULL;
-  while (CurlPerformOnce()) {
-    ResumeTransfer(url_);
-  }
+  CurlPerformOnce();
   return TRUE;
 }
 
 void LibcurlHttpFetcher::CleanUp() {
   if (timeout_source_) {
     g_source_destroy(timeout_source_);
     timeout_source_ = NULL;
   }
 
   for (IOChannels::iterator it = io_channels_.begin();
@@ skipping 12 matching lines @@
     curl_handle_ = NULL;
   }
   if (curl_multi_handle_) {
     CHECK_EQ(curl_multi_cleanup(curl_multi_handle_), CURLM_OK);
     curl_multi_handle_ = NULL;
   }
   transfer_in_progress_ = false;
 }
 
 }  // namespace chromeos_update_engine