AU: MultiHttpFetcher cleanup/rewrite

AU: MultiHttpFetcher cleanup/rewrite

This is the first of many CLs to cleanup/refactor/unfork the
HttpFetcher classes.

This CL changes MultiHttpFetcher to MultiRangeHTTPFetcher, makes it
work with a single base fetcher, and un-templatizes it.

Also, fix a (new?) bug in SConstruct w/ setting CCFLAGS.

TEST=unittests, tested an interrupted/resumed update on device.
BUG=10395
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=819fef2

[petkov, 2010-12-16 18:30:56]

This looks better, I think -- thanks for cleaning up. Some comments below.

http://codereview.chromium.org/5835004/diff/1/http_fetcher.h
File http_fetcher.h (right):

http://codereview.chromium.org/5835004/diff/1/http_fetcher.h#newcode91
http_fetcher.h:91: virtual void SetConnectionAsExpensive(bool is_expensive) {}
Maybe add a comment that these methods are used for testing.

http://codereview.chromium.org/5835004/diff/1/http_fetcher.h#newcode130
http_fetcher.h:130: // When a transfer has completed, exactly one of these two
methods will be
You may want to combine the docstrings for the two methods into a single
docstring and then just declare the methods.

http://codereview.chromium.org/5835004/diff/1/http_fetcher.h#newcode136
http_fetcher.h:136: virtual void TransferComplete(HttpFetcher* fetcher, bool
successful) = 0;
Side note: Chrome has a convention to use "OnEvent" style names for this sort of
callback methods (e.g., "OnTransferComplete"). It might make sense to adopt a
similar convention (or at least some convention) for the rest of the Chrome OS
packages (or at least for the update engine).

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc
File multi_range_http_fetcher.cc (right):

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:13: LOG_IF(ERROR, base_fetcher_active_) <<
"BeginTransfer but already active.";
These conditions would be pretty bad if they actually occurred. Should we just
CHECK/crash instead?

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:31: // Stage change: Downloading -> Pending transfer
ended
State

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:35: return;
So, if the client invokes TerminateTransfer before BeginTransfer, the client
won't get the TransferTerminated callback?

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:41: return;
no need for this return here.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:64: TEST_AND_RETURN(current_index_ <
ranges_.size());
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.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:94: LOG_IF(ERROR, !pending_transfer_ended_)
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.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:131: // LOG(INFO) << "passed in success: " <<
successful;
remove commented code

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h
File multi_range_http_fetcher.h (right):

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:16: #include "update_engine/utils.h"
do you need this include here?

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:49: void set_ranges(const RangesVect& ranges) {
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.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:53: void SetOffset(off_t offset) {}  // for now,
doesn't support this
Per style, all inherited virtual methods should also be declared virtual -- here
and below (BeginTransfer, TerminateTransfer, Pause, Unpause, etc.)

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:60: // Stage change: Downloading -> Pending transfer
ended
typo: Stage->State

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:63: void Pause() {
Fits on one line?

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:67: void Unpause() {
Fits on one line?

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:72: void set_idle_seconds(int seconds) {
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.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:84: virtual void SetProxies(const
std::deque<std::string>& proxies) {
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.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:89: // Stage change: Stopped or Downloading ->
Downloading
State

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:92: // Stage change: Downloading -> Downloading
State

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:95: // Stage change: Pending transfer ended ->
Stopped
State

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:98: LOG(INFO) << "Received transfer complete.";
I'd move to the .cc file.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:102: LOG(INFO) << "Received transfer terminated.";
I'd move to the .cc file.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:107: base_fetcher_active_ = pending_transfer_ended_ =
terminating_ = false;
Move to the .cc files -- it's 3 lines of code :-)

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:128: off_t bytes_received_this_fetcher_;
It's more like bytes_received_this_range now.

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

Addressed your comments. Please have another look.

Thanks,

http://codereview.chromium.org/5835004/diff/1/http_fetcher.h
File http_fetcher.h (right):

http://codereview.chromium.org/5835004/diff/1/http_fetcher.h#newcode91
http_fetcher.h:91: virtual void SetConnectionAsExpensive(bool is_expensive) {}
On 2010/12/16 18:30:57, petkov wrote:
> Maybe add a comment that these methods are used for testing.

Done.

http://codereview.chromium.org/5835004/diff/1/http_fetcher.h#newcode130
http_fetcher.h:130: // When a transfer has completed, exactly one of these two
methods will be
On 2010/12/16 18:30:57, petkov wrote:
> You may want to combine the docstrings for the two methods into a single
> docstring and then just declare the methods.

Done.

http://codereview.chromium.org/5835004/diff/1/http_fetcher.h#newcode136
http_fetcher.h:136: virtual void TransferComplete(HttpFetcher* fetcher, bool
successful) = 0;
On 2010/12/16 18:30:57, petkov wrote:
> Side note: Chrome has a convention to use "OnEvent" style names for this sort
of
> callback methods (e.g., "OnTransferComplete"). It might make sense to adopt a
> similar convention (or at least some convention) for the rest of the Chrome OS
> packages (or at least for the update engine).

That's a good idea. I'll make a note to update our callbacks like this.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc
File multi_range_http_fetcher.cc (right):

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:13: LOG_IF(ERROR, base_fetcher_active_) <<
"BeginTransfer but already active.";
On 2010/12/16 18:30:57, petkov wrote:
> These conditions would be pretty bad if they actually occurred. Should we just
> CHECK/crash instead?

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:31: // Stage change: Downloading -> Pending transfer
ended
On 2010/12/16 18:30:57, petkov wrote:
> State

done.

Copy paste FAIL :(

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:35: return;
On 2010/12/16 18:30:57, petkov wrote:
> So, if the client invokes TerminateTransfer before BeginTransfer, the client
> won't get the TransferTerminated callback?

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.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:41: return;
On 2010/12/16 18:30:57, petkov wrote:
> no need for this return here.

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:64: TEST_AND_RETURN(current_index_ <
ranges_.size());
On 2010/12/16 18:30:57, petkov wrote:
> 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.

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:94: LOG_IF(ERROR, !pending_transfer_ended_)
On 2010/12/16 18:30:57, petkov wrote:
> 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.

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

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.cc#new...
multi_range_http_fetcher.cc:131: // LOG(INFO) << "passed in success: " <<
successful;
On 2010/12/16 18:30:57, petkov wrote:
> remove commented code

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h
File multi_range_http_fetcher.h (right):

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:16: #include "update_engine/utils.h"
On 2010/12/16 18:30:57, petkov wrote:
> do you need this include here?

nope. cleaned up the headers

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:49: void set_ranges(const RangesVect& ranges) {
On 2010/12/16 18:30:57, petkov wrote:
> 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.

interesting idea, done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:53: void SetOffset(off_t offset) {}  // for now,
doesn't support this
On 2010/12/16 18:30:57, petkov wrote:
> Per style, all inherited virtual methods should also be declared virtual --
here
> and below (BeginTransfer, TerminateTransfer, Pause, Unpause, etc.)

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:60: // Stage change: Downloading -> Pending transfer
ended
On 2010/12/16 18:30:57, petkov wrote:
> typo: Stage->State

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:63: void Pause() {
On 2010/12/16 18:30:57, petkov wrote:
> Fits on one line?

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:67: void Unpause() {
On 2010/12/16 18:30:57, petkov wrote:
> Fits on one line?

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:72: void set_idle_seconds(int seconds) {
On 2010/12/16 18:30:57, petkov wrote:
> 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.
> 

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.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:84: virtual void SetProxies(const
std::deque<std::string>& proxies) {
On 2010/12/16 18:30:57, petkov wrote:
> 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.

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.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:89: // Stage change: Stopped or Downloading ->
Downloading
On 2010/12/16 18:30:57, petkov wrote:
> State

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:92: // Stage change: Downloading -> Downloading
On 2010/12/16 18:30:57, petkov wrote:
> State

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:95: // Stage change: Pending transfer ended ->
Stopped
On 2010/12/16 18:30:57, petkov wrote:
> State

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:98: LOG(INFO) << "Received transfer complete.";
On 2010/12/16 18:30:57, petkov wrote:
> I'd move to the .cc file.

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:102: LOG(INFO) << "Received transfer terminated.";
On 2010/12/16 18:30:57, petkov wrote:
> I'd move to the .cc file.

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:107: base_fetcher_active_ = pending_transfer_ended_ =
terminating_ = false;
On 2010/12/16 18:30:57, petkov wrote:
> Move to the .cc files -- it's 3 lines of code :-)

Done.

http://codereview.chromium.org/5835004/diff/1/multi_range_http_fetcher.h#newc...
multi_range_http_fetcher.h:128: off_t bytes_received_this_fetcher_;
On 2010/12/16 18:30:57, petkov wrote:
> It's more like bytes_received_this_range now.

Done.

[petkov, 2010-12-17 13:37:25]

LGTM w/ some nits mostly about comments.

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.cc
File multi_range_http_fetcher.cc (right):

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.cc#...
multi_range_http_fetcher.cc:84: // Terminates the current fetcher. Waits for its
TransferTerminated
this comment needs to be updated to reflect the fact that we're reusing the same
base fetcher.

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.cc#...
multi_range_http_fetcher.cc:127: LOG(INFO) << "Starting next transfer.";
maybe include the index in the log...

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.cc#...
multi_range_http_fetcher.cc:145: void
MultiRangeHTTPFetcher::TransferTerminated(HttpFetcher* fetcher) {
add a blank line

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.h
File multi_range_http_fetcher.h (right):

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.h#n...
multi_range_http_fetcher.h:16: // This class is a simple wrapper around an
HttpFetcher. The client
maybe we should document here that the same base http fetcher is reused for all
ranges so it should support starting/terminating transfers through the same
class instance. or maybe that should be documented as a requirement in the
HttpFetcher.

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.h#n...
multi_range_http_fetcher.h:113: typedef std::vector<std::pair<off_t, off_t> >
RangesVect;
per style, the typedef should be right after private:, I think

[petkov, 2010-12-17 18:50:23]

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.cc
File multi_range_http_fetcher.cc (right):

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.cc#...
multi_range_http_fetcher.cc:97: CHECK(base_fetcher_active_) << "Transfer ended
unexpectedly.";
Btw, you could also CHECK here that fetcher == base_fetcher_

[adlr, 2010-12-17 19:34:15]

thanks. fixed and pushed

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.cc
File multi_range_http_fetcher.cc (right):

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.cc#...
multi_range_http_fetcher.cc:84: // Terminates the current fetcher. Waits for its
TransferTerminated
On 2010/12/17 13:37:25, petkov wrote:
> this comment needs to be updated to reflect the fact that we're reusing the
same
> base fetcher.

Done.

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.cc#...
multi_range_http_fetcher.cc:97: CHECK(base_fetcher_active_) << "Transfer ended
unexpectedly.";
On 2010/12/17 18:50:24, petkov wrote:
> Btw, you could also CHECK here that fetcher == base_fetcher_

Done.

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.cc#...
multi_range_http_fetcher.cc:127: LOG(INFO) << "Starting next transfer.";
On 2010/12/17 13:37:25, petkov wrote:
> maybe include the index in the log...

Done.

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.cc#...
multi_range_http_fetcher.cc:145: void
MultiRangeHTTPFetcher::TransferTerminated(HttpFetcher* fetcher) {
On 2010/12/17 13:37:25, petkov wrote:
> add a blank line

Done.

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.h
File multi_range_http_fetcher.h (right):

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.h#n...
multi_range_http_fetcher.h:16: // This class is a simple wrapper around an
HttpFetcher. The client
On 2010/12/17 13:37:25, petkov wrote:
> maybe we should document here that the same base http fetcher is reused for
all
> ranges so it should support starting/terminating transfers through the same
> class instance. or maybe that should be documented as a requirement in the
> HttpFetcher.

Done.

http://codereview.chromium.org/5835004/diff/7001/multi_range_http_fetcher.h#n...
multi_range_http_fetcher.h:113: typedef std::vector<std::pair<off_t, off_t> >
RangesVect;
On 2010/12/17 13:37:25, petkov wrote:
> per style, the typedef should be right after private:, I think

Done.