[Downloads] Retry renames after transient failures.

content/browser/download/download_file_impl.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 "content/browser/download/download_file_impl.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "content/browser/byte_stream.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_interrupt_reasons_impl.h"
 #include "content/browser/download/download_net_log_parameters.h"
 #include "content/browser/download/download_stats.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_destination_observer.h"
 #include "net/base/io_buffer.h"
 
 namespace content {
 
 const int kUpdatePeriodMs = 500;
 const int kMaxTimeBlockingFileThreadMs = 1000;
 
+// These constants control the default retry behavior for failing renames. Each
+// retry is performed after a delay that is twice the previous delay. The
+// initial delay is specified by kInitialRenameRetryDelayMs.
+const int kMaxRenameRetries = 3;
+const int kInitialRenameRetryDelayMs = 200;
+
 int DownloadFile::number_active_objects_ = 0;
 
 DownloadFileImpl::DownloadFileImpl(
     scoped_ptr<DownloadSaveInfo> save_info,
     const base::FilePath& default_download_directory,
     const GURL& url,
     const GURL& referrer_url,
     bool calculate_hash,
     scoped_ptr<ByteStreamReader> stream,
     const net::BoundNetLog& bound_net_log,
     base::WeakPtr<DownloadDestinationObserver> observer)
-        : file_(save_info->file_path,
-                url,
-                referrer_url,
-                save_info->offset,
-                calculate_hash,
-                save_info->hash_state,
-                save_info->file.Pass(),
-                bound_net_log),
-          default_download_directory_(default_download_directory),
-          stream_reader_(stream.Pass()),
-          bytes_seen_(0),
-          bound_net_log_(bound_net_log),
-          observer_(observer),
-          weak_factory_(this) {
+    : file_(save_info->file_path,
+            url,
+            referrer_url,
+            save_info->offset,
+            calculate_hash,
+            save_info->hash_state,
+            save_info->file.Pass(),
+            bound_net_log),
+      default_download_directory_(default_download_directory),
+      stream_reader_(stream.Pass()),
+      bytes_seen_(0),
+      bound_net_log_(bound_net_log),
+      observer_(observer),
+      weak_factory_(this) {
 }
 
 DownloadFileImpl::~DownloadFileImpl() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   --number_active_objects_;
 }
 
 void DownloadFileImpl::Initialize(const InitializeCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
@@ skipping 30 matching lines @@
 
   if (!update_timer_->IsRunning()) {
     update_timer_->Start(FROM_HERE,
                          base::TimeDelta::FromMilliseconds(kUpdatePeriodMs),
                          this, &DownloadFileImpl::SendUpdate);
   }
   rate_estimator_.Increment(data_len);
   return file_.AppendDataToFile(data, data_len);
 }
 
 void DownloadFileImpl::RenameAndUniquify(

[Randy Smith (Not in Mondays), 2014/06/11 18:07:29]

[Not for this CL]  

Why are we doing a RenameAndUniquify now that the DownloadTargetDeterminer takes
care of all that?  If it's just to handle races with external file creation,
that seems fishy to me--doesn't that result in our having a reservation on a
name that isn't the one we actually use?

[asanka, 2014/06/12 20:02:37]

RenameAndUniquify is invoked for the intermediate file whose exact filename we
don't care too much about. The final rename is done using RenameAndAnnotate
which overwrites if there are any conflicts.

     const base::FilePath& full_path,
     const RenameCompletionCallback& callback) {
+  RenameWithRetryInternal(
+      full_path, UNIQUIFY, kMaxRenameRetries, base::TimeTicks(), callback);
+}
+
+void DownloadFileImpl::RenameAndAnnotate(
+    const base::FilePath& full_path,
+    const RenameCompletionCallback& callback) {
+  RenameWithRetryInternal(full_path,
+                          ANNOTATE_WITH_SOURCE_INFORMATION,
+                          kMaxRenameRetries,
+                          base::TimeTicks(),
+                          callback);
+}
+
+base::TimeDelta DownloadFileImpl::GetRetryDelayForFailedRename(
+    int attempt_number) {
+  DCHECK_GE(attempt_number, 0);
+  // |delay| starts at kInitialRenameRetryDelayMs and increases by a factor of
+  // 2 at each subsequent retry. Assumes that |retries_left| starts at
+  // kMaxRenameRetries. Also assumes that kMaxRenameRetries is less than the
+  // number of bits in an int.
+  return base::TimeDelta::FromMilliseconds(kInitialRenameRetryDelayMs) *
+         (1 << attempt_number);
+}
+
+bool DownloadFileImpl::ShouldRetryFailedRename(DownloadInterruptReason reason) {
+  return reason == DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR;
+}
+
+void DownloadFileImpl::RenameWithRetryInternal(
+    const base::FilePath& full_path,
+    RenameOption option,
+    int retries_left,
+    base::TimeTicks time_of_first_failure,
+    const RenameCompletionCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   base::FilePath new_path(full_path);
 
-  int uniquifier = base::GetUniquePathNumber(
-      new_path, base::FilePath::StringType());
-  if (uniquifier > 0) {
-    new_path = new_path.InsertBeforeExtensionASCII(
-        base::StringPrintf(" (%d)", uniquifier));
+  if ((option & UNIQUIFY) && full_path != file_.full_path()) {
+    int uniquifier =
+        base::GetUniquePathNumber(new_path, base::FilePath::StringType());
+    if (uniquifier > 0)
+      new_path = new_path.InsertBeforeExtensionASCII(
+          base::StringPrintf(" (%d)", uniquifier));
   }
 
   DownloadInterruptReason reason = file_.Rename(new_path);
-  if (reason != DOWNLOAD_INTERRUPT_REASON_NONE) {
-    // Make sure our information is updated, since we're about to
-    // error out.
-    SendUpdate();
 
-    // Null out callback so that we don't do any more stream processing.
-    stream_reader_->RegisterCallback(base::Closure());
-
-    new_path.clear();
+  // Attempt to retry the rename if possible. If the rename failed and the
+  // subsequent open also failed, then in_progress() would be false. We don't
+  // try to retry renames if the in_progress() was false to begin with since we
+  // have less assurance that the file at file_.full_path() was the one we were
+  // working with.
+  if (ShouldRetryFailedRename(reason) && file_.in_progress() &&
+      retries_left > 0) {
+    int attempt_number = kMaxRenameRetries - retries_left;
+    BrowserThread::PostDelayedTask(
+        BrowserThread::FILE,
+        FROM_HERE,
+        base::Bind(&DownloadFileImpl::RenameWithRetryInternal,
+                   weak_factory_.GetWeakPtr(),
+                   full_path,
+                   option,
+                   --retries_left,
+                   time_of_first_failure.is_null() ? base::TimeTicks::Now()
+                                                   : time_of_first_failure,
+                   callback),
+        GetRetryDelayForFailedRename(attempt_number));
+    return;
   }
 
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(callback, reason, new_path));
-}
+  if (!time_of_first_failure.is_null())
+    RecordDownloadFileRenameResultAfterRetry(
+        base::TimeTicks::Now() - time_of_first_failure, reason);
 
-void DownloadFileImpl::RenameAndAnnotate(
-    const base::FilePath& full_path,
-    const RenameCompletionCallback& callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
-  base::FilePath new_path(full_path);
-
-  DownloadInterruptReason reason = DOWNLOAD_INTERRUPT_REASON_NONE;
-  // Short circuit null rename.
-  if (full_path != file_.full_path())
-    reason = file_.Rename(new_path);
-
-  if (reason == DOWNLOAD_INTERRUPT_REASON_NONE) {
+  if (reason == DOWNLOAD_INTERRUPT_REASON_NONE &&
+      (option & ANNOTATE_WITH_SOURCE_INFORMATION)) {
     // Doing the annotation after the rename rather than before leaves
     // a very small window during which the file has the final name but
     // hasn't been marked with the Mark Of The Web.  However, it allows
     // anti-virus scanners on Windows to actually see the data
     // (http://crbug.com/127999) under the correct name (which is information
     // it uses).
     reason = file_.AnnotateWithSourceInformation();
   }
 
   if (reason != DOWNLOAD_INTERRUPT_REASON_NONE) {
@@ skipping 152 matching lines @@
                  observer_, file_.bytes_so_far(), CurrentSpeed(),
                  GetHashState()));
 }
 
 // static
 int DownloadFile::GetNumberOfDownloadFiles() {
   return number_active_objects_;
 }
 
 }  // namespace content