contacts: Give up after repeated transient photo errors.

chrome/browser/chromeos/gdata/gdata_contacts_service.cc

 // Copyright (c) 2012 The Chromium 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 "chrome/browser/chromeos/gdata/gdata_contacts_service.h"
 
 #include <cstring>
 #include <string>
 #include <map>
 #include <utility>
@@ skipping 16 matching lines @@
 using content::BrowserThread;
 
 namespace gdata {
 
 namespace {
 
 // Maximum number of profile photos that we'll download per second.
 // 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";
 
 // Top-level field in a contact groups feed containing the list of entries.
 const char kGroupEntryField[] = "feed.entry";
 
 // Field in group entries containing the system group ID (e.g. ID "Contacts"
 // for the "My Contacts" system group).  See
 // https://developers.google.com/google-apps/contacts/v3/#contact_group_entry
@@ skipping 607 matching lines @@
   void HandlePhotoData(contacts::Contact* contact,
                        GDataErrorCode error,
                        scoped_ptr<std::string> download_data) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     VLOG(1) << "Got photo data for " << contact->provider_id()
             << " (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;
+      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) {
       LOG(WARNING) << "Got error " << error << " while downloading photo "
                    << "for " << contact->provider_id() << "; skipping";
       CheckCompletion();
       return;
     }
 
     if (error != HTTP_SUCCESS) {
@@ skipping 32 matching lines @@
   // 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_;
 
   // 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_;
 
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate its weak pointers before any other members are destroyed.
   base::WeakPtrFactory<DownloadContactsRequest> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadContactsRequest);
 };
 
@@ skipping 39 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(request);
   VLOG(1) << "Download request " << request << " complete";
   if (!request->my_contacts_group_id().empty())
     cached_my_contacts_group_id_ = request->my_contacts_group_id();
   requests_.erase(request);
   delete request;
 }
 
 }  // namespace contacts