AU: MultiHttpFetcher cleanup/rewrite

multi_range_http_fetcher.h

+// 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.
+
+#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_MULTI_RANGE_HTTP_FETCHER_H__
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_MULTI_RANGE_HTTP_FETCHER_H__
+
+#include <deque>
+#include <tr1/memory>
+#include <utility>
+#include <vector>
+
+#include <base/scoped_ptr.h>
+
+#include "update_engine/http_fetcher.h"
+#include "update_engine/utils.h"

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

do you need this include here?

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

nope. cleaned up the headers

+
+// This class is a simple wrapper around an HttpFetcher. The client
+// specifies a vector of byte ranges. MultiRangeHTTPFetcher will fetch bytes
+// from those offsets. Pass -1 as a length to specify unlimited length.
+// It really only would make sense for the last range specified to have
+// unlimited length, tho it is legal for other entries to have unlimited
+// length.
+
+// There are three states a MultiRangeHTTPFetcher object will be in:
+// - Stopped (start state)
+// - Downloading
+// - Pending transfer ended
+// Various functions below that might change state indicate possible
+// state changes.
+
+namespace chromeos_update_engine {
+
+class MultiRangeHTTPFetcher : public HttpFetcher, public HttpFetcherDelegate {
+ public:
+  typedef std::vector<std::pair<off_t, off_t> > RangesVect;
+
+  // Takes ownership of the passed in fetcher.
+  explicit MultiRangeHTTPFetcher(HttpFetcher* base_fetcher)
+      : HttpFetcher(base_fetcher->proxy_resolver()),
+        base_fetcher_(base_fetcher),
+        base_fetcher_active_(false),
+        pending_transfer_ended_(false),
+        terminating_(false),
+        current_index_(0),
+        bytes_received_this_fetcher_(0) {}
+  ~MultiRangeHTTPFetcher() {}
+
+  void set_ranges(const RangesVect& ranges) {

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

Fits on one line?

Btw, have you considered providing just an AddRange method that takes two
parameters -- |offset| and |size|? Then you won't need to expose the range
vector and pair types to clients. It might be a bit cleaner interface.

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

interesting idea, done.

+    ranges_ = ranges;
+  }
+
+  void SetOffset(off_t offset) {}  // for now, doesn't support this

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

Per style, all inherited virtual methods should also be declared virtual -- here
and below (BeginTransfer, TerminateTransfer, Pause, Unpause, etc.)

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

Done.

+
+  // Begins the transfer to the specified URL.
+  // State change: Stopped -> Downloading
+  // (corner case: Stopped -> Stopped for an empty request)
+  void BeginTransfer(const std::string& url);
+
+  // Stage change: Downloading -> Pending transfer ended

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

typo: Stage->State

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

Done.

+  void TerminateTransfer();
+
+  void Pause() {

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

Fits on one line?

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

Done.

+    base_fetcher_->Pause();
+  }
+
+  void Unpause() {

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

Fits on one line?

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

Done.

+    base_fetcher_->Unpause();
+  }
+
+  // These functions are overloaded in LibcurlHttp fetcher for testing purposes.
+  void set_idle_seconds(int seconds) {

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

Side note: It's kind of unfortunate that there's no good way to just forward
these methods automatically and implicitly (e.g., like they would be if this
class inherited the base fetcher rather than using composition). I don't think I
have any good suggestions though -- these methods do clutter the interface
though.

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

I agree it's a bit annoying, but I like having the default be that I must
specify what happens rather than being to forward.

+    base_fetcher_->set_idle_seconds(seconds);
+  }
+  void set_retry_seconds(int seconds) {
+    base_fetcher_->set_retry_seconds(seconds);
+  }
+  void SetConnectionAsExpensive(bool is_expensive) {
+    base_fetcher_->SetConnectionAsExpensive(is_expensive);
+  }
+  void SetBuildType(bool is_official) {
+    base_fetcher_->SetBuildType(is_official);
+  }
+  virtual void SetProxies(const std::deque<std::string>& proxies) {

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

Why isn't SetProxies just set_proxies? Because it's virtual?

Just to verify my understanding and your intent because I think the behavior is
different than before. With the single base fetcher and a list of proxies, the
base fetcher would find a proxy that works for the first range and then reuse
that proxy for the subsequent ranges? This is probably the right thing to do but
it's different than the previous implementation which would retry all proxies
for every range, I think.

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

Good catch. I don't know how big a deal it is, but I would lean toward frequent
proxy recalculation. For example, if the proxies change between ranges, we would
want the subsequent ranges to use the new proxies. This CL doesn't help get us
there, but a future CL I'd like to write (ProxyHTTPFetcher) hopefully would.

For now I think it's okay to keep as-is, esp since proxy support is half-baked
anyway.

+    base_fetcher_->SetProxies(proxies);
+  }
+
+ private:
+  // Stage change: Stopped or Downloading -> Downloading

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

State

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

Done.

+  void StartTransfer();
+
+  // Stage change: Downloading -> Downloading

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

State

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

Done.

+  void ReceivedBytes(HttpFetcher* fetcher, const char* bytes, int length);
+
+  // Stage change: Pending transfer ended -> Stopped

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

State

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

Done.

+  void TransferEnded(HttpFetcher* fetcher, bool successful);
+  void TransferComplete(HttpFetcher* fetcher, bool successful) {
+    LOG(INFO) << "Received transfer complete.";

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

I'd move to the .cc file.

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

Done.

+    TransferEnded(fetcher, successful);
+  }
+  void TransferTerminated(HttpFetcher* fetcher) {
+    LOG(INFO) << "Received transfer terminated.";

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

I'd move to the .cc file.

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

Done.

+    TransferEnded(fetcher, false);
+  }
+
+  void Reset() {
+    base_fetcher_active_ = pending_transfer_ended_ = terminating_ = false;

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

Move to the .cc files -- it's 3 lines of code :-)

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

Done.

+    current_index_ = 0;
+    bytes_received_this_fetcher_ = 0;
+  }
+
+  scoped_ptr<HttpFetcher> base_fetcher_;
+
+  // If true, do not send any more data or TransferComplete to the delegate.
+  bool base_fetcher_active_;
+
+  // If true, the next fetcher needs to be started when TransferTerminated is
+  // received from the current fetcher.
+  bool pending_transfer_ended_;
+  
+  // True if we are waiting for base fetcher to terminate b/c we are
+  // ourselves terminating.
+  bool terminating_;
+
+  RangesVect ranges_;
+
+  RangesVect::size_type current_index_;  // index into ranges_
+  off_t bytes_received_this_fetcher_;

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

It's more like bytes_received_this_range now.

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

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(MultiRangeHTTPFetcher);
+};
+
+}  // namespace chromeos_update_engine
+
+#endif  // CHROMEOS_PLATFORM_UPDATE_ENGINE_MULTI_RANGE_HTTP_FETCHER_H__