contacts: Give up after repeated transient photo errors.

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 a7db23f4ed1ebb11fda9c0236debd7dcffdfc915..79915aed016a87edc7ab7000be164a565d01da19 100644
--- a/chrome/browser/chromeos/gdata/gdata_contacts_service.cc
+++ b/chrome/browser/chromeos/gdata/gdata_contacts_service.cc
@@ -34,6 +34,10 @@ namespace {
 // At values above 10, Google starts returning 503 errors.
 const int kMaxPhotoDownloadsPerSecond = 10;
 
+// Give up after seeing more than this many transient errors while trying to
+// download a photo for a single contact.
+const int kMaxTransientPhotoDownloadErrorsPerContact = 2;
+
 // Hardcoded system group ID for the "My Contacts" group, per
 // https://developers.google.com/google-apps/contacts/v3/#contact_group_entry.
 const char kMyContactsSystemGroupId[] = "Contacts";
@@ -661,10 +665,13 @@ class GDataContactsService::DownloadContactsRequest {
 
     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;
+      int num_errors = ++transient_photo_download_errors_per_contact_[contact];
+      if (num_errors <= kMaxTransientPhotoDownloadErrorsPerContact) {
+        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) {
@@ -717,6 +724,12 @@ class GDataContactsService::DownloadContactsRequest {
   // Number of in-progress photo downloads.
   int num_in_progress_photo_downloads_;
 
+  // Map from a contact to the number of transient errors that we've encountered
+  // while trying to download its photo.  Contacts for which no errors have been
+  // encountered aren't represented in the map.
+  std::map<contacts::Contact*, int>

[satorux1, 2012/08/17 21:55:21]

I'm slightly concerned about using a key as a pointer. It probably won't happen
here, but with a pointer as a key, the following scenario could happen:

1) an object is created at address 1234 and added to the map
2) the object is deleted, but the map is not updated
3) a different object is crated at address 1234 again. oops.

Maybe using provider_id is slightly better?

BTW, 'privider_id' sounds like the ID of the provider, rather than a
provider-assigned unique ID. You might want to rename it to something like
contact_id.

[Daniel Erat, 2012/08/17 22:05:03]

I'll change this if you really want me to, but none of this object's contacts
are deleted while it's being used, and I prefer using known-width pointers as
keys instead of potentially-very-long URLs assigned by the provider.

I'll rename provider_id to contact_id in a separate change.  I initially wanted
to keep "provider" in the name to imply that these IDs are necessarily unique
across providers, but at least in the case of contacts that we get from Google,
I believe that they are always unique (i.e. the user's account name appers in
the URL).

+      transient_photo_download_errors_per_contact_;
+
   // Did we encounter a fatal error while downloading a photo?
   bool photo_download_failed_;