AU: MultiHttpFetcher cleanup/rewrite

multi_range_http_fetcher.cc

Index: multi_range_http_fetcher.cc
diff --git a/multi_range_http_fetcher.cc b/multi_range_http_fetcher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0a1650294b86ad5e5c26fba92eb0414bc344569f
--- /dev/null
+++ b/multi_range_http_fetcher.cc
@@ -0,0 +1,143 @@
+// Copyright (c) 2010 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/multi_range_http_fetcher.h"
+
+namespace chromeos_update_engine {
+
+// Begins the transfer to the specified URL.
+// State change: Stopped -> Downloading
+// (corner case: Stopped -> Stopped for an empty request)
+void MultiRangeHTTPFetcher::BeginTransfer(const std::string& url) {
+  LOG_IF(ERROR, base_fetcher_active_) << "BeginTransfer but already active.";

[petkov, 2010/12/16 18:30:57]

These conditions would be pretty bad if they actually occurred. Should we just
CHECK/crash instead?

[adlr, 2010/12/17 01:03:51]

Done.

+  LOG_IF(ERROR, pending_transfer_ended_) << "BeginTransfer but pending.";
+  LOG_IF(ERROR, terminating_) << "BeginTransfer but terminating.";
+
+  if (ranges_.empty()) {
+    // Note that after the callback returns this object may be destroyed.
+    if (delegate_)
+      delegate_->TransferComplete(this, true);
+    return;
+  }
+  url_ = url;
+  current_index_ = 0;
+  bytes_received_this_fetcher_ = 0;
+  LOG(INFO) << "starting first transfer";
+  base_fetcher_->set_delegate(this);
+  StartTransfer();
+}
+
+// Stage change: Downloading -> Pending transfer ended

[petkov, 2010/12/16 18:30:57]

State

[adlr, 2010/12/17 01:03:51]

done.

Copy paste FAIL :(

+void MultiRangeHTTPFetcher::TerminateTransfer() {
+  if (!base_fetcher_active_) {
+    LOG(INFO) << "Called TerminateTransfer but not active.";
+    return;

[petkov, 2010/12/16 18:30:57]

So, if the client invokes TerminateTransfer before BeginTransfer, the client
won't get the TransferTerminated callback?

[adlr, 2010/12/17 01:03:51]

I guess it's a bit of a corner case. I don't know why a client would call
TerminateTransfer on a non-started fetcher. Changed to call it.

+  }
+  terminating_ = true;
+    
+  if (!pending_transfer_ended_) {
+    base_fetcher_->TerminateTransfer();
+    return;

[petkov, 2010/12/16 18:30:57]

no need for this return here.

[adlr, 2010/12/17 01:03:51]

Done.

+  }
+}
+
+// Stage change: Stopped or Downloading -> Downloading
+void MultiRangeHTTPFetcher::StartTransfer() {
+  if (current_index_ >= ranges_.size()) {
+    return;
+  }
+  LOG(INFO) << "Starting a transfer @" << ranges_[current_index_].first << "("
+            << ranges_[current_index_].second << ")";
+  bytes_received_this_fetcher_ = 0;
+  base_fetcher_->SetOffset(ranges_[current_index_].first);
+  if (delegate_)
+    delegate_->SeekToOffset(ranges_[current_index_].first);
+  base_fetcher_active_ = true;
+  base_fetcher_->BeginTransfer(url_);
+}
+
+// Stage change: Downloading -> Downloading
+void MultiRangeHTTPFetcher::ReceivedBytes(HttpFetcher* fetcher,
+                                          const char* bytes,
+                                          int length) {
+  TEST_AND_RETURN(current_index_ < ranges_.size());

[petkov, 2010/12/16 18:30:57]

Again, we may as well use CHECKs here -- it might help us catch bugs in the
field. And it seems that if the conditions don't hold, something pretty bad is
going on.

[adlr, 2010/12/17 01:03:51]

Done.

+  TEST_AND_RETURN(fetcher == base_fetcher_);
+  TEST_AND_RETURN(!pending_transfer_ended_);
+  off_t next_size = length;
+  if (ranges_[current_index_].second >= 0) {
+    next_size = std::min(next_size,
+                         ranges_[current_index_].second -
+                         bytes_received_this_fetcher_);
+  }
+  LOG_IF(WARNING, next_size <= 0) << "Asked to write length <= 0";
+  if (delegate_) {
+    delegate_->ReceivedBytes(this, bytes, next_size);
+  }
+  bytes_received_this_fetcher_ += length;
+  if (ranges_[current_index_].second >= 0 &&
+      bytes_received_this_fetcher_ >= ranges_[current_index_].second) {
+    // Terminates the current fetcher. Waits for its TransferTerminated
+    // callback before starting the next fetcher so that we don't end up
+    // signalling the delegate that the whole multi-transfer is complete
+    // before all fetchers are really done and cleaned up.
+    pending_transfer_ended_ = true;
+    LOG(INFO) << "terminating transfer";
+    fetcher->TerminateTransfer();
+  }
+}
+
+// Stage change: Downloading or Pending transfer ended -> Stopped
+void MultiRangeHTTPFetcher::TransferEnded(HttpFetcher* fetcher,
+                                          bool successful) {
+  LOG_IF(ERROR, !base_fetcher_active_) << "Transfer ended unexpectedly.";
+  LOG_IF(ERROR, !pending_transfer_ended_)

[petkov, 2010/12/16 18:30:57]

What happens when the last transfer completes, the one with unlimited size? I
think pending_transfer_ended_ would be false in that case but you'd issue an
error in the log.

[adlr, 2010/12/17 01:03:51]

Good catch. Yes, this was in the logs. Removed it and turned the other one into
a CHECK

+      << "Unexpectedly not waiting for transfer to end.";
+  pending_transfer_ended_ = false;
+  http_response_code_ = fetcher->http_response_code();
+  LOG(INFO) << "TransferEnded w/ code " << http_response_code_;
+  if (terminating_) {
+    LOG(INFO) << "Terminating.";
+    Reset();
+    // Note that after the callback returns this object may be destroyed.
+    if (delegate_)
+      delegate_->TransferTerminated(this);
+    return;
+  }
+
+  // If we didn't get enough bytes, it's failure
+  if (ranges_[current_index_].second >= 0) {
+    if (bytes_received_this_fetcher_ < ranges_[current_index_].second) {
+      // Failure
+      LOG(INFO) << "Didn't get enough bytes. Ending w/ failure.";
+      Reset();
+      // Note that after the callback returns this object may be destroyed.
+      if (delegate_)
+        delegate_->TransferComplete(this, false);
+      return;
+    }
+    // We got enough bytes and there were bytes specified, so this is success.
+    successful = true;
+  }
+
+  // If we have another fetcher, use that.
+  if (current_index_ + 1 < ranges_.size()) {
+    LOG(INFO) << "Starting next transfer.";
+    current_index_++;
+    StartTransfer();
+    return;
+  }
+
+  // LOG(INFO) << "passed in success: " << successful;

[petkov, 2010/12/16 18:30:57]

remove commented code

[adlr, 2010/12/17 01:03:51]

Done.

+  // if (ranges_[current_index_].second >= 0) {
+  //   successful = bytes_received_this_fetcher_ >= ranges_[current_index_].second;
+  // }
+  // LOG(INFO) << "computed success: " << successful;
+  LOG(INFO) << "Done w/ all transfers";
+  Reset();
+  // Note that after the callback returns this object may be destroyed.
+  if (delegate_)
+    delegate_->TransferComplete(this, successful);
+}
+
+}  // namespace chromeos_update_engine