contacts: Rate-limit GData photo requests and handle 404s.

chrome/browser/chromeos/gdata/gdata_contacts_service.cc

Index: chrome/browser/chromeos/gdata/gdata_contacts_service.cc
diff --git a/chrome/browser/chromeos/gdata/gdata_contacts_service.cc b/chrome/browser/chromeos/gdata/gdata_contacts_service.cc
index 16f49ad4ff536ad77199447c6f3191342153086a..4799189d4c138fc00c8d736bbaad5ba9cc0d0a90 100644
--- a/chrome/browser/chromeos/gdata/gdata_contacts_service.cc
+++ b/chrome/browser/chromeos/gdata/gdata_contacts_service.cc
@@ -13,6 +13,8 @@
 #include "base/logging.h"
 #include "base/memory/weak_ptr.h"
 #include "base/stl_util.h"
+#include "base/time.h"
+#include "base/timer.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/contacts/contact.pb.h"
 #include "chrome/browser/chromeos/gdata/gdata_operation_registry.h"
@@ -29,8 +31,9 @@ namespace gdata {
 
 namespace {
 
-// Maximum number of profile photos that we'll download at once.
-const int kMaxSimultaneousPhotoDownloads = 10;
+// Maximum number of profile photos that we'll download per second.
+// At values above 10, Google starts returning 503 errors.
+const int kMaxPhotoDownloadsPerSecond = 10;
 
 // Field in the top-level object containing the contacts feed.
 const char kFeedField[] = "feed";
@@ -322,7 +325,7 @@ class GDataContactsService::DownloadContactsRequest
                           SuccessCallback success_callback,
                           FailureCallback failure_callback,
                           const base::Time& min_update_time,
-                          int max_simultaneous_photo_downloads)
+                          int max_photo_downloads_per_second)
       : service_(service),
         profile_(profile),
         runner_(runner),
@@ -330,7 +333,7 @@ class GDataContactsService::DownloadContactsRequest
         failure_callback_(failure_callback),
         min_update_time_(min_update_time),
         contacts_(new ScopedVector<contacts::Contact>),
-        max_simultaneous_photo_downloads_(max_simultaneous_photo_downloads),
+        max_photo_downloads_per_second_(max_photo_downloads_per_second),
         num_in_progress_photo_downloads_(0),
         photo_download_failed_(false) {
     DCHECK(service_);
@@ -380,6 +383,10 @@ class GDataContactsService::DownloadContactsRequest
       return;
     }
 
+    StartPhotoDownloads();
+    photo_download_timer_.Start(
+        FROM_HERE, base::TimeDelta::FromSeconds(1),
+        this, &DownloadContactsRequest::StartPhotoDownloads);
     CheckCompletion();
   }
 
@@ -473,6 +480,7 @@ class GDataContactsService::DownloadContactsRequest
     if (contacts_needing_photo_downloads_.empty() &&
         num_in_progress_photo_downloads_ == 0) {
       VLOG(1) << "Done downloading photos; invoking callback";
+      photo_download_timer_.Stop();
       if (photo_download_failed_)
         failure_callback_.Run();
       else
@@ -480,10 +488,14 @@ class GDataContactsService::DownloadContactsRequest
       service_->OnRequestComplete(this);
       return;
     }
+  }
 
+  // Starts photo downloads for contacts in |contacts_needing_photo_downloads_|.
+  // Should be invoked only once per second.
+  void StartPhotoDownloads() {
     while (!contacts_needing_photo_downloads_.empty() &&
            (num_in_progress_photo_downloads_ <
-            max_simultaneous_photo_downloads_)) {
+            max_photo_downloads_per_second_)) {
       contacts::Contact* contact = contacts_needing_photo_downloads_.back();
       contacts_needing_photo_downloads_.pop_back();
       DCHECK(contact_photo_urls_.count(contact));
@@ -512,10 +524,24 @@ class GDataContactsService::DownloadContactsRequest
             << " (error=" << error << " size=" << download_data->size() << ")";
     num_in_progress_photo_downloads_--;
 
+    if (error == HTTP_INTERNAL_SERVER_ERROR ||
+        error == HTTP_SERVICE_UNAVAILABLE) {
+      LOG(WARNING) << "Got error " << error << " while downloading photo "
+                   << "for " << contact->provider_id() << "; retrying";
+      contacts_needing_photo_downloads_.push_back(contact);
+      return;
+    }
+
+    if (error == HTTP_NOT_FOUND) {
+      LOG(WARNING) << "Got error " << error << " while downloading photo "
+                   << "for " << contact->provider_id() << "; skipping";
+      CheckCompletion();
+      return;
+    }
+
     if (error != HTTP_SUCCESS) {
       LOG(WARNING) << "Got error " << error << " while downloading photo "
-                   << "for " << contact->provider_id();
-      // TODO(derat): Retry several times for temporary failures?
+                   << "for " << contact->provider_id() << "; giving up";

[satorux1, 2012/08/06 20:57:08]

don't have to do it in the patch, but you might want to add some UMA for
failures like this.

[Daniel Erat, 2012/08/06 23:12:21]

Yep, I plan to add a lot of metrics in a later patch for temporary and permanent
failures (along with histograms for things like total number of contacts and
time taken to download them).

       photo_download_failed_ = true;
       // Make sure we don't start any more downloads.
       contacts_needing_photo_downloads_.clear();
@@ -545,12 +571,15 @@ class GDataContactsService::DownloadContactsRequest
   // Contacts without photos do not appear in this map.
   ContactPhotoUrls contact_photo_urls_;
 
+  // Invokes StartPhotoDownloads() once per second.
+  base::RepeatingTimer<DownloadContactsRequest> photo_download_timer_;
+
   // Contacts that have photos that we still need to start downloading.
   // When we start a download, the contact is removed from this list.
   std::vector<contacts::Contact*> contacts_needing_photo_downloads_;
 
-  // Maximum number of photos we'll try to download at once.
-  int max_simultaneous_photo_downloads_;
+  // Maximum number of photos we'll try to download per second.
+  int max_photo_downloads_per_second_;
 
   // Number of in-progress photo downloads.
   int num_in_progress_photo_downloads_;
@@ -564,7 +593,7 @@ class GDataContactsService::DownloadContactsRequest
 GDataContactsService::GDataContactsService(Profile* profile)
     : profile_(profile),
       runner_(new GDataOperationRunner(profile)),
-      max_simultaneous_photo_downloads_(kMaxSimultaneousPhotoDownloads) {
+      max_photo_downloads_per_second_(kMaxPhotoDownloadsPerSecond) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(profile_);
 }
@@ -596,7 +625,7 @@ void GDataContactsService::DownloadContacts(SuccessCallback success_callback,
                                   success_callback,
                                   failure_callback,
                                   min_update_time,
-                                  max_simultaneous_photo_downloads_);
+                                  max_photo_downloads_per_second_);
   VLOG(1) << "Starting contacts download with request " << request;
   requests_.insert(request);
   request->Run();