Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4194001:
Implement exponential back-off mechanism and enforce it at the URLRequestHttpJob level. (Closed)
|
Created:
10 years, 1 month ago by yzshen Modified:
9 years, 7 months ago Reviewers:
Peter Kasting, willchan no longer on Chromium, Jói, Sigurður Ásgeirsson, sanjeevr, eroman, joi CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionImplement exponential back-off mechanism and enforce it at the URLRequestHttpJob level for all outgoing HTTP requests.
The reason why to make this change is that we need back-off logic at a lower enough level to manage all outgoing HTTP traffic, so that the browser won't cause any DDoS attack.
This change:
1) patches http://codereview.chromium.org/2487001/show, which is the exponential back-off implementation.
2) resolves conflicts with URLFetcher, by removing its own back-off logic:
-- removes url_fetcher_protect.{h,cc};
-- integrates the sliding window mechanism of URLFetcherProtectEntry into RequestThrottlerEntry.
3) resolves conflicts with CloudPrintURLFetcher.
4) makes unit tests of CloudPrintURLFetcher, URLFetcher and URLRequest work.
BUG=none
TEST=pass all existing tests and also the newly-added request_throttler_unittest.cc
Patch Set 1 #Patch Set 2 : '' #
Total comments: 10
Patch Set 3 : '' #
Total comments: 3
Patch Set 4 : '' #
Total comments: 19
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 14
Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 38
Patch Set 11 : '' #
Total comments: 13
Patch Set 12 : '' #
Total comments: 4
Patch Set 13 : '' #
Total comments: 25
Patch Set 14 : '' #Patch Set 15 : '' #Patch Set 16 : '' #Patch Set 17 : '' #Messages
Total messages: 50 (0 generated)
Hi Yuzhu, Great work. Just two high-level comments to get the ball rolling. I'm sorry this has taken so long, and honestly I still haven't reviewed in much detail, although I've skimmed through all the deltas from the original reviewed change and the current change. Cheers, Jói http://codereview.chromium.org/4194001/diff/15001/16010 File net/base/run_all_unittests.cc (right): http://codereview.chromium.org/4194001/diff/15001/16010#newcode28 net/base/run_all_unittests.cc:28: Singleton<RequestThrottlerManager>::get()->DisableThrottlingByDefault(); I'm a bit wary of this. Since we're trying to make the throttler as unobtrusive as possible, it would be nice to see that most of the tests pass without doing anything special. The only tests that should really be affected are tests that are making requests that fail with e.g. 4xx or 5xx HTTP error codes, and this would hopefully be a small subset of the tests. Can we keep the throttler enabled and turn it off (or add entries) for just the tests needed? If it's causing lots of tests to fail right now I think that's actually a bad sign. http://codereview.chromium.org/4194001/diff/15001/16013 File net/request_throttler/request_throttler_entry.h (right): http://codereview.chromium.org/4194001/diff/15001/16013#newcode122 net/request_throttler/request_throttler_entry.h:122: // A list of the recent send events. We use them to decide whether there are I haven't looked in detail yet, but if I understand correctly, the sliding window is intended to limit the number of requests (non-failing or failing) in a given time period. If this is the case I'm not sure it belongs at this low level; I think in the interest of being the least intrusive possible, we should be focusing only on backing off on error conditions (4xx and 5xx, maybe server timeouts). Please explain if I am misunderstanding the concept. As I said, I have yet to review in detail (sorry it's taking so long).
Hi, Joi. Thanks a lot for your comments! http://codereview.chromium.org/4194001/diff/15001/net/base/run_all_unittests.cc File net/base/run_all_unittests.cc (right): http://codereview.chromium.org/4194001/diff/15001/net/base/run_all_unittests.... net/base/run_all_unittests.cc:28: Singleton<RequestThrottlerManager>::get()->DisableThrottlingByDefault(); On 2010/11/09 20:07:17, joi wrote: > I'm a bit wary of this. Since we're trying to make the throttler as unobtrusive > as possible, it would be nice to see that most of the tests pass without doing > anything special. The only tests that should really be affected are tests that > are making requests that fail with e.g. 4xx or 5xx HTTP error codes, and this > would hopefully be a small subset of the tests. Yes, tests that deal with HTTP failures are a small subset and probably won't cause trouble. What bothers us is the sliding window mechanism. It is possible that automatic tests run so fast that a lot of requests to fetch the same URL are issued in a short period. I met two test cases which tried to send 50 requests in a short period. But it is possible that the sliding window results in flakiness in more obscure way, so I explicitly disable it. (Maybe this is a reason why sliding window should not belong to this level?) > Can we keep the throttler enabled and turn it off (or add entries) for just the > tests needed? If it's causing lots of tests to fail right now I think that's > actually a bad sign. http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... File net/request_throttler/request_throttler_entry.h (right): http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... net/request_throttler/request_throttler_entry.h:122: // A list of the recent send events. We use them to decide whether there are On 2010/11/09 20:07:17, joi wrote: > I haven't looked in detail yet, but if I understand correctly, the sliding > window is intended to limit the number of requests (non-failing or failing) in a > given time period. Yes, that is how it works. > If this is the case I'm not sure it belongs at this low > level; I think in the interest of being the least intrusive possible, we should > be focusing only on backing off on error conditions (4xx and 5xx, maybe server > timeouts). At the first glance, I thought that sliding window is also a method to do request throttling and should be moved downwards as well. But you are right, I realized that it is more intrusive after I took a closer look at it. I will reconsider it and propose another solution, probably keeping the sliding window mechanism at the URLFetcher level. > Please explain if I am misunderstanding the concept. As I said, I have yet to > review in detail (sorry it's taking so long).
My concern with the sliding level at such a low level is exactly that it would be too intrusive, which is what you're seeing in the net tests. I agree with your conclusion, the sliding window mechanism should be moved back up to the URLFetcher level. Cheers, Jói On Tue, Nov 9, 2010 at 5:32 PM, <yzshen@google.com> wrote: > Hi, Joi. > > Thanks a lot for your comments! > > > http://codereview.chromium.org/4194001/diff/15001/net/base/run_all_unittests.cc > File net/base/run_all_unittests.cc (right): > > http://codereview.chromium.org/4194001/diff/15001/net/base/run_all_unittests.... > net/base/run_all_unittests.cc:28: > Singleton<RequestThrottlerManager>::get()->DisableThrottlingByDefault(); > On 2010/11/09 20:07:17, joi wrote: >> >> I'm a bit wary of this. Since we're trying to make the throttler as > > unobtrusive >> >> as possible, it would be nice to see that most of the tests pass > > without doing >> >> anything special. The only tests that should really be affected are > > tests that >> >> are making requests that fail with e.g. 4xx or 5xx HTTP error codes, > > and this >> >> would hopefully be a small subset of the tests. > > Yes, tests that deal with HTTP failures are a small subset and probably > won't cause trouble. > > What bothers us is the sliding window mechanism. It is possible that > automatic tests run so fast that a lot of requests to fetch the same URL > are issued in a short period. > > I met two test cases which tried to send 50 requests in a short period. > But it is possible that the sliding window results in flakiness in more > obscure way, so I explicitly disable it. > (Maybe this is a reason why sliding window should not belong to this > level?) > >> Can we keep the throttler enabled and turn it off (or add entries) for > > just the >> >> tests needed? If it's causing lots of tests to fail right now I think > > that's >> >> actually a bad sign. > > http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... > File net/request_throttler/request_throttler_entry.h (right): > > http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... > net/request_throttler/request_throttler_entry.h:122: // A list of the > recent send events. We use them to decide whether there are > On 2010/11/09 20:07:17, joi wrote: >> >> I haven't looked in detail yet, but if I understand correctly, the > > sliding >> >> window is intended to limit the number of requests (non-failing or > > failing) in a >> >> given time period. > > Yes, that is how it works. > >> If this is the case I'm not sure it belongs at this low >> level; I think in the interest of being the least intrusive possible, > > we should >> >> be focusing only on backing off on error conditions (4xx and 5xx, > > maybe server >> >> timeouts). > > At the first glance, I thought that sliding window is also a method to > do request throttling and should be moved downwards as well. But you are > right, I realized that it is more intrusive after I took a closer look > at it. > > I will reconsider it and propose another solution, probably keeping the > sliding window mechanism at the URLFetcher level. > >> Please explain if I am misunderstanding the concept. As I said, I have > > yet to >> >> review in detail (sorry it's taking so long). > > http://codereview.chromium.org/4194001/ >
Hi, Joi. Thanks for your suggestion! I will work on that and update the change ASAP. On Tue, Nov 9, 2010 at 2:34 PM, Jói Sigurðsson <joi@google.com> wrote: > My concern with the sliding level at such a low level is exactly that > it would be too intrusive, which is what you're seeing in the net > tests. I agree with your conclusion, the sliding window mechanism > should be moved back up to the URLFetcher level. > > Cheers, > Jói > > > On Tue, Nov 9, 2010 at 5:32 PM, <yzshen@google.com> wrote: >> Hi, Joi. >> >> Thanks a lot for your comments! >> >> >> http://codereview.chromium.org/4194001/diff/15001/net/base/run_all_unittests.cc >> File net/base/run_all_unittests.cc (right): >> >> http://codereview.chromium.org/4194001/diff/15001/net/base/run_all_unittests.... >> net/base/run_all_unittests.cc:28: >> Singleton<RequestThrottlerManager>::get()->DisableThrottlingByDefault(); >> On 2010/11/09 20:07:17, joi wrote: >>> >>> I'm a bit wary of this. Since we're trying to make the throttler as >> >> unobtrusive >>> >>> as possible, it would be nice to see that most of the tests pass >> >> without doing >>> >>> anything special. The only tests that should really be affected are >> >> tests that >>> >>> are making requests that fail with e.g. 4xx or 5xx HTTP error codes, >> >> and this >>> >>> would hopefully be a small subset of the tests. >> >> Yes, tests that deal with HTTP failures are a small subset and probably >> won't cause trouble. >> >> What bothers us is the sliding window mechanism. It is possible that >> automatic tests run so fast that a lot of requests to fetch the same URL >> are issued in a short period. >> >> I met two test cases which tried to send 50 requests in a short period. >> But it is possible that the sliding window results in flakiness in more >> obscure way, so I explicitly disable it. >> (Maybe this is a reason why sliding window should not belong to this >> level?) >> >>> Can we keep the throttler enabled and turn it off (or add entries) for >> >> just the >>> >>> tests needed? If it's causing lots of tests to fail right now I think >> >> that's >>> >>> actually a bad sign. >> >> http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... >> File net/request_throttler/request_throttler_entry.h (right): >> >> http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... >> net/request_throttler/request_throttler_entry.h:122: // A list of the >> recent send events. We use them to decide whether there are >> On 2010/11/09 20:07:17, joi wrote: >>> >>> I haven't looked in detail yet, but if I understand correctly, the >> >> sliding >>> >>> window is intended to limit the number of requests (non-failing or >> >> failing) in a >>> >>> given time period. >> >> Yes, that is how it works. >> >>> If this is the case I'm not sure it belongs at this low >>> level; I think in the interest of being the least intrusive possible, >> >> we should >>> >>> be focusing only on backing off on error conditions (4xx and 5xx, >> >> maybe server >>> >>> timeouts). >> >> At the first glance, I thought that sliding window is also a method to >> do request throttling and should be moved downwards as well. But you are >> right, I realized that it is more intrusive after I took a closer look >> at it. >> >> I will reconsider it and propose another solution, probably keeping the >> sliding window mechanism at the URLFetcher level. >> >>> Please explain if I am misunderstanding the concept. As I said, I have >> >> yet to >>> >>> review in detail (sorry it's taking so long). >> >> http://codereview.chromium.org/4194001/ >> > -- Best regards, Yuzhu Shen.
Hi, Joi. I have made change according to your suggestions. Please take another look. Thanks! On 2010/11/09 22:36:44, yzshen wrote: > Hi, Joi. > > Thanks for your suggestion! > I will work on that and update the change ASAP. > > On Tue, Nov 9, 2010 at 2:34 PM, Jói Sigurðsson <mailto:joi@google.com> wrote: > > My concern with the sliding level at such a low level is exactly that > > it would be too intrusive, which is what you're seeing in the net > > tests. I agree with your conclusion, the sliding window mechanism > > should be moved back up to the URLFetcher level. > > > > Cheers, > > Jói > > > > > > On Tue, Nov 9, 2010 at 5:32 PM, mailto: <yzshen@google.com> wrote: > >> Hi, Joi. > >> > >> Thanks a lot for your comments! > >> > >> > >> > http://codereview.chromium.org/4194001/diff/15001/net/base/run_all_unittests.cc > >> File net/base/run_all_unittests.cc (right): > >> > >> > http://codereview.chromium.org/4194001/diff/15001/net/base/run_all_unittests.... > >> net/base/run_all_unittests.cc:28: > >> Singleton<RequestThrottlerManager>::get()->DisableThrottlingByDefault(); > >> On 2010/11/09 20:07:17, joi wrote: > >>> > >>> I'm a bit wary of this. Since we're trying to make the throttler as > >> > >> unobtrusive > >>> > >>> as possible, it would be nice to see that most of the tests pass > >> > >> without doing > >>> > >>> anything special. The only tests that should really be affected are > >> > >> tests that > >>> > >>> are making requests that fail with e.g. 4xx or 5xx HTTP error codes, > >> > >> and this > >>> > >>> would hopefully be a small subset of the tests. > >> > >> Yes, tests that deal with HTTP failures are a small subset and probably > >> won't cause trouble. > >> > >> What bothers us is the sliding window mechanism. It is possible that > >> automatic tests run so fast that a lot of requests to fetch the same URL > >> are issued in a short period. > >> > >> I met two test cases which tried to send 50 requests in a short period. > >> But it is possible that the sliding window results in flakiness in more > >> obscure way, so I explicitly disable it. > >> (Maybe this is a reason why sliding window should not belong to this > >> level?) > >> > >>> Can we keep the throttler enabled and turn it off (or add entries) for > >> > >> just the > >>> > >>> tests needed? If it's causing lots of tests to fail right now I think > >> > >> that's > >>> > >>> actually a bad sign. > >> > >> > http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... > >> File net/request_throttler/request_throttler_entry.h (right): > >> > >> > http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... > >> net/request_throttler/request_throttler_entry.h:122: // A list of the > >> recent send events. We use them to decide whether there are > >> On 2010/11/09 20:07:17, joi wrote: > >>> > >>> I haven't looked in detail yet, but if I understand correctly, the > >> > >> sliding > >>> > >>> window is intended to limit the number of requests (non-failing or > >> > >> failing) in a > >>> > >>> given time period. > >> > >> Yes, that is how it works. > >> > >>> If this is the case I'm not sure it belongs at this low > >>> level; I think in the interest of being the least intrusive possible, > >> > >> we should > >>> > >>> be focusing only on backing off on error conditions (4xx and 5xx, > >> > >> maybe server > >>> > >>> timeouts). > >> > >> At the first glance, I thought that sliding window is also a method to > >> do request throttling and should be moved downwards as well. But you are > >> right, I realized that it is more intrusive after I took a closer look > >> at it. > >> > >> I will reconsider it and propose another solution, probably keeping the > >> sliding window mechanism at the URLFetcher level. > >> > >>> Please explain if I am misunderstanding the concept. As I said, I have > >> > >> yet to > >>> > >>> review in detail (sorry it's taking so long). > >> > >> http://codereview.chromium.org/4194001/ > >> > > > > > > -- > Best regards, > Yuzhu Shen.
Also adding pkasting/willchan who will likely want to comment. http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... File net/request_throttler/request_throttler_manager.cc (right): http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... net/request_throttler/request_throttler_manager.cc:50: url_id += url.scheme(); Is this going to be reached for data: URLs? We wouldn't ever want those to get throttled. Same thing for externally registered schemes. http://codereview.chromium.org/4194001/diff/42001/net/request_throttler/reque... File net/request_throttler/request_throttler_entry.cc (right): http://codereview.chromium.org/4194001/diff/42001/net/request_throttler/reque... net/request_throttler/request_throttler_entry.cc:151: AutoLock auto_lock(lock_); There is a lot of locking in this CL; I guess that is because the "request throttling manager" is a global. We really try to avoid globals/singletons. Rather, can you put this dependency into the URLRequestContext, and then it does not need to be threadsafe (since by definition URLRequestContexts are only used one thread).
On 2010/11/11 17:54:21, eroman wrote:
> Also adding pkasting/willchan who will likely want to comment.
What is it you want me to look at and comment on? I glanced briefly at the
changes to url_fetcher.{cc,h} and they looked OK.
Hi Yuzhu, I'm confused, I thought in this patch set you had moved the sliding window out of the RequestThrottlerManager and back up to the URLFetcher level, but I'm still seeing it in request_throttler_manager.cc Let me know if I'm misunderstanding something. Cheers, Jói On Thu, Nov 11, 2010 at 1:53 PM, <pkasting@chromium.org> wrote: > On 2010/11/11 17:54:21, eroman wrote: >> >> Also adding pkasting/willchan who will likely want to comment. > > What is it you want me to look at and comment on? I glanced briefly at the > changes to url_fetcher.{cc,h} and they looked OK. > > http://codereview.chromium.org/4194001/ >
On Thu, Nov 11, 2010 at 10:53 AM, <pkasting@chromium.org> wrote: > On 2010/11/11 17:54:21, eroman wrote: > >> Also adding pkasting/willchan who will likely want to comment. >> > > What is it you want me to look at and comment on? I glanced briefly at the > changes to url_fetcher.{cc,h} and they looked OK. I thought you were the original author of the URL Fetcher's "protect entry", and hence would want to be kept in the loop that it was being deleted. > > http://codereview.chromium.org/4194001/ >
On 2010/11/11 19:22:58, eroman wrote: > I thought you were the original author of the URL Fetcher's "protect entry", > and hence would want to be kept in the loop that it was being deleted. Actually I didn't write that code. There was a separate thread in which I got worried about this change and was assured that smarter minds had already OKed it, so I have no objections.
I'm going to dive into the review later today. First question, why isn't all
this request throttling stuff in net/url_request? Do we really need another
toplevel net dir for this?
On 2010/11/11 19:22:58, eroman wrote:
> On Thu, Nov 11, 2010 at 10:53 AM, <mailto:pkasting@chromium.org> wrote:
>
> > On 2010/11/11 17:54:21, eroman wrote:
> >
> >> Also adding pkasting/willchan who will likely want to comment.
> >>
> >
> > What is it you want me to look at and comment on? I glanced briefly at the
> > changes to url_fetcher.{cc,h} and they looked OK.
>
>
> I thought you were the original author of the URL Fetcher's "protect entry",
> and hence would want to be kept in the loop that it was being deleted.
>
>
> >
> > http://codereview.chromium.org/4194001/
> >
Hi, Joi. On Thu, Nov 11, 2010 at 11:04 AM, Jói Sigurðsson <joi@chromium.org> wrote: > Hi Yuzhu, > > I'm confused, I thought in this patch set you had moved the sliding > window out of the RequestThrottlerManager and back up to the > URLFetcher level, but I'm still seeing it in > request_throttler_manager.cc > The sliding window logic is still in RequestThrottler*, however, URLRequestHttpJob won't consider it when deciding whether to reject outgoing requests. It is only used when upper layer modules (say, URLFetcher) want to get a "recommended" delay for the next request. I have added comments in request_throttler_entry_interface.h about this. If I move the sliding window logic back to URLFetcher(Protect), we have two sets of *Manager and *Entry. It seems confusing and redundant to me. What do you think? Thanks! > > Let me know if I'm misunderstanding something. > > Cheers, > Jói > > > On Thu, Nov 11, 2010 at 1:53 PM, <pkasting@chromium.org> wrote: > > On 2010/11/11 17:54:21, eroman wrote: > >> > >> Also adding pkasting/willchan who will likely want to comment. > > > > What is it you want me to look at and comment on? I glanced briefly at > the > > changes to url_fetcher.{cc,h} and they looked OK. > > > > http://codereview.chromium.org/4194001/ > > > -- Best regards, Yuzhu Shen.
Hi, William. That sounds a good idea to me. Thanks for your suggestion! On Thu, Nov 11, 2010 at 11:34 AM, <willchan@chromium.org> wrote: > I'm going to dive into the review later today. First question, why isn't > all > this request throttling stuff in net/url_request? Do we really need > another > toplevel net dir for this? > > > On 2010/11/11 19:22:58, eroman wrote: > >> On Thu, Nov 11, 2010 at 10:53 AM, <mailto:pkasting@chromium.org> wrote: >> > > > On 2010/11/11 17:54:21, eroman wrote: >> > >> >> Also adding pkasting/willchan who will likely want to comment. >> >> >> > >> > What is it you want me to look at and comment on? I glanced briefly at >> the >> > changes to url_fetcher.{cc,h} and they looked OK. >> > > > I thought you were the original author of the URL Fetcher's "protect >> entry", >> and hence would want to be kept in the loop that it was being deleted. >> > > > > >> >> > http://codereview.chromium.org/4194001/ >> > >> > > > > http://codereview.chromium.org/4194001/ > -- Best regards, Yuzhu Shen.
Hi, Eric. Thanks for your comments! http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... File net/request_throttler/request_throttler_manager.cc (right): http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... net/request_throttler/request_throttler_manager.cc:50: url_id += url.scheme(); On 2010/11/11 17:54:21, eroman wrote: > Is this going to be reached for data: URLs? We wouldn't ever want those to get > throttled. Same thing for externally registered schemes. At URLRequest(Job) level, only URLRequestHttpJob enforces exponential back-off and rejects outgoing requests when server errors are encountered. Other schemes won't be affected. The only scenario I can think up in which RequestThrottlerEntry/Manager can affect other schemes: use URLFetcher to fetch URLs with schemes other than http/https. In that case, the sliding window will apply to those requests. However, I don't think that it is a problem: it behaves in the same way as before. http://codereview.chromium.org/4194001/diff/42001/net/request_throttler/reque... File net/request_throttler/request_throttler_entry.cc (right): http://codereview.chromium.org/4194001/diff/42001/net/request_throttler/reque... net/request_throttler/request_throttler_entry.cc:151: AutoLock auto_lock(lock_); On 2010/11/11 17:54:21, eroman wrote: > There is a lot of locking in this CL; I guess that is because the "request > throttling manager" is a global. > > We really try to avoid globals/singletons. Rather, can you put this dependency > into the URLRequestContext, and then it does not need to be threadsafe (since by > definition URLRequestContexts are only used one thread). > One thing that asks for all the locking is that upper layer modules (say, URLFetcher) need to call methods such as RegisterRequestUrl() or GetRecommendedDelayForNextRequest() on other threads: scoped_refptr<RequestThrottlerEntryInterface> entry = request_throttler_manager_->RegisterRequestUrl(original_url_); io_message_loop_proxy_->PostDelayedTask( FROM_HERE, NewRunnableMethod(this, &Core::StartURLRequest), entry->GetRecommendedDelayForNextRequest()); What do you think?
http://codereview.chromium.org/4194001/diff/42001/net/request_throttler/reque... File net/request_throttler/request_throttler_entry.cc (right): http://codereview.chromium.org/4194001/diff/42001/net/request_throttler/reque... net/request_throttler/request_throttler_entry.cc:151: AutoLock auto_lock(lock_); On 2010/11/11 22:17:05, yzshen wrote: > On 2010/11/11 17:54:21, eroman wrote: > > There is a lot of locking in this CL; I guess that is because the "request > > throttling manager" is a global. > > > > We really try to avoid globals/singletons. Rather, can you put this dependency > > into the URLRequestContext, and then it does not need to be threadsafe (since > by > > definition URLRequestContexts are only used one thread). > > > One thing that asks for all the locking is that upper layer modules (say, > URLFetcher) need to call methods such as RegisterRequestUrl() or > GetRecommendedDelayForNextRequest() on other threads: > > scoped_refptr<RequestThrottlerEntryInterface> entry = > request_throttler_manager_->RegisterRequestUrl(original_url_); > io_message_loop_proxy_->PostDelayedTask( > FROM_HERE, > NewRunnableMethod(this, &Core::StartURLRequest), > entry->GetRecommendedDelayForNextRequest()); > > What do you think? I see. Is URLFetcher the only such consumer? If so, we could still support it by having it only access the throttling entry from the IO thread. URLFetcher is already posting tasks to the IO loop in order to call URLRequest, so we would just need to shift the backoff checks there as well.
For the GetRecommendedDelayForNextRequest thing, I think URLFetcher is the only consumer, so your suggestion seems possible - but doing things on a recommended delay sounds like an application-level thing (as we decided never to use the sliding window to actually block things) and so should probably stay at the URLFetcher level (?) At least one other feature, the RequestThrottlerManager::NotifyRequestBodyWasMalformed function, is designed to be called from various locations at the application level (wherever we're parsing a document body and find that it is broken). That particular function could be an asynchronous post to the thread on which the RTM lives, though. Alternatively, we could use locking with the expectation that almost all accesses occur on the I/O thread (i.e. we would push down the basic RegisterRequest / NotifyOfResponse stuff to the I/O thread) so there would be very low contention on the locks. Cheers, Jói
I went through the code again and I have some nits and suggestions, but overall I think it looks good. We need to work through eroman's questions and suggestions about threading. I didn't review all of the core logic in great detail, as I've reviewed it several times before, but I want to make sure to take a detailed look at the new/changed things. Is that primarily the sliding window stuff, or is there something else that's changed from malavv's original change? Cheers, Jói http://codereview.chromium.org/4194001/diff/15001/chrome/common/net/url_fetch... File chrome/common/net/url_fetcher.h (right): http://codereview.chromium.org/4194001/diff/15001/chrome/common/net/url_fetch... chrome/common/net/url_fetcher.h:210: // Maximum retris allowed. retris -> retries http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... File net/request_throttler/request_throttler_entry.h (right): http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... net/request_throttler/request_throttler_entry.h:44: // request. ex: "X-Retry-After" Suggest removing the example name. http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... File net/url_request/request_throttler_entry.cc (right): http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:67: exponential_backoff_release_time_ = base::TimeTicks::Now(); suggest using GetTimeNow() http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:75: sliding_window_release_time_ = base::TimeTicks::Now(); likewise http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:134: tracking_exponential_backoff_ = false; A comment on why this now gets set to false is in order; I believe it's correct to do this based on how the IsEntryOutdated function works, but it's non-obvious. Also, nit: This whole block could use a couple of lines of whitespace. http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... File net/url_request/request_throttler_entry.h (right): http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_entry.h:21: // sent during the back-off period will be cancelled. Secondly, a sliding window Secondly, a sliding... -> suggest replacement paragraph (and starting it as its own paragraph): A sliding window is used to count recent requests to a given destination and provide guidance (to the application level only) on whether too many requests have been sent and when a good time to send the next one would be. This is never used to deny requests at the network level. http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... File net/url_request/request_throttler_entry_interface.h (right): http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_entry_interface.h:13: // Represents an entry of the request throttler manager. change to: Interface provided on entries of the request throttler manager. http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... File net/url_request/request_throttler_manager.h (right): http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_manager.h:24: // Please refer to request_throttler_entry.h for further informations. request_throttler_entry.h -> request_throttler_entry_interface.h http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_manager.h:28: // This method is used by higher level modules which can notify this class if This comment is not clear enough. Suggest the following edit: Lets higher-level modules, that know how to parse particular response bodies, notify of receiving malformed content for the given URL. This will be handled by the throttler as if an HTTP 5xx response had been received to the request, i.e. it will count as a failure. http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_manager.h:36: // any) for the URL. if this is primarily for unit testing, we should say so
On 2010/11/11 23:57:14, eroman wrote: > http://codereview.chromium.org/4194001/diff/42001/net/request_throttler/reque... > File net/request_throttler/request_throttler_entry.cc (right): > > http://codereview.chromium.org/4194001/diff/42001/net/request_throttler/reque... > net/request_throttler/request_throttler_entry.cc:151: AutoLock auto_lock(lock_); > On 2010/11/11 22:17:05, yzshen wrote: > > On 2010/11/11 17:54:21, eroman wrote: > > > There is a lot of locking in this CL; I guess that is because the "request > > > throttling manager" is a global. > > > > > > We really try to avoid globals/singletons. Rather, can you put this > dependency > > > into the URLRequestContext, and then it does not need to be threadsafe > (since > > by > > > definition URLRequestContexts are only used one thread). > > > > > One thing that asks for all the locking is that upper layer modules (say, > > URLFetcher) need to call methods such as RegisterRequestUrl() or > > GetRecommendedDelayForNextRequest() on other threads: > > > > scoped_refptr<RequestThrottlerEntryInterface> entry = > > request_throttler_manager_->RegisterRequestUrl(original_url_); > > io_message_loop_proxy_->PostDelayedTask( > > FROM_HERE, > > NewRunnableMethod(this, &Core::StartURLRequest), > > entry->GetRecommendedDelayForNextRequest()); > > > > What do you think? > > I see. Is URLFetcher the only such consumer? If so, we could still support it by > having it only access the throttling entry from the IO thread. URLFetcher is > already posting tasks to the IO loop in order to call URLRequest, so we would > just need to shift the backoff checks there as well. Yes, currently URLFetcher is the only consumer. So the basic idea is: 1) post a task to the IO thread, GetRecommendedDelayForNextRequest() there; 2) if the delay is greater than 0, post another task to the IO thread to execute later. Is that correct? I think it is feasible. And as mentioned in Joi's mail, we could also make NotifyRequestBodyWasMalformed/OverrideEntry/etc. post tasks to the IO thread. However, it is harder for the application layer to use RequestThrottlerManager in this way. We probably need to write a proxy class for RequestThrottlerManager to handle threading issues. And I am not sure in this case whether running tasks on the IO thread is more efficient than locking, since the lock probably won't be used by other threads frequently. Please let me know whether you think this is a must and whether it needs to be done in this CL.
> Please let me know whether you think this is a must and whether it needs to > be > done in this CL. I think both of those use cases (recommended delay and notifying of a malformed body) can be done in a separate CL once we know for a fact that they are useful in the context of Chrome. As background for this change (and why I would like to keep e.g. "on malformed body" even though nobody is using it right now), the idea is for this code to be easily reusable in other code bases, e.g. other browsers, mobile platforms, etc., hence the dependency injection techniques around the interface on entries, the header adapter etc. It's not quite there yet, but since it is intended to end up as that kind of drop-in implementation, I'd like to keep the features. They are unit tested so they shouldn't rot. Cheers, Jói On Thu, Nov 11, 2010 at 7:49 PM, <yzshen@google.com> wrote: > On 2010/11/11 23:57:14, eroman wrote: > > http://codereview.chromium.org/4194001/diff/42001/net/request_throttler/reque... >> >> File net/request_throttler/request_throttler_entry.cc (right): > > > http://codereview.chromium.org/4194001/diff/42001/net/request_throttler/reque... >> >> net/request_throttler/request_throttler_entry.cc:151: AutoLock > > auto_lock(lock_); >> >> On 2010/11/11 22:17:05, yzshen wrote: >> > On 2010/11/11 17:54:21, eroman wrote: >> > > There is a lot of locking in this CL; I guess that is because the >> > > "request >> > > throttling manager" is a global. >> > > >> > > We really try to avoid globals/singletons. Rather, can you put this >> dependency >> > > into the URLRequestContext, and then it does not need to be threadsafe >> (since >> > by >> > > definition URLRequestContexts are only used one thread). >> > > >> > One thing that asks for all the locking is that upper layer modules >> > (say, >> > URLFetcher) need to call methods such as RegisterRequestUrl() or >> > GetRecommendedDelayForNextRequest() on other threads: >> > >> > scoped_refptr<RequestThrottlerEntryInterface> entry = >> > request_throttler_manager_->RegisterRequestUrl(original_url_); >> > io_message_loop_proxy_->PostDelayedTask( >> > FROM_HERE, >> > NewRunnableMethod(this, &Core::StartURLRequest), >> > entry->GetRecommendedDelayForNextRequest()); >> > >> > What do you think? > >> I see. Is URLFetcher the only such consumer? If so, we could still support >> it > > by >> >> having it only access the throttling entry from the IO thread. URLFetcher >> is >> already posting tasks to the IO loop in order to call URLRequest, so we >> would >> just need to shift the backoff checks there as well. > > Yes, currently URLFetcher is the only consumer. > > So the basic idea is: > 1) post a task to the IO thread, GetRecommendedDelayForNextRequest() there; > 2) if the delay is greater than 0, post another task to the IO thread to > execute > later. > Is that correct? > > I think it is feasible. And as mentioned in Joi's mail, we could also make > NotifyRequestBodyWasMalformed/OverrideEntry/etc. post tasks to the IO > thread. > > However, it is harder for the application layer to use > RequestThrottlerManager > in this way. We probably need to write a proxy class for > RequestThrottlerManager > to handle threading issues. And I am not sure in this case whether running > tasks > on the IO thread is more efficient than locking, since the lock probably > won't > be used by other threads frequently. > > Please let me know whether you think this is a must and whether it needs to > be > done in this CL. > > > http://codereview.chromium.org/4194001/ >
Hi, Joi. Thanks! >I didn't review all of the core logic in great detail, as >I've reviewed it >several times before, but I want to make sure to take a >detailed look at the >new/changed things. Is that primarily the sliding window >stuff, or is there >something else that's changed from malavv's original >change? Yes, the sliding window stuff is the primary change comparing with malavv's original change. http://codereview.chromium.org/4194001/diff/15001/chrome/common/net/url_fetch... File chrome/common/net/url_fetcher.h (right): http://codereview.chromium.org/4194001/diff/15001/chrome/common/net/url_fetch... chrome/common/net/url_fetcher.h:210: // Maximum retris allowed. On 2010/11/12 00:12:08, Jói wrote: > retris -> retries Done. http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... File net/request_throttler/request_throttler_entry.h (right): http://codereview.chromium.org/4194001/diff/15001/net/request_throttler/reque... net/request_throttler/request_throttler_entry.h:44: // request. ex: "X-Retry-After" On 2010/11/12 00:12:08, Jói wrote: > Suggest removing the example name. Done. http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... File net/url_request/request_throttler_entry.cc (right): http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:67: exponential_backoff_release_time_ = base::TimeTicks::Now(); On 2010/11/12 00:12:08, Jói wrote: > suggest using GetTimeNow() Using GetTimeNow() here is a little bit confusing, since this method is called in constructors, while in constructors virtual won't work. I added a comment. Is it okay? http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:75: sliding_window_release_time_ = base::TimeTicks::Now(); On 2010/11/12 00:12:08, Jói wrote: > likewise Please see the comment above. http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:134: tracking_exponential_backoff_ = false; On 2010/11/12 00:12:08, Jói wrote: > A comment on why this now gets set to false is in order; I believe it's correct > to do this based on how the IsEntryOutdated function works, but it's > non-obvious. Since this variable is mainly used for life span management, so I finally decide to change the comment in IsEntryOutdated(). Please let me know if it looks okay or not. > > Also, nit: This whole block could use a couple of lines of whitespace. Done. http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... File net/url_request/request_throttler_entry.h (right): http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_entry.h:21: // sent during the back-off period will be cancelled. Secondly, a sliding window On 2010/11/12 00:12:08, Jói wrote: > Secondly, a sliding... -> suggest replacement paragraph (and starting it as its > own paragraph): > > A sliding window is used to count recent requests to a given destination and > provide guidance (to the application level only) on whether too many requests > have been sent and when a good time to send the next one would be. This is never > used to deny requests at the network level. Done. http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... File net/url_request/request_throttler_entry_interface.h (right): http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_entry_interface.h:13: // Represents an entry of the request throttler manager. On 2010/11/12 00:12:08, Jói wrote: > change to: Interface provided on entries of the request throttler manager. Done. http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... File net/url_request/request_throttler_manager.h (right): http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_manager.h:24: // Please refer to request_throttler_entry.h for further informations. On 2010/11/12 00:12:08, Jói wrote: > request_throttler_entry.h -> request_throttler_entry_interface.h Done. http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_manager.h:28: // This method is used by higher level modules which can notify this class if On 2010/11/12 00:12:08, Jói wrote: > This comment is not clear enough. Suggest the following edit: > > Lets higher-level modules, that know how to parse particular response bodies, > notify of receiving malformed content for the given URL. This will be handled by > the throttler as if an HTTP 5xx response had been received to the request, i.e. > it will count as a failure. Thanks a lot! http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_manager.h:36: // any) for the URL. On 2010/11/12 00:12:08, Jói wrote: > if this is primarily for unit testing, we should say so It won't be for unit tests only, since I read the recent changes (those submitted after my last sync) and found that cloud printing needs to use this method once I sync to the tip of the tree and resolve the conflicts.
Hi Yuzhu, Took a closer look at the sliding window code and some of the overall logic, and I have a bunch of comments. Siggi, you did a lot of the initial review for malavv@ so if you have counter-opinions to what I'm suggesting, please chime in. Cheers, Jói http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... File net/url_request/request_throttler_entry.cc (right): http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:67: exponential_backoff_release_time_ = base::TimeTicks::Now(); On 2010/11/12 02:05:46, yzshen wrote: > On 2010/11/12 00:12:08, Jói wrote: > > suggest using GetTimeNow() > > Using GetTimeNow() here is a little bit confusing, since this method is called > in constructors, while in constructors virtual won't work. > > I added a comment. Is it okay? That's fine. http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:134: tracking_exponential_backoff_ = false; On 2010/11/12 02:05:46, yzshen wrote: > Since this variable is mainly used for life span management, so I finally decide > to change the comment in IsEntryOutdated(). Please let me know if it looks okay > or not. The confusing part is that the invariant of data (according to the name of the variable, and also the comment you added) that if tracking_exponential_backoff_ is false, then everything is fine - but in fact this variable gets set to false immediately on a successful request (in UpdateWithResponse) whereas we may still be "decaying" the number of times delayed - so tracking_exponential_backoff_ being false does not imply failure_count_ == 0. I think the documentation is probably OK as is though; perhaps in the .h file we could have a simple invariant for this member, something like: // If true, the last request response was a failure. Note // that this member can be false at the same time // as failure_count_ can be greater than 0. http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:90: It would help the readability of the code to explain in which case the sliding_window_release_time_ might become different (greater) than exponential_backoff_release_time_. Also, I wonder if there is really a need to differentiate? Should we just use the exponential_backoff_release_time_ for both? http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:99: base::TimeDelta sliding_window_period = base::TimeDelta::FromMilliseconds( It seems strange that send_log_ is only updated in GetRecommendedDelayForNextRequest, instead of updating it on every UpdateWithResponse, it seems the latter would give you much better coverage? http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:101: can we make the constant a base::TimeDelta instead of an integer number of milliseconds, to avoid the calculation every time? http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... File net/url_request/request_throttler_entry.h (right): http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_entry.h:121: OldValues old_values_; (Outdated comment because of my comment in the unit test file, but leaving here for posterity): Move the definition of the OldValues struct to right above this, to make the documentation of that struct more obviously cover this. Alternatively, document this as "See description of OldValues struct above." http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_entry.h:124: // allowed to start sending requests again. This comment makes it sound like we will deny requests within the period; maybe reword to something like: // Timestamp calculated by the sliding window algorithm // for when we advise clients the next request should be // made, at the earliest. Advisory only, not used to deny // requests. http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_entry.h:131: const int sliding_window_period_ms_; for any of these constants in milliseconds that get calculated over to a base::TimeDelta in the code, could we just store the constant as a base::TimeDelta instead? http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... File net/url_request/request_throttler_unittest.cc (right): http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_unittest.cc:260: // Then inform the entry that previous package was malformed, This, and the whole OldState / SaveState thing, seems bogus to me. It seems to assume that requests and responses are received in a tidy sequential fashion with no overlap. In fact there is no guarantee that the OldState that you saved is the state previous to processing the response that you are then told is malformed. I think ReceivedContentWasMalformed just needs to act as if there was a new error for the URL. No need to attempt to match it up with a previously handled request. This should simplify the code a bit.
Hi, Joi. Here are replies to part of the comments. It may take me some time (hours) to upload a new patch with all changes according to your comments, since I am resolving conflicts with recent changes to cloud printing. Thanks! http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... File net/url_request/request_throttler_entry.cc (right): http://codereview.chromium.org/4194001/diff/69001/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:134: tracking_exponential_backoff_ = false; On 2010/11/12 19:14:24, Jói wrote: > On 2010/11/12 02:05:46, yzshen wrote: > > Since this variable is mainly used for life span management, so I finally > decide > > to change the comment in IsEntryOutdated(). Please let me know if it looks > okay > > or not. > > The confusing part is that the invariant of data (according to the name of the > variable, and also the comment you added) that if tracking_exponential_backoff_ > is false, then everything is fine - but in fact this variable gets set to false > immediately on a successful request (in UpdateWithResponse) whereas we may still > be "decaying" the number of times delayed - so tracking_exponential_backoff_ > being false does not imply failure_count_ == 0. > > I think the documentation is probably OK as is though; perhaps in the .h file we > could have a simple invariant for this member, something like: > > // If true, the last request response was a failure. Note > // that this member can be false at the same time > // as failure_count_ can be greater than 0. I could change its name to latest_response_is_error_ and only use it for life span calculation. I think that is even more clear. http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:90: On 2010/11/12 19:14:24, Jói wrote: > It would help the readability of the code to explain in which case the > sliding_window_release_time_ might become different (greater) than > exponential_backoff_release_time_. If a lot of requests are successfully made in the recent time, sliding_window_release_time_ may be greater than exponential_backoff_release_time_. I have added a comment. > Also, I wonder if there is really a need to differentiate? Should we just use > the exponential_backoff_release_time_ for both? Yes, we need the two separate variables. Please consider the following situation: If recently there are a lot of (successful) requests, the sliding_window_release_time_ is pushed forward and probably greater than exponential_backoff_release_time_. If we merge sliding_window_release_time_ into exponential_backoff_release_time_. Since the network level will reject requests based on exponential_backoff_release_time_, we actually enforce the sliding window rule at the network level. http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:99: base::TimeDelta sliding_window_period = base::TimeDelta::FromMilliseconds( On 2010/11/12 19:14:24, Jói wrote: > It seems strange that send_log_ is only updated in > GetRecommendedDelayForNextRequest, instead of updating it on every > UpdateWithResponse, it seems the latter would give you much better coverage? I thought about this problem before I did it this way. If we update the send_log_ by UpdateWithResponse (or as my previous patch, by NotifyRequestStart on the network level), it has better coverage, but it is too late. Consider the following scenario: 1) [time_1] we know from UpdateWithResponse that according to the sliding window rule, it is better to make request after time_3. 2) [time_2] 100 upper layer modules would like to make request to the same URL, and thus call GetRecommendedDelayForNextRequest, they all get time_3. As a result, 100 tasks are posted to start at time_3. 3) [time_3] a lot of requests are started around this time. To avoid this problem, we need to reserve a sending time for each request at time_2, and spread the requests over time properly. http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... File net/url_request/request_throttler_unittest.cc (right): http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_unittest.cc:260: // Then inform the entry that previous package was malformed, On 2010/11/12 19:14:24, Jói wrote: > This, and the whole OldState / SaveState thing, seems bogus to me. It seems to > assume that requests and responses are received in a tidy sequential fashion > with no overlap. In fact there is no guarantee that the OldState that you saved > is the state previous to processing the response that you are then told is > malformed. > > I think ReceivedContentWasMalformed just needs to act as if there was a new > error for the URL. No need to attempt to match it up with a previously handled > request. This should simplify the code a bit. I agree with you. I will remove all the OldState/SaveState logic.
Hi, reviewers. I have resolved conflicts with CloudPrintURLFetcher and updated the CL. Please take a look. Thanks! Joi: I agree with you that we should reconsider the back-off condition (part of 5XX errors, timeout, etc.). However, since making such change may result in more design discussion and make the CL even bigger. I suggest that we do that in a separate CL, what do you think? Thanks! http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... File net/url_request/request_throttler_entry.cc (right): http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_entry.cc:101: base::TimeDelta sliding_window_period = base::TimeDelta::FromMilliseconds( On 2010/11/12 19:14:24, Jói wrote: > can we make the constant a base::TimeDelta instead of an integer number of > milliseconds, to avoid the calculation every time? Thanks! Done. http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... File net/url_request/request_throttler_entry.h (right): http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_entry.h:121: OldValues old_values_; On 2010/11/12 19:14:24, Jói wrote: > (Outdated comment because of my comment in the unit test file, but leaving here > for posterity): > > Move the definition of the OldValues struct to right above this, to make the > documentation of that struct more obviously cover this. Alternatively, document > this as "See description of OldValues struct above." Removed OldValues and will keep your suggestion in mind. http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_entry.h:124: // allowed to start sending requests again. On 2010/11/12 19:14:24, Jói wrote: > This comment makes it sound like we will deny requests within the period; maybe > reword to something like: > > // Timestamp calculated by the sliding window algorithm > // for when we advise clients the next request should be > // made, at the earliest. Advisory only, not used to deny > // requests. Done. http://codereview.chromium.org/4194001/diff/70022/net/url_request/request_thr... net/url_request/request_throttler_entry.h:131: const int sliding_window_period_ms_; On 2010/11/12 19:14:24, Jói wrote: > for any of these constants in milliseconds that get calculated over to a > base::TimeDelta in the code, could we just store the constant as a > base::TimeDelta instead? Done. I think only the first one is always used as TimeDelta.
I am going to be gone the next 2 days. apologies if I don't get to this until then.
LGTM > Joi: I agree with you that we should reconsider the back-off condition (part of > 5XX errors, timeout, etc.). However, since making such change may result in more > design discussion and make the CL even bigger. I suggest that we do that in a > separate CL, what do you think? Sounds good to me. http://codereview.chromium.org/4194001/diff/128003/chrome/common/net/url_fetc... File chrome/common/net/url_fetcher.cc (right): http://codereview.chromium.org/4194001/diff/128003/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:356: int64 back_off_time = entry->GetRecommendedDelayForNextRequest(); Doesn't this call to GetRecommendedDelayForNextRequest add a sent item to the sliding window? Is that what you intend? http://codereview.chromium.org/4194001/diff/128003/chrome/service/cloud_print... File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right): http://codereview.chromium.org/4194001/diff/128003/chrome/service/cloud_print... chrome/service/cloud_print/cloud_print_url_fetcher.cc:111: entry->GetRecommendedDelayForNextRequest()); Same comment here on the sliding window - doesn't this add an entry to it, that should have already been added when the request was initially sent?
I'm still going through this changelist. You should update the changelist description, especially the first line. Please refer to http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre.... Many tools only show the first line, and having your first line just say that it patches another changelist is pretty near useless. Also, either you or the changelist you patched have a ton of style guide violations. If you have not gotten a C++ readability review yet, please do so. I need to keep reading to get a higher level picture of the change, but one thing I'm going to look at for is how this global manager object is managed. If it can be accessed from a WorkerPool thread, then it will cause shutdown crashes since the AtExitManager may have run before then. Also, typically we hide the actual Singleton<> access in the code behind a GetInstance() call. Also, we are probably moving towards allowing multiple uses of the network stack (pull in libfoo and libbar which each use libnet, possibly in different threads), therefore this manager object shouldn't be global. It's probably better for it to be accessed via the URLRequestContext. Since it appears that it is already global, I won't require a refactor to eliminate this at this time, but I am going to try to prevent any additional thread-hostile behavior. I'm going to try to do a deeper read of the code later today. Sorry for being so slow :( http://codereview.chromium.org/4194001/diff/128003/chrome/service/cloud_print... File chrome/service/cloud_print/cloud_print_url_fetcher.h (right): http://codereview.chromium.org/4194001/diff/128003/chrome/service/cloud_print... chrome/service/cloud_print/cloud_print_url_fetcher.h:110: RequestThrottlerManager* request_throttler_manager_; Why are we caching the singleton into a pointer? http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... File net/url_request/request_throttler_entry.h (right): http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_entry.h:8: #include "net/url_request/request_throttler_entry_interface.h" http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... Move this one below the system headers. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_entry.h:26: class RequestThrottlerEntry : public RequestThrottlerEntryInterface { Why is this in the global namespace? In general, net code should be in the net namespace. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_entry.h:97: All member variables should be private. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Access_Control. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_entry.h:134: DISALLOW_COPY_AND_ASSIGN(RequestThrottlerEntry); You need basictypes.h for this macro. You're transitively pulling it in from somewhere else, but if someone refactors that header file to remove that include, this will break. You should include what you use. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... File net/url_request/request_throttler_entry_interface.h (right): http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_entry_interface.h:29: virtual int64 GetRecommendedDelayForNextRequest() = 0; You should include basictypes.h for the int64 definition. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_entry_interface.h:43: private: Please refer to http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_... which says: "Except for the first instance, these keywords should be preceded by a blank line. This rule is optional in small classes." I do not consider this a small class. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... File net/url_request/request_throttler_header_adapter.h (right): http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_header_adapter.h:18: const scoped_refptr<net::HttpResponseHeaders>& headers); http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre... says to pass in the raw pointer here. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... File net/url_request/request_throttler_manager.h (right): http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:33: const scoped_refptr<RequestThrottlerEntry>& entry); This should take a raw pointer rather than a const scoped_refptr<T>& http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:43: virtual ~RequestThrottlerManager(); Why is the destructor virtual? I don't see virtual methods. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:59: typedef std::map<std::string, scoped_refptr<RequestThrottlerEntry> > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... says types should go at the beginning of the section. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:66: UrlEntryMap url_entries_; http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_... says to make this private. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:81: DISALLOW_COPY_AND_ASSIGN(RequestThrottlerManager); Include basictypes.h for this macro.
I'm still going through this, but I started looking at Eric's comments, and it looks like his instincts jive with mine on this. The locking seems to be unnecessary. We should be trying to make all accesses happen on the IO thread. This is consistent with almost all other net/ code. It will make the code easier to maintain in the future if we don't need to use locks. The one place where I saw that it needed to be accessed in a separate thread seemed to be where the URLFetcher proxies to the IO message loop. For that case, why can't we proxy to a wrapper function that accesses the RequestThrottlerEntry on the IO message loop and then invokes StartURLRequest() with the appropriate parameter? http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... File net/url_request/request_throttler_manager.cc (right): http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.cc:17: AutoLock auto_lock(lock_); Why do we need all this locking? Shouldn't the IO thread be the only thing that accesses the RequestThrottleManager? http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... File net/url_request/request_throttler_manager.h (right): http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:19: // their URLs in this request throttler manager on each request. This comment is insufficient. It doesn't describe the garbage collection at all. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:31: // It is only used by unit tests. I think I'd prefer that this be named OverrideEntryForTests() or something if it's only used by unit tests. I'd worry about someone not reading the comments and using this function. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:39: void EraseEntry(const GURL& url); Can you name this EraseEntryForTests()? I read this for the first time in the code and it wasn't obvious to me it was only for tests.
Hi, reviewers. I have removed all locks and made sure RequestThrottler* are only accessed on the IO thread. Please take another look. Thanks! willchan: > I'm still going through this changelist. You should > update the changelist > description, especially the first line. Thanks! I have done so. > Also, either you or the changelist you patched have a ton > of style guide > violations. If you have not gotten a C++ readability > review yet, please do so. Sorry I haven't caught all of them. (And I introduced some of them myself.) Thanks for pointing them out. > Also, typically we hide the actual Singleton<> access in > the code behind a > GetInstance() call. Done. > Also, we are probably moving towards allowing multiple > uses of the network stack > (pull in libfoo and libbar which each use libnet, > possibly in different > threads), therefore this manager object shouldn't be > global. It's probably > better for it to be accessed via the URLRequestContext. > Since it appears that > it is already global, I won't require a refactor to > eliminate this at this time, > but I am going to try to prevent any additional thread- > hostile behavior. Sounds good to me. I think I could create a separate CL for it. Thanks! http://codereview.chromium.org/4194001/diff/128003/chrome/common/net/url_fetc... File chrome/common/net/url_fetcher.cc (right): http://codereview.chromium.org/4194001/diff/128003/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:356: int64 back_off_time = entry->GetRecommendedDelayForNextRequest(); On 2010/11/17 16:39:18, Jói wrote: > Doesn't this call to GetRecommendedDelayForNextRequest add a sent item to the > sliding window? Is that what you intend? Since every retry is a new request, I think we need to spread them over time properly, by reserving them in the sliding window. However, the code does have some problems, because sometimes it reserves a send item in the sliding window while not using it. I have fixed it. http://codereview.chromium.org/4194001/diff/128003/chrome/service/cloud_print... File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right): http://codereview.chromium.org/4194001/diff/128003/chrome/service/cloud_print... chrome/service/cloud_print/cloud_print_url_fetcher.cc:111: entry->GetRecommendedDelayForNextRequest()); On 2010/11/17 16:39:18, Jói wrote: > Same comment here on the sliding window - doesn't this add an entry to it, that > should have already been added when the request was initially sent? I realize that we don't even need to wait here, since the underlying URLFetcher will handle it. Thanks for pointing it out! http://codereview.chromium.org/4194001/diff/128003/chrome/service/cloud_print... File chrome/service/cloud_print/cloud_print_url_fetcher.h (right): http://codereview.chromium.org/4194001/diff/128003/chrome/service/cloud_print... chrome/service/cloud_print/cloud_print_url_fetcher.h:110: RequestThrottlerManager* request_throttler_manager_; On 2010/11/17 20:32:34, willchan wrote: > Why are we caching the singleton into a pointer? I thought it was right since I read this in singleton.h: "Every call to get(), operator->() and operator*() incurs some overhead... You may wish to cache the result of get(); it will not change." I've changed it. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... File net/url_request/request_throttler_entry.h (right): http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_entry.h:8: #include "net/url_request/request_throttler_entry_interface.h" On 2010/11/17 20:32:34, willchan wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... > > Move this one below the system headers. Done. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_entry.h:26: class RequestThrottlerEntry : public RequestThrottlerEntryInterface { On 2010/11/17 20:32:34, willchan wrote: > Why is this in the global namespace? In general, net code should be in the net > namespace. Since URLRequest/URLRequest.*Job are not in the net namespace, I thought it was okay. Please let me know if you think I need to change it. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_entry.h:97: On 2010/11/17 20:32:34, willchan wrote: > All member variables should be private. See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Access_Control. Thanks! Will keep in mind. :) http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_entry.h:134: DISALLOW_COPY_AND_ASSIGN(RequestThrottlerEntry); On 2010/11/17 20:32:34, willchan wrote: > You need basictypes.h for this macro. You're transitively pulling it in from > somewhere else, but if someone refactors that header file to remove that > include, this will break. You should include what you use. Done. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... File net/url_request/request_throttler_entry_interface.h (right): http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_entry_interface.h:29: virtual int64 GetRecommendedDelayForNextRequest() = 0; On 2010/11/17 20:32:34, willchan wrote: > You should include basictypes.h for the int64 definition. Done. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_entry_interface.h:43: private: On 2010/11/17 20:32:34, willchan wrote: > Please refer to > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_... > which says: "Except for the first instance, these keywords should be preceded by > a blank line. This rule is optional in small classes." I do not consider this a > small class. Done. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... File net/url_request/request_throttler_header_adapter.h (right): http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_header_adapter.h:18: const scoped_refptr<net::HttpResponseHeaders>& headers); On 2010/11/17 20:32:34, willchan wrote: > http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre... > says to pass in the raw pointer here. Done. Thanks! http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... File net/url_request/request_throttler_manager.cc (right): http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.cc:17: AutoLock auto_lock(lock_); On 2010/11/18 06:47:47, willchan wrote: > Why do we need all this locking? Shouldn't the IO thread be the only thing that > accesses the RequestThrottleManager? I have eliminated all locks. Thanks! http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... File net/url_request/request_throttler_manager.h (right): http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:19: // their URLs in this request throttler manager on each request. On 2010/11/18 06:47:47, willchan wrote: > This comment is insufficient. It doesn't describe the garbage collection at > all. Done. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:31: // It is only used by unit tests. On 2010/11/18 06:47:47, willchan wrote: > I think I'd prefer that this be named OverrideEntryForTests() or something if > it's only used by unit tests. I'd worry about someone not reading the comments > and using this function. Done. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:33: const scoped_refptr<RequestThrottlerEntry>& entry); On 2010/11/17 20:32:34, willchan wrote: > This should take a raw pointer rather than a const scoped_refptr<T>& Done. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:39: void EraseEntry(const GURL& url); On 2010/11/18 06:47:47, willchan wrote: > Can you name this EraseEntryForTests()? I read this for the first time in the > code and it wasn't obvious to me it was only for tests. Done. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:43: virtual ~RequestThrottlerManager(); On 2010/11/17 20:32:34, willchan wrote: > Why is the destructor virtual? I don't see virtual methods. Done. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:59: typedef std::map<std::string, scoped_refptr<RequestThrottlerEntry> > On 2010/11/17 20:32:34, willchan wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... > says types should go at the beginning of the section. Done. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:66: UrlEntryMap url_entries_; On 2010/11/17 20:32:34, willchan wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Class_... > says to make this private. Done. http://codereview.chromium.org/4194001/diff/128003/net/url_request/request_th... net/url_request/request_throttler_manager.h:81: DISALLOW_COPY_AND_ASSIGN(RequestThrottlerManager); On 2010/11/17 20:32:34, willchan wrote: > Include basictypes.h for this macro. Done.
Mostly LGTM, I have a few nits and a couple of questions. Nice work removing the locking. http://codereview.chromium.org/4194001/diff/185002/chrome/common/net/url_fetc... File chrome/common/net/url_fetcher.cc (right): http://codereview.chromium.org/4194001/diff/185002/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:92: void StartURLRequest(); It might be more readable to name this one StartURLRequestWhenAllowed (as opposed to StartURLRequestNow). http://codereview.chromium.org/4194001/diff/185002/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:132: scoped_refptr<RequestThrottlerEntryInterface> original_url_throttler_entry_; It's non-obvious why you need two (entry vs. original entry); perhaps a line or two here would help make it clearer. http://codereview.chromium.org/4194001/diff/185002/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:145: base::TimeTicks backoff_release_time_; I only see this set in Core::OnReadCompleted but it's used in Core::OnCompletedURLRequest without even checking if it's 0. Forgive my lack of deep understanding of URLFetcher, but is it guaranteed that OnReadCompleted gets called before OnCompletedURLRequest ? http://codereview.chromium.org/4194001/diff/185002/net/url_request/request_th... File net/url_request/request_throttler_entry.cc (right): http://codereview.chromium.org/4194001/diff/185002/net/url_request/request_th... net/url_request/request_throttler_entry.cc:186: failure_count_ += 2; Why it's +2 deserves documentation. http://codereview.chromium.org/4194001/diff/185002/net/url_request/request_th... File net/url_request/request_throttler_manager.h (right): http://codereview.chromium.org/4194001/diff/185002/net/url_request/request_th... net/url_request/request_throttler_manager.h:26: // NOTE: All usage of the singleton object of this class should be on the same Can we / should we assert on this in debug builds, e.g. keep the first thread ID seen accessing the class and assert that subsequent usages are from the same thread?
Thanks, Joi! http://codereview.chromium.org/4194001/diff/185002/chrome/common/net/url_fetc... File chrome/common/net/url_fetcher.cc (right): http://codereview.chromium.org/4194001/diff/185002/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:92: void StartURLRequest(); On 2010/11/21 19:31:40, Jói wrote: > It might be more readable to name this one StartURLRequestWhenAllowed (as > opposed to StartURLRequestNow). Done. Since the method tries to decide an appropriate sending time, taking into account the exponential back-off time as well as the sliding window (which is not enforced by the network level), so I decided to call it StartURLRequestWhenAppropriate. http://codereview.chromium.org/4194001/diff/185002/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:132: scoped_refptr<RequestThrottlerEntryInterface> original_url_throttler_entry_; On 2010/11/21 19:31:40, Jói wrote: > It's non-obvious why you need two (entry vs. original entry); perhaps a line or > two here would help make it clearer. Done. http://codereview.chromium.org/4194001/diff/185002/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:145: base::TimeTicks backoff_release_time_; On 2010/11/21 19:31:40, Jói wrote: > I only see this set in Core::OnReadCompleted but it's used in > Core::OnCompletedURLRequest without even checking if it's 0. Forgive my lack of > deep understanding of URLFetcher, but is it guaranteed that OnReadCompleted gets > called before OnCompletedURLRequest ? Since OnCompletedURLRequest is only called by OnReadCompleted, I thought it's okay. http://codereview.chromium.org/4194001/diff/185002/net/url_request/request_th... File net/url_request/request_throttler_entry.cc (right): http://codereview.chromium.org/4194001/diff/185002/net/url_request/request_th... net/url_request/request_throttler_entry.cc:186: failure_count_ += 2; On 2010/11/21 19:31:40, Jói wrote: > Why it's +2 deserves documentation. Done. I have commented the reason why it is +2 here and the limitation (it may lead to a larger failure count than expected). If we need a more precise algorithm to solve this limitation, we probably need a data structure of O(n) size to record the minus-1-or-not information for successful responses and their order. I finally decided that the cost outweighed the gain. Please let me know if you don't think so. Thanks! :) http://codereview.chromium.org/4194001/diff/185002/net/url_request/request_th... File net/url_request/request_throttler_manager.h (right): http://codereview.chromium.org/4194001/diff/185002/net/url_request/request_th... net/url_request/request_throttler_manager.h:26: // NOTE: All usage of the singleton object of this class should be on the same On 2010/11/21 19:31:40, Jói wrote: > Can we / should we assert on this in debug builds, e.g. keep the first thread ID > seen accessing the class and assert that subsequent usages are from the same > thread? I tried to do so. However, the unit tests use different threads as the IO thread. They won't access the request throttler module concurrently, so writing the request throttler module in a non-thread-safe way has no problem. But they prevent me from adding the checks as you suggested. I think this is another reason that we don't like RequestThrottlerManager to be a singleton. I will change this in a separate CL, and add the thread checks. Does it sound okay?
Almost LGTM for the cloud print code. A couple of minor issues. http://codereview.chromium.org/4194001/diff/224001/chrome/service/cloud_print... File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right): http://codereview.chromium.org/4194001/diff/224001/chrome/service/cloud_print... chrome/service/cloud_print/cloud_print_url_fetcher.cc:104: StartRequestNow(); We don't need StartRequestNow as a separate method any longer. http://codereview.chromium.org/4194001/diff/224001/chrome/service/cloud_print... File chrome/service/cloud_print/cloud_print_url_fetcher.h (right): http://codereview.chromium.org/4194001/diff/224001/chrome/service/cloud_print... chrome/service/cloud_print/cloud_print_url_fetcher.h:23: : public base::RefCountedThreadSafe<CloudPrintURLFetcher>, Now that we don't post tasks to this object, perhaps it does not need to implement RefCountedThreadSafe any longer.
LGTM http://codereview.chromium.org/4194001/diff/185002/chrome/common/net/url_fetc... File chrome/common/net/url_fetcher.cc (right): http://codereview.chromium.org/4194001/diff/185002/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:145: base::TimeTicks backoff_release_time_; On 2010/11/22 17:35:01, yzshen wrote: > On 2010/11/21 19:31:40, Jói wrote: > > I only see this set in Core::OnReadCompleted but it's used in > > Core::OnCompletedURLRequest without even checking if it's 0. Forgive my lack > of > > deep understanding of URLFetcher, but is it guaranteed that OnReadCompleted > gets > > called before OnCompletedURLRequest ? > > Since OnCompletedURLRequest is only called by OnReadCompleted, I thought it's > okay. OK. http://codereview.chromium.org/4194001/diff/185002/net/url_request/request_th... File net/url_request/request_throttler_entry.cc (right): http://codereview.chromium.org/4194001/diff/185002/net/url_request/request_th... net/url_request/request_throttler_entry.cc:186: failure_count_ += 2; On 2010/11/22 17:35:01, yzshen wrote: > On 2010/11/21 19:31:40, Jói wrote: > > Why it's +2 deserves documentation. > > Done. > > I have commented the reason why it is +2 here and the limitation (it may lead to > a larger failure count than expected). > If we need a more precise algorithm to solve this limitation, we probably need a > data structure of O(n) size to record the minus-1-or-not information for > successful responses and their order. I finally decided that the cost outweighed > the gain. Please let me know if you don't think so. Thanks! :) Looks good. I don't see a problem with this approach. http://codereview.chromium.org/4194001/diff/185002/net/url_request/request_th... File net/url_request/request_throttler_manager.h (right): http://codereview.chromium.org/4194001/diff/185002/net/url_request/request_th... net/url_request/request_throttler_manager.h:26: // NOTE: All usage of the singleton object of this class should be on the same On 2010/11/22 17:35:01, yzshen wrote: > On 2010/11/21 19:31:40, Jói wrote: > > Can we / should we assert on this in debug builds, e.g. keep the first thread > ID > > seen accessing the class and assert that subsequent usages are from the same > > thread? > > I tried to do so. However, the unit tests use different threads as the IO > thread. > > They won't access the request throttler module concurrently, so writing the > request throttler module in a non-thread-safe way has no problem. But they > prevent me from adding the checks as you suggested. > > I think this is another reason that we don't like RequestThrottlerManager to be > a singleton. I will change this in a separate CL, and add the thread checks. > > Does it sound okay? Sounds good.
http://codereview.chromium.org/4194001/diff/224001/chrome/service/cloud_print... File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right): http://codereview.chromium.org/4194001/diff/224001/chrome/service/cloud_print... chrome/service/cloud_print/cloud_print_url_fetcher.cc:104: StartRequestNow(); On 2010/11/22 19:09:26, sanjeevr wrote: > We don't need StartRequestNow as a separate method any longer. Done. http://codereview.chromium.org/4194001/diff/224001/chrome/service/cloud_print... File chrome/service/cloud_print/cloud_print_url_fetcher.h (right): http://codereview.chromium.org/4194001/diff/224001/chrome/service/cloud_print... chrome/service/cloud_print/cloud_print_url_fetcher.h:23: : public base::RefCountedThreadSafe<CloudPrintURLFetcher>, On 2010/11/22 19:09:26, sanjeevr wrote: > Now that we don't post tasks to this object, perhaps it does not need to > implement RefCountedThreadSafe any longer. I saw the following code in CloudPrintURLFetcher::OnURLFetchComplete: scoped_refptr<CloudPrintURLFetcher> keep_alive(this); I guess declaring this class as ref-counted serves two purposes: 1) we want to support posted tasks; 2) by doing this, the delegate can abandon its reference to the fetcher object within the delegate methods: ResponseAction SomeDelegate::HandleRawData(...) { ... // request_ is scoped_refptr<CloudPrintURLFetche>, // the following operation is safe. request_ = NULL; } However, if we remove the base class RefCountedThreadSafe, we cannot do the same thing anymore: ResponseAction SomeDelegate::HandleRawData(...) { ... // request_ is scoped_ptr<CloudPrintURLFetche>, // the following operation will crash. request_.reset(NULL); } Since I am not very familiar with the context and this part of codebase, I would like to leave it as it is if possible.
On 2010/11/22 22:59:17, yzshen wrote: > http://codereview.chromium.org/4194001/diff/224001/chrome/service/cloud_print... > File chrome/service/cloud_print/cloud_print_url_fetcher.cc (right): > > http://codereview.chromium.org/4194001/diff/224001/chrome/service/cloud_print... > chrome/service/cloud_print/cloud_print_url_fetcher.cc:104: StartRequestNow(); > On 2010/11/22 19:09:26, sanjeevr wrote: > > We don't need StartRequestNow as a separate method any longer. > > Done. > > http://codereview.chromium.org/4194001/diff/224001/chrome/service/cloud_print... > File chrome/service/cloud_print/cloud_print_url_fetcher.h (right): > > http://codereview.chromium.org/4194001/diff/224001/chrome/service/cloud_print... > chrome/service/cloud_print/cloud_print_url_fetcher.h:23: : public > base::RefCountedThreadSafe<CloudPrintURLFetcher>, > On 2010/11/22 19:09:26, sanjeevr wrote: > > Now that we don't post tasks to this object, perhaps it does not need to > > implement RefCountedThreadSafe any longer. > > I saw the following code in CloudPrintURLFetcher::OnURLFetchComplete: > > scoped_refptr<CloudPrintURLFetcher> keep_alive(this); > > I guess declaring this class as ref-counted serves two purposes: > 1) we want to support posted tasks; > 2) by doing this, the delegate can abandon its reference to the fetcher object > within the delegate methods: > > ResponseAction SomeDelegate::HandleRawData(...) { > ... > // request_ is scoped_refptr<CloudPrintURLFetche>, > // the following operation is safe. > request_ = NULL; > } > > However, if we remove the base class RefCountedThreadSafe, we cannot do the same > thing anymore: > > ResponseAction SomeDelegate::HandleRawData(...) { > ... > // request_ is scoped_ptr<CloudPrintURLFetche>, > // the following operation will crash. > request_.reset(NULL); > } > > Since I am not very familiar with the context and this part of codebase, I would > like to leave it as it is if possible. Sounds fine. I can change it later. LGTM.
I consulted with other networking folks. You need to rename your classes. Everything in net/ should be in namespace net. And RequestThrottler.* is to general a name. We have a lot of different kinds of requests in the network stack. If this only applies to URLRequests, then name it URLRequestThrottler.*. The memory management is done incorrectly. Refcounting is the wrong tool here. I don't want to be too harsh about it, but you should think harder next time about whether or not to use refcounting. It should be the last option you employ, only when ownership is truly shared. The Google C++ primer (internal to Google) strongly recommends against it. Please fix the memory management. http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... File chrome/common/net/url_fetcher.cc (right): http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:58: void ReceivedContentWasMalformed(); Is this new functionality? I don't see any mention of this functionality in the changelist description. http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:402: if (response_code_ >= 500 || Can you explain why this checking happens on the delegate thread? It seems like if we haven't maxed out on retries, we should stay on the IO thread. We should only proxy to the delegate thread when we need to notify the delegate. http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:403: status.os_error() == net::ERR_TEMPORARILY_THROTTLED_BY_DDOS) { I think I'd rather this error be called ERR_TEMPORARILY_THROTTLED or ERR_TEMPORARY_BACK_OFF. Saying BY_DDOS makes it sound like the client is getting DDoS'd. http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... File chrome/common/net/url_fetcher_unittest.cc (right): http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... chrome/common/net/url_fetcher_unittest.cc:315: fetcher_->set_max_retries(11); 11 is a magic number. Does this have any specific meaning? Perhaps you can use a named constant instead. http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... File net/url_request/request_throttler_manager.cc (right): http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_manager.cc:41: std::string RequestThrottlerManager::GetIdFromUrl(const GURL& url) { Why are you not using the GURL canonicalization routines? What if the URL has a port or query or username or password or what not? The port especially seems important. It seems to me that you should use url_canon::Replacements to generate a new GURL and use .spec() to convert. http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... File net/url_request/request_throttler_manager.h (right): http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_manager.h:27: // thread. Please use ThreadChecker to enforce this. http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_manager.h:36: scoped_refptr<RequestThrottlerEntryInterface> RegisterRequestUrl( You're using ref counted RequestThrottlerEntryInterfaces. Please document somewhere the intended ownership semantics of this object. How are they supposed to ever get deleted? It looks like RequestThrottlerManager doesn't expose any non-test interface for unregistering a RequestThrottlerEntry. How will the RequestThrottlerManager ever release its reference? Oh I see, you use garbage collection. That's fine. It doesn't explain why you're using reference counting. Let's think about this conceptually, shouldn't RequestThrottlerManager be owning these entries? From what I can tell, you're using reference counting because you don't know who is holding onto the entry still. Is that correct? This sounds suboptimal. The manager shouldn't try to garbage collect an entry that is "active", where "active" means that a URLFetcher or something is holding onto it. Rather than refcounting the entry, you should let clients register themselves with the entry. And make them unregister themselves from the entry too. The RequestThrottlerManager should own the entry and should make sure to check for other references to the entry, or should cancel their references via base::WeakPtr or something like that. Please think REAL HARD whenever you use RefCounted*. It is usually the wrong thing to use.
Hi Will, Thanks for doing such a thorough review. Please don't be too hard on Yuzhu, he is landing the patch at my request, most of it was written this summer by an intern of mine, and the original patch had been through multiple rounds of code reviews - so it's normal that he should expect that issues at the level you are addressing (ref counting, namespacing etc.) would have been worked out then. I'm the one responsible for the original change, and I should have paid closer attention to it when it was originally implemented and went through code review. I agree it sounds like refcounting is not necessary in this instance, and I should have realized it then. Cheers, Jói On Tue, Nov 23, 2010 at 7:23 PM, <willchan@chromium.org> wrote: > I consulted with other networking folks. You need to rename your classes. > Everything in net/ should be in namespace net. And RequestThrottler.* is to > general a name. We have a lot of different kinds of requests in the network > stack. If this only applies to URLRequests, then name it > URLRequestThrottler.*. > > The memory management is done incorrectly. Refcounting is the wrong tool > here. > I don't want to be too harsh about it, but you should think harder next time > about whether or not to use refcounting. It should be the last option you > employ, only when ownership is truly shared. The Google C++ primer > (internal to > Google) strongly recommends against it. Please fix the memory management. > > > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... > File chrome/common/net/url_fetcher.cc (right): > > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... > chrome/common/net/url_fetcher.cc:58: void ReceivedContentWasMalformed(); > Is this new functionality? I don't see any mention of this > functionality in the changelist description. > > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... > chrome/common/net/url_fetcher.cc:402: if (response_code_ >= 500 || > Can you explain why this checking happens on the delegate thread? It > seems like if we haven't maxed out on retries, we should stay on the IO > thread. We should only proxy to the delegate thread when we need to > notify the delegate. > > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... > chrome/common/net/url_fetcher.cc:403: status.os_error() == > net::ERR_TEMPORARILY_THROTTLED_BY_DDOS) { > I think I'd rather this error be called ERR_TEMPORARILY_THROTTLED or > ERR_TEMPORARY_BACK_OFF. Saying BY_DDOS makes it sound like the client > is getting DDoS'd. > > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... > File chrome/common/net/url_fetcher_unittest.cc (right): > > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... > chrome/common/net/url_fetcher_unittest.cc:315: > fetcher_->set_max_retries(11); > 11 is a magic number. Does this have any specific meaning? Perhaps you > can use a named constant instead. > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > File net/url_request/request_throttler_manager.cc (right): > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > net/url_request/request_throttler_manager.cc:41: std::string > RequestThrottlerManager::GetIdFromUrl(const GURL& url) { > Why are you not using the GURL canonicalization routines? What if the > URL has a port or query or username or password or what not? The port > especially seems important. It seems to me that you should use > url_canon::Replacements to generate a new GURL and use .spec() to > convert. > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > File net/url_request/request_throttler_manager.h (right): > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > net/url_request/request_throttler_manager.h:27: // thread. > Please use ThreadChecker to enforce this. > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > net/url_request/request_throttler_manager.h:36: > scoped_refptr<RequestThrottlerEntryInterface> RegisterRequestUrl( > You're using ref counted RequestThrottlerEntryInterfaces. Please > document somewhere the intended ownership semantics of this object. How > are they supposed to ever get deleted? It looks like > RequestThrottlerManager doesn't expose any non-test interface for > unregistering a RequestThrottlerEntry. How will the > RequestThrottlerManager ever release its reference? > > Oh I see, you use garbage collection. That's fine. It doesn't explain > why you're using reference counting. Let's think about this > conceptually, shouldn't RequestThrottlerManager be owning these entries? > From what I can tell, you're using reference counting because you don't > know who is holding onto the entry still. Is that correct? This sounds > suboptimal. The manager shouldn't try to garbage collect an entry that > is "active", where "active" means that a URLFetcher or something is > holding onto it. Rather than refcounting the entry, you should let > clients register themselves with the entry. And make them unregister > themselves from the entry too. The RequestThrottlerManager should own > the entry and should make sure to check for other references to the > entry, or should cancel their references via base::WeakPtr or something > like that. > > Please think REAL HARD whenever you use RefCounted*. It is usually the > wrong thing to use. > > http://codereview.chromium.org/4194001/ >
Yeah, I realized it's not yzshen's fault, since he inherited the code. I realize my tone was very harsh when I wrote the inline comments, so I tried to soften it a bit when I wrote the meta-comment at the end. But I do want to emphasize the importance of appropriate memory management. I'm spending the next X quarters undoing most of the net/ refcounting to fix a gazillion bugs/crashes, so I take every opportunity to emphasize in code reviews that refcounting is usually the wrong choice. I'm also at fault here, because you guys cc'd me and eroman earlier on when the intern wrote the code and I didn't spend the time then to review it thoroughly. So sorry for coming down on Yuzhu. I know it's not his fault. But I'm still going to be a hardass :P Please don't take it personally, because it definitely isn't. Thanks for taking on this changelist Yuzhu. On Tue, Nov 23, 2010 at 4:28 PM, Jói Sigurðsson <joi@google.com> wrote: > Hi Will, > > Thanks for doing such a thorough review. Please don't be too hard on > Yuzhu, he is landing the patch at my request, most of it was written > this summer by an intern of mine, and the original patch had been > through multiple rounds of code reviews - so it's normal that he > should expect that issues at the level you are addressing (ref > counting, namespacing etc.) would have been worked out then. > > I'm the one responsible for the original change, and I should have > paid closer attention to it when it was originally implemented and > went through code review. I agree it sounds like refcounting is not > necessary in this instance, and I should have realized it then. > > Cheers, > Jói > > > On Tue, Nov 23, 2010 at 7:23 PM, <willchan@chromium.org> wrote: > > I consulted with other networking folks. You need to rename your > classes. > > Everything in net/ should be in namespace net. And RequestThrottler.* is > to > > general a name. We have a lot of different kinds of requests in the > network > > stack. If this only applies to URLRequests, then name it > > URLRequestThrottler.*. > > > > The memory management is done incorrectly. Refcounting is the wrong tool > > here. > > I don't want to be too harsh about it, but you should think harder next > time > > about whether or not to use refcounting. It should be the last option > you > > employ, only when ownership is truly shared. The Google C++ primer > > (internal to > > Google) strongly recommends against it. Please fix the memory > management. > > > > > > > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... > > File chrome/common/net/url_fetcher.cc (right): > > > > > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... > > chrome/common/net/url_fetcher.cc:58: void ReceivedContentWasMalformed(); > > Is this new functionality? I don't see any mention of this > > functionality in the changelist description. > > > > > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... > > chrome/common/net/url_fetcher.cc:402: if (response_code_ >= 500 || > > Can you explain why this checking happens on the delegate thread? It > > seems like if we haven't maxed out on retries, we should stay on the IO > > thread. We should only proxy to the delegate thread when we need to > > notify the delegate. > > > > > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... > > chrome/common/net/url_fetcher.cc:403: status.os_error() == > > net::ERR_TEMPORARILY_THROTTLED_BY_DDOS) { > > I think I'd rather this error be called ERR_TEMPORARILY_THROTTLED or > > ERR_TEMPORARY_BACK_OFF. Saying BY_DDOS makes it sound like the client > > is getting DDoS'd. > > > > > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... > > File chrome/common/net/url_fetcher_unittest.cc (right): > > > > > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... > > chrome/common/net/url_fetcher_unittest.cc:315: > > fetcher_->set_max_retries(11); > > 11 is a magic number. Does this have any specific meaning? Perhaps you > > can use a named constant instead. > > > > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > > File net/url_request/request_throttler_manager.cc (right): > > > > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > > net/url_request/request_throttler_manager.cc:41: std::string > > RequestThrottlerManager::GetIdFromUrl(const GURL& url) { > > Why are you not using the GURL canonicalization routines? What if the > > URL has a port or query or username or password or what not? The port > > especially seems important. It seems to me that you should use > > url_canon::Replacements to generate a new GURL and use .spec() to > > convert. > > > > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > > File net/url_request/request_throttler_manager.h (right): > > > > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > > net/url_request/request_throttler_manager.h:27: // thread. > > Please use ThreadChecker to enforce this. > > > > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > > net/url_request/request_throttler_manager.h:36: > > scoped_refptr<RequestThrottlerEntryInterface> RegisterRequestUrl( > > You're using ref counted RequestThrottlerEntryInterfaces. Please > > document somewhere the intended ownership semantics of this object. How > > are they supposed to ever get deleted? It looks like > > RequestThrottlerManager doesn't expose any non-test interface for > > unregistering a RequestThrottlerEntry. How will the > > RequestThrottlerManager ever release its reference? > > > > Oh I see, you use garbage collection. That's fine. It doesn't explain > > why you're using reference counting. Let's think about this > > conceptually, shouldn't RequestThrottlerManager be owning these entries? > > From what I can tell, you're using reference counting because you don't > > know who is holding onto the entry still. Is that correct? This sounds > > suboptimal. The manager shouldn't try to garbage collect an entry that > > is "active", where "active" means that a URLFetcher or something is > > holding onto it. Rather than refcounting the entry, you should let > > clients register themselves with the entry. And make them unregister > > themselves from the entry too. The RequestThrottlerManager should own > > the entry and should make sure to check for other references to the > > entry, or should cancel their references via base::WeakPtr or something > > like that. > > > > Please think REAL HARD whenever you use RefCounted*. It is usually the > > wrong thing to use. > > > > http://codereview.chromium.org/4194001/ > > >
Hi, William and Joi. Thanks for all your help and patience! William: I knew you were a hardass long before this CL. :) I always think that it takes a lot of effort for the reviewer to be "picky" and I really appreciate that. Answering difficult questions during code reviews is much much better than fixing a ton of bugs later. So, no offense taken. And I will ask for your favor next time for reviewing my other CLs, since you really care about code quality. I know there are still things to improve. However, I wonder whether the new memory management model issue could be saved to a later CL. Since I am blocking the TBLF team, and also Sanjeev, who is waiting for me to check in this CL in this week. If you do think it is a must to make the change in this CL. Sure I will do it. But please help me to do code reviews during the holiday. :) On Tue, Nov 23, 2010 at 4:50 PM, William Chan (陈智昌) <willchan@chromium.org>wrote: > Yeah, I realized it's not yzshen's fault, since he inherited the code. I > realize my tone was very harsh when I wrote the inline comments, so I tried > to soften it a bit when I wrote the meta-comment at the end. But I do want > to emphasize the importance of appropriate memory management. I'm spending > the next X quarters undoing most of the net/ refcounting to fix a gazillion > bugs/crashes, so I take every opportunity to emphasize in code reviews that > refcounting is usually the wrong choice. > > I'm also at fault here, because you guys cc'd me and eroman earlier on when > the intern wrote the code and I didn't spend the time then to review it > thoroughly. > > So sorry for coming down on Yuzhu. I know it's not his fault. But I'm > still going to be a hardass :P Please don't take it personally, because it > definitely isn't. Thanks for taking on this changelist Yuzhu. > > > On Tue, Nov 23, 2010 at 4:28 PM, Jói Sigurðsson <joi@google.com> wrote: > >> Hi Will, >> >> Thanks for doing such a thorough review. Please don't be too hard on >> Yuzhu, he is landing the patch at my request, most of it was written >> this summer by an intern of mine, and the original patch had been >> through multiple rounds of code reviews - so it's normal that he >> should expect that issues at the level you are addressing (ref >> counting, namespacing etc.) would have been worked out then. >> >> I'm the one responsible for the original change, and I should have >> paid closer attention to it when it was originally implemented and >> went through code review. I agree it sounds like refcounting is not >> necessary in this instance, and I should have realized it then. >> >> Cheers, >> Jói >> >> >> On Tue, Nov 23, 2010 at 7:23 PM, <willchan@chromium.org> wrote: >> > I consulted with other networking folks. You need to rename your >> classes. >> > Everything in net/ should be in namespace net. And RequestThrottler.* >> is to >> > general a name. We have a lot of different kinds of requests in the >> network >> > stack. If this only applies to URLRequests, then name it >> > URLRequestThrottler.*. >> > >> > The memory management is done incorrectly. Refcounting is the wrong >> tool >> > here. >> > I don't want to be too harsh about it, but you should think harder next >> time >> > about whether or not to use refcounting. It should be the last option >> you >> > employ, only when ownership is truly shared. The Google C++ primer >> > (internal to >> > Google) strongly recommends against it. Please fix the memory >> management. >> > >> > >> > >> http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >> > File chrome/common/net/url_fetcher.cc (right): >> > >> > >> http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >> > chrome/common/net/url_fetcher.cc:58: void ReceivedContentWasMalformed(); >> > Is this new functionality? I don't see any mention of this >> > functionality in the changelist description. >> > >> > >> http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >> > chrome/common/net/url_fetcher.cc:402: if (response_code_ >= 500 || >> > Can you explain why this checking happens on the delegate thread? It >> > seems like if we haven't maxed out on retries, we should stay on the IO >> > thread. We should only proxy to the delegate thread when we need to >> > notify the delegate. >> > >> > >> http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >> > chrome/common/net/url_fetcher.cc:403: status.os_error() == >> > net::ERR_TEMPORARILY_THROTTLED_BY_DDOS) { >> > I think I'd rather this error be called ERR_TEMPORARILY_THROTTLED or >> > ERR_TEMPORARY_BACK_OFF. Saying BY_DDOS makes it sound like the client >> > is getting DDoS'd. >> > >> > >> http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >> > File chrome/common/net/url_fetcher_unittest.cc (right): >> > >> > >> http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >> > chrome/common/net/url_fetcher_unittest.cc:315: >> > fetcher_->set_max_retries(11); >> > 11 is a magic number. Does this have any specific meaning? Perhaps you >> > can use a named constant instead. >> > >> > >> http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >> > File net/url_request/request_throttler_manager.cc (right): >> > >> > >> http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >> > net/url_request/request_throttler_manager.cc:41: std::string >> > RequestThrottlerManager::GetIdFromUrl(const GURL& url) { >> > Why are you not using the GURL canonicalization routines? What if the >> > URL has a port or query or username or password or what not? The port >> > especially seems important. It seems to me that you should use >> > url_canon::Replacements to generate a new GURL and use .spec() to >> > convert. >> > >> > >> http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >> > File net/url_request/request_throttler_manager.h (right): >> > >> > >> http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >> > net/url_request/request_throttler_manager.h:27: // thread. >> > Please use ThreadChecker to enforce this. >> > >> > >> http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >> > net/url_request/request_throttler_manager.h:36: >> > scoped_refptr<RequestThrottlerEntryInterface> RegisterRequestUrl( >> > You're using ref counted RequestThrottlerEntryInterfaces. Please >> > document somewhere the intended ownership semantics of this object. How >> > are they supposed to ever get deleted? It looks like >> > RequestThrottlerManager doesn't expose any non-test interface for >> > unregistering a RequestThrottlerEntry. How will the >> > RequestThrottlerManager ever release its reference? >> > >> > Oh I see, you use garbage collection. That's fine. It doesn't explain >> > why you're using reference counting. Let's think about this >> > conceptually, shouldn't RequestThrottlerManager be owning these entries? >> > From what I can tell, you're using reference counting because you don't >> > know who is holding onto the entry still. Is that correct? This sounds >> > suboptimal. The manager shouldn't try to garbage collect an entry that >> > is "active", where "active" means that a URLFetcher or something is >> > holding onto it. Rather than refcounting the entry, you should let >> > clients register themselves with the entry. And make them unregister >> > themselves from the entry too. The RequestThrottlerManager should own >> > the entry and should make sure to check for other references to the >> > entry, or should cancel their references via base::WeakPtr or something >> > like that. >> > >> > Please think REAL HARD whenever you use RefCounted*. It is usually the >> > wrong thing to use. >> > >> > http://codereview.chromium.org/4194001/ >> > >> > > -- Best regards, Yuzhu Shen.
On Tue, Nov 23, 2010 at 5:12 PM, Yuzhu Shen <yzshen@google.com> wrote: > Hi, William and Joi. > > Thanks for all your help and patience! > > William: I knew you were a hardass long before this CL. :) I always think > that it takes a lot of effort for the reviewer to be "picky" and I really > appreciate that. Answering difficult questions during code reviews is much > much better than fixing a ton of bugs later. So, no offense taken. And I > will ask for your favor next time for reviewing my other CLs, since you > really care about code quality. > > I know there are still things to improve. However, I wonder whether the new > memory management model issue could be saved to a later CL. > Since I am blocking the TBLF team, and also Sanjeev, who is waiting for me > to check in this CL in this week. If you do think it is a must to make the > change in this CL. Sure I will do it. But please help me to do code reviews > during the holiday. :) Ah, I didn't know this was blocking other efforts. Yes, I think it's ok to refactor the code later on. Please address my other comments and I'll make sure I iterate again tonight to see if there's anything I would consider blocking committing this change. > > > > On Tue, Nov 23, 2010 at 4:50 PM, William Chan (陈智昌) <willchan@chromium.org > > wrote: > >> Yeah, I realized it's not yzshen's fault, since he inherited the code. I >> realize my tone was very harsh when I wrote the inline comments, so I tried >> to soften it a bit when I wrote the meta-comment at the end. But I do want >> to emphasize the importance of appropriate memory management. I'm spending >> the next X quarters undoing most of the net/ refcounting to fix a gazillion >> bugs/crashes, so I take every opportunity to emphasize in code reviews that >> refcounting is usually the wrong choice. >> >> I'm also at fault here, because you guys cc'd me and eroman earlier on >> when the intern wrote the code and I didn't spend the time then to review it >> thoroughly. >> >> So sorry for coming down on Yuzhu. I know it's not his fault. But I'm >> still going to be a hardass :P Please don't take it personally, because it >> definitely isn't. Thanks for taking on this changelist Yuzhu. >> >> >> On Tue, Nov 23, 2010 at 4:28 PM, Jói Sigurðsson <joi@google.com> wrote: >> >>> Hi Will, >>> >>> Thanks for doing such a thorough review. Please don't be too hard on >>> Yuzhu, he is landing the patch at my request, most of it was written >>> this summer by an intern of mine, and the original patch had been >>> through multiple rounds of code reviews - so it's normal that he >>> should expect that issues at the level you are addressing (ref >>> counting, namespacing etc.) would have been worked out then. >>> >>> I'm the one responsible for the original change, and I should have >>> paid closer attention to it when it was originally implemented and >>> went through code review. I agree it sounds like refcounting is not >>> necessary in this instance, and I should have realized it then. >>> >>> Cheers, >>> Jói >>> >>> >>> On Tue, Nov 23, 2010 at 7:23 PM, <willchan@chromium.org> wrote: >>> > I consulted with other networking folks. You need to rename your >>> classes. >>> > Everything in net/ should be in namespace net. And RequestThrottler.* >>> is to >>> > general a name. We have a lot of different kinds of requests in the >>> network >>> > stack. If this only applies to URLRequests, then name it >>> > URLRequestThrottler.*. >>> > >>> > The memory management is done incorrectly. Refcounting is the wrong >>> tool >>> > here. >>> > I don't want to be too harsh about it, but you should think harder next >>> time >>> > about whether or not to use refcounting. It should be the last option >>> you >>> > employ, only when ownership is truly shared. The Google C++ primer >>> > (internal to >>> > Google) strongly recommends against it. Please fix the memory >>> management. >>> > >>> > >>> > >>> http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >>> > File chrome/common/net/url_fetcher.cc (right): >>> > >>> > >>> http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >>> > chrome/common/net/url_fetcher.cc:58: void >>> ReceivedContentWasMalformed(); >>> > Is this new functionality? I don't see any mention of this >>> > functionality in the changelist description. >>> > >>> > >>> http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >>> > chrome/common/net/url_fetcher.cc:402: if (response_code_ >= 500 || >>> > Can you explain why this checking happens on the delegate thread? It >>> > seems like if we haven't maxed out on retries, we should stay on the IO >>> > thread. We should only proxy to the delegate thread when we need to >>> > notify the delegate. >>> > >>> > >>> http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >>> > chrome/common/net/url_fetcher.cc:403: status.os_error() == >>> > net::ERR_TEMPORARILY_THROTTLED_BY_DDOS) { >>> > I think I'd rather this error be called ERR_TEMPORARILY_THROTTLED or >>> > ERR_TEMPORARY_BACK_OFF. Saying BY_DDOS makes it sound like the client >>> > is getting DDoS'd. >>> > >>> > >>> http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >>> > File chrome/common/net/url_fetcher_unittest.cc (right): >>> > >>> > >>> http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >>> > chrome/common/net/url_fetcher_unittest.cc:315: >>> > fetcher_->set_max_retries(11); >>> > 11 is a magic number. Does this have any specific meaning? Perhaps >>> you >>> > can use a named constant instead. >>> > >>> > >>> http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >>> > File net/url_request/request_throttler_manager.cc (right): >>> > >>> > >>> http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >>> > net/url_request/request_throttler_manager.cc:41: std::string >>> > RequestThrottlerManager::GetIdFromUrl(const GURL& url) { >>> > Why are you not using the GURL canonicalization routines? What if the >>> > URL has a port or query or username or password or what not? The port >>> > especially seems important. It seems to me that you should use >>> > url_canon::Replacements to generate a new GURL and use .spec() to >>> > convert. >>> > >>> > >>> http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >>> > File net/url_request/request_throttler_manager.h (right): >>> > >>> > >>> http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >>> > net/url_request/request_throttler_manager.h:27: // thread. >>> > Please use ThreadChecker to enforce this. >>> > >>> > >>> http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >>> > net/url_request/request_throttler_manager.h:36: >>> > scoped_refptr<RequestThrottlerEntryInterface> RegisterRequestUrl( >>> > You're using ref counted RequestThrottlerEntryInterfaces. Please >>> > document somewhere the intended ownership semantics of this object. >>> How >>> > are they supposed to ever get deleted? It looks like >>> > RequestThrottlerManager doesn't expose any non-test interface for >>> > unregistering a RequestThrottlerEntry. How will the >>> > RequestThrottlerManager ever release its reference? >>> > >>> > Oh I see, you use garbage collection. That's fine. It doesn't explain >>> > why you're using reference counting. Let's think about this >>> > conceptually, shouldn't RequestThrottlerManager be owning these >>> entries? >>> > From what I can tell, you're using reference counting because you >>> don't >>> > know who is holding onto the entry still. Is that correct? This >>> sounds >>> > suboptimal. The manager shouldn't try to garbage collect an entry that >>> > is "active", where "active" means that a URLFetcher or something is >>> > holding onto it. Rather than refcounting the entry, you should let >>> > clients register themselves with the entry. And make them unregister >>> > themselves from the entry too. The RequestThrottlerManager should own >>> > the entry and should make sure to check for other references to the >>> > entry, or should cancel their references via base::WeakPtr or something >>> > like that. >>> > >>> > Please think REAL HARD whenever you use RefCounted*. It is usually the >>> > wrong thing to use. >>> > >>> > http://codereview.chromium.org/4194001/ >>> > >>> >> >> > > > -- > Best regards, > Yuzhu Shen. > >
Thanks guys! And yes, thanks for being a hardass Will - really focusing on quality and getting it right is just awesome. If I may coin a term, it's very Chromey of you :) Cheers, Jói On Tue, Nov 23, 2010 at 8:43 PM, William Chan (陈智昌) <willchan@chromium.org> wrote: > On Tue, Nov 23, 2010 at 5:12 PM, Yuzhu Shen <yzshen@google.com> wrote: >> >> Hi, William and Joi. >> >> Thanks for all your help and patience! >> >> William: I knew you were a hardass long before this CL. :) I always think >> that it takes a lot of effort for the reviewer to be "picky" and I really >> appreciate that. Answering difficult questions during code reviews is much >> much better than fixing a ton of bugs later. So, no offense taken. And I >> will ask for your favor next time for reviewing my other CLs, since you >> really care about code quality. >> >> I know there are still things to improve. However, I wonder whether the >> new memory management model issue could be saved to a later CL. >> Since I am blocking the TBLF team, and also Sanjeev, who is waiting for me >> to check in this CL in this week. If you do think it is a must to make the >> change in this CL. Sure I will do it. But please help me to do code reviews >> during the holiday. :) > > Ah, I didn't know this was blocking other efforts. Yes, I think it's ok to > refactor the code later on. Please address my other comments and I'll make > sure I iterate again tonight to see if there's anything I would consider > blocking committing this change. > >> >> >> On Tue, Nov 23, 2010 at 4:50 PM, William Chan (陈智昌) >> <willchan@chromium.org> wrote: >>> >>> Yeah, I realized it's not yzshen's fault, since he inherited the code. I >>> realize my tone was very harsh when I wrote the inline comments, so I tried >>> to soften it a bit when I wrote the meta-comment at the end. But I do want >>> to emphasize the importance of appropriate memory management. I'm spending >>> the next X quarters undoing most of the net/ refcounting to fix a gazillion >>> bugs/crashes, so I take every opportunity to emphasize in code reviews that >>> refcounting is usually the wrong choice. >>> I'm also at fault here, because you guys cc'd me and eroman earlier on >>> when the intern wrote the code and I didn't spend the time then to review it >>> thoroughly. >>> So sorry for coming down on Yuzhu. I know it's not his fault. But I'm >>> still going to be a hardass :P Please don't take it personally, because it >>> definitely isn't. Thanks for taking on this changelist Yuzhu. >>> >>> On Tue, Nov 23, 2010 at 4:28 PM, Jói Sigurðsson <joi@google.com> wrote: >>>> >>>> Hi Will, >>>> >>>> Thanks for doing such a thorough review. Please don't be too hard on >>>> Yuzhu, he is landing the patch at my request, most of it was written >>>> this summer by an intern of mine, and the original patch had been >>>> through multiple rounds of code reviews - so it's normal that he >>>> should expect that issues at the level you are addressing (ref >>>> counting, namespacing etc.) would have been worked out then. >>>> >>>> I'm the one responsible for the original change, and I should have >>>> paid closer attention to it when it was originally implemented and >>>> went through code review. I agree it sounds like refcounting is not >>>> necessary in this instance, and I should have realized it then. >>>> >>>> Cheers, >>>> Jói >>>> >>>> >>>> On Tue, Nov 23, 2010 at 7:23 PM, <willchan@chromium.org> wrote: >>>> > I consulted with other networking folks. You need to rename your >>>> > classes. >>>> > Everything in net/ should be in namespace net. And RequestThrottler.* >>>> > is to >>>> > general a name. We have a lot of different kinds of requests in the >>>> > network >>>> > stack. If this only applies to URLRequests, then name it >>>> > URLRequestThrottler.*. >>>> > >>>> > The memory management is done incorrectly. Refcounting is the wrong >>>> > tool >>>> > here. >>>> > I don't want to be too harsh about it, but you should think harder >>>> > next time >>>> > about whether or not to use refcounting. It should be the last option >>>> > you >>>> > employ, only when ownership is truly shared. The Google C++ primer >>>> > (internal to >>>> > Google) strongly recommends against it. Please fix the memory >>>> > management. >>>> > >>>> > >>>> > >>>> > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >>>> > File chrome/common/net/url_fetcher.cc (right): >>>> > >>>> > >>>> > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >>>> > chrome/common/net/url_fetcher.cc:58: void >>>> > ReceivedContentWasMalformed(); >>>> > Is this new functionality? I don't see any mention of this >>>> > functionality in the changelist description. >>>> > >>>> > >>>> > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >>>> > chrome/common/net/url_fetcher.cc:402: if (response_code_ >= 500 || >>>> > Can you explain why this checking happens on the delegate thread? It >>>> > seems like if we haven't maxed out on retries, we should stay on the >>>> > IO >>>> > thread. We should only proxy to the delegate thread when we need to >>>> > notify the delegate. >>>> > >>>> > >>>> > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >>>> > chrome/common/net/url_fetcher.cc:403: status.os_error() == >>>> > net::ERR_TEMPORARILY_THROTTLED_BY_DDOS) { >>>> > I think I'd rather this error be called ERR_TEMPORARILY_THROTTLED or >>>> > ERR_TEMPORARY_BACK_OFF. Saying BY_DDOS makes it sound like the client >>>> > is getting DDoS'd. >>>> > >>>> > >>>> > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >>>> > File chrome/common/net/url_fetcher_unittest.cc (right): >>>> > >>>> > >>>> > http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... >>>> > chrome/common/net/url_fetcher_unittest.cc:315: >>>> > fetcher_->set_max_retries(11); >>>> > 11 is a magic number. Does this have any specific meaning? Perhaps >>>> > you >>>> > can use a named constant instead. >>>> > >>>> > >>>> > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >>>> > File net/url_request/request_throttler_manager.cc (right): >>>> > >>>> > >>>> > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >>>> > net/url_request/request_throttler_manager.cc:41: std::string >>>> > RequestThrottlerManager::GetIdFromUrl(const GURL& url) { >>>> > Why are you not using the GURL canonicalization routines? What if the >>>> > URL has a port or query or username or password or what not? The port >>>> > especially seems important. It seems to me that you should use >>>> > url_canon::Replacements to generate a new GURL and use .spec() to >>>> > convert. >>>> > >>>> > >>>> > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >>>> > File net/url_request/request_throttler_manager.h (right): >>>> > >>>> > >>>> > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >>>> > net/url_request/request_throttler_manager.h:27: // thread. >>>> > Please use ThreadChecker to enforce this. >>>> > >>>> > >>>> > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... >>>> > net/url_request/request_throttler_manager.h:36: >>>> > scoped_refptr<RequestThrottlerEntryInterface> RegisterRequestUrl( >>>> > You're using ref counted RequestThrottlerEntryInterfaces. Please >>>> > document somewhere the intended ownership semantics of this object. >>>> > How >>>> > are they supposed to ever get deleted? It looks like >>>> > RequestThrottlerManager doesn't expose any non-test interface for >>>> > unregistering a RequestThrottlerEntry. How will the >>>> > RequestThrottlerManager ever release its reference? >>>> > >>>> > Oh I see, you use garbage collection. That's fine. It doesn't >>>> > explain >>>> > why you're using reference counting. Let's think about this >>>> > conceptually, shouldn't RequestThrottlerManager be owning these >>>> > entries? >>>> > From what I can tell, you're using reference counting because you >>>> > don't >>>> > know who is holding onto the entry still. Is that correct? This >>>> > sounds >>>> > suboptimal. The manager shouldn't try to garbage collect an entry >>>> > that >>>> > is "active", where "active" means that a URLFetcher or something is >>>> > holding onto it. Rather than refcounting the entry, you should let >>>> > clients register themselves with the entry. And make them unregister >>>> > themselves from the entry too. The RequestThrottlerManager should own >>>> > the entry and should make sure to check for other references to the >>>> > entry, or should cancel their references via base::WeakPtr or >>>> > something >>>> > like that. >>>> > >>>> > Please think REAL HARD whenever you use RefCounted*. It is usually >>>> > the >>>> > wrong thing to use. >>>> > >>>> > http://codereview.chromium.org/4194001/ >>>> > >>> >> >> >> >> -- >> Best regards, >> Yuzhu Shen. >> > >
Need to take another look later tonight, but I don't think I'll have any functional objections (bugs or whatever) that would block landing this changelist. But I'll have suggestions for future changes to make this cleaner. Another suggestion for the future is to use NetLog. It'll be good to keep track somewhere that requests aren't going out because they're being throttled. Otherwise when debugging issues, it may be confusing why a request isn't starting up. http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... File net/url_request/request_throttler_entry.cc (right): http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_entry.cc:54: DCHECK(sliding_window_period_ms > 0 && Nit: I recommend breaking out each of these into its own DCHECK_GE/DCHECK_GT/etc. Each of these is already its own line. The advantage is you can leverage the DCHECK_OP macros to get better assertion messages. Also, in a big conditional like this, it's not clear which condition failed. Breaking out into smaller assertions will make it easier to debug. http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... File net/url_request/request_throttler_manager.h (right): http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_manager.h:27: // thread. On 2010/11/24 00:23:07, willchan wrote: > Please use ThreadChecker to enforce this. Btw, I would have said NonThreadSafe, except this is a Singleton, which means it'll be deleted on the main thread, instead of the IO thread. Eventually, when you move this to the URLRequestContext, it should be NonThreadSafe, so it gets created and deleted on the IO thread. http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_manager.h:57: std::string GetIdFromUrl(const GURL& url); const member function.
> Another suggestion for the future is to use NetLog. It'll > be good to keep track > somewhere that requests aren't going out because they're > being throttled. > Otherwise when debugging issues, it may be confusing why a > request isn't > starting up. Thanks! Will keep it in mind. > I consulted with other networking folks. You need to > rename your classes. > Everything in net/ should be in namespace net. And > RequestThrottler.* is to > general a name. We have a lot of different kinds of > requests in the network > stack. If this only applies to URLRequests, then name it > URLRequestThrottler.*. Done. http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... File chrome/common/net/url_fetcher.cc (right): http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:58: void ReceivedContentWasMalformed(); On 2010/11/24 00:23:07, willchan wrote: > Is this new functionality? I don't see any mention of this functionality in the > changelist description. This is not some new functionality, it is in the throttler entry's interface from the first patch. Because I removed all the locks, I had to provide a wrapper method to run RequestThrottleEntryInterface::ReceivedContentWasMalformed() on the IO thread. http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:402: if (response_code_ >= 500 || On 2010/11/24 00:23:07, willchan wrote: > Can you explain why this checking happens on the delegate thread? It seems like > if we haven't maxed out on retries, we should stay on the IO thread. We should > only proxy to the delegate thread when we need to notify the delegate. This is the original logic, I haven't changed it. But I think the reason is that we access fetcher_'s state, which is supposed to happen on the delegate's thread. http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... chrome/common/net/url_fetcher.cc:403: status.os_error() == net::ERR_TEMPORARILY_THROTTLED_BY_DDOS) { On 2010/11/24 00:23:07, willchan wrote: > I think I'd rather this error be called ERR_TEMPORARILY_THROTTLED or > ERR_TEMPORARY_BACK_OFF. Saying BY_DDOS makes it sound like the client is > getting DDoS'd. Done. http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... File chrome/common/net/url_fetcher_unittest.cc (right): http://codereview.chromium.org/4194001/diff/234001/chrome/common/net/url_fetc... chrome/common/net/url_fetcher_unittest.cc:315: fetcher_->set_max_retries(11); On 2010/11/24 00:23:07, willchan wrote: > 11 is a magic number. Does this have any specific meaning? Perhaps you can use > a named constant instead. It is only meaningful for this specific test. I had a hard time to come up with a name better than kMaxRetriesForURLFetcherProtectTest. :( I think we have a lot of magic number in this file (say, line 338). And I haven't added this magic number, I just moved it here from the other part of this file. I personally think it is fine. Please let me know if you have better idea. :) http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... File net/url_request/request_throttler_entry.cc (right): http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_entry.cc:54: DCHECK(sliding_window_period_ms > 0 && On 2010/11/24 03:10:30, willchan wrote: > Nit: I recommend breaking out each of these into its own > DCHECK_GE/DCHECK_GT/etc. Each of these is already its own line. The advantage > is you can leverage the DCHECK_OP macros to get better assertion messages. > Also, in a big conditional like this, it's not clear which condition failed. > Breaking out into smaller assertions will make it easier to debug. Done. Thanks! :) http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... File net/url_request/request_throttler_manager.cc (right): http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_manager.cc:41: std::string RequestThrottlerManager::GetIdFromUrl(const GURL& url) { On 2010/11/24 00:23:07, willchan wrote: > Why are you not using the GURL canonicalization routines? What if the URL has a > port or query or username or password or what not? The port especially seems > important. It seems to me that you should use url_canon::Replacements to > generate a new GURL and use .spec() to convert. I have changed to use Replacements to convert. Thanks! I agree with you that port is important, I have added it. For other parts, it is intended to throw them away. http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... File net/url_request/request_throttler_manager.h (right): http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_manager.h:27: // thread. On 2010/11/24 00:23:07, willchan wrote: > Please use ThreadChecker to enforce this. I tried to do so. However, the unit tests use different threads as the IO thread. They won't access the request throttler module concurrently, so writing the request throttler module in a non-thread-safe way has no problem. But they prevent me from adding the checks. I think this is another reason that we don't like RequestThrottlerManager to be a singleton. As we discussed before, I will make another CL for it. http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_manager.h:27: // thread. On 2010/11/24 03:10:30, willchan wrote: > On 2010/11/24 00:23:07, willchan wrote: > > Please use ThreadChecker to enforce this. > > Btw, I would have said NonThreadSafe, except this is a Singleton, which means > it'll be deleted on the main thread, instead of the IO thread. Eventually, when > you move this to the URLRequestContext, it should be NonThreadSafe, so it gets > created and deleted on the IO thread. I tried to use NonThreadSafe and used the LeakySingletonTraits before. However, as I mentioned above, unit tests use different IO threads. I would put it in the same CL of moving this to URLRequestContext, if you don't mind. http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_manager.h:57: std::string GetIdFromUrl(const GURL& url); On 2010/11/24 03:10:30, willchan wrote: > const member function. Done.
After reading it some more, I'm stepping back and questioning the whole RequestThrottlerEntry stuff in the first place. It doesn't seem to have any way to handle cancellations of URLFetchers/URLRequests. Also, just to double check, the throttling is done on a host+port+path basis, but not the query, right? Is this because we assume that a service is identifier by a unique host+port+path? Is this a reasonable assumption? Is host+port better? I'm curious about the choice of "key" for request throttling. One thing that seems weird to me about the design is that the URLFetchers have to request a "slot" in advance. If a URLFetcher is cancelled, the slots are not updated. If there are two URLFetchers for the exact same cacheable URL, then the second URLFetcher cannot immediately read the cached response of the first one. It needs to wait until its throttled delay to finally read the cached response. Is this intended? Let me know if I'm misreading something. I'm also fine with you saying that my points are all correct but you'd rather land the initial implementation and work towards cleaning it up later. But I'm trying to figure out if the functionality makes sense. It seems like it might be better off for the request to submit itself to the manager. If the request gets cancelled/deleted, it can unregister itself from the manager. The manager would be in charge of starting up the jobs for the requests at the appropriate throttled intervals, and if the requests are for the same URLs, it can satisfy both of them at the same time. Also, if the requests have different priority levels, then the manager can respect them in terms of when to start the requests. With this reservation of time slot approach, there's no way to let a higher priority request move in front of a low priority request that reserved its slot earlier. I've delayed this changelist long enough, but I want to get some clarity on the future direction of this code and if the questions are raised are actual issues or if they're addressed in some way I haven't seen. http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... File net/url_request/request_throttler_header_adapter.h (right): http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_header_adapter.h:10: #include "base/scoped_ptr.h" Don't you need ref_counted.h for scoped_refptr instead? http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_header_adapter.h:11: #include "net/http/http_response_headers.h" Can net::HttpResponseHeaders be forward declared instead? http://codereview.chromium.org/4194001/diff/234001/net/url_request/url_reques... File net/url_request/url_request_http_job.cc (right): http://codereview.chromium.org/4194001/diff/234001/net/url_request/url_reques... net/url_request/url_request_http_job.cc:629: int return_value; rv is sufficiently common in the network stack that unless we wanted to change the rest of the network stack, it's better to simply be consistent and continue using rv here to mean return_value. i understand why you changed it, but it's inconsistent with network stack code.
Let me throw in a couple of things and let yzshen take the rest. > After reading it some more, I'm stepping back and questioning the whole > RequestThrottlerEntry stuff in the first place. It doesn't seem to have any > way to handle cancellations of URLFetchers/URLRequests. The RequestThrottler stuff right now has two things: a) A mechanism to deny requests if they occur within the exponential back-off timeout, which is designed to be quite conservative and not block requests unless there really is a problem with the server. This part is based only on the error count and the calculated timeout based on it. From my point of view, this is the more important part; the motivation for all of this stuff was a request from Urs to make sure we have a low-level exponential back-off safety mechanism in place (whereas the previous URLFetcherProtect could be considered to be at the application level, with lots of requests bypassing it). b) A mechanism for suggesting a good time to retry a request. This is taken from URLFetcherProtect, in order to satisfy that part the interface that was previously provided by URLFetcherProtect. It's based on a sliding window. yzshen moved it to the RequestThrottler classes as part of removing conflicts of functionality between URLFetcher and RequestThrottler. I think (b) is probably what you are not so sure about (e.g. with the cancellation of URLFetchers - as that doesn't really matter at all for the low-level exponential back-off functionality). Is that correct? > Also, just to double check, > the throttling is done on a host+port+path basis, but not the query, right? > Is this because we assume that a service is identifier by a unique > host+port+path? > Is this a reasonable assumption? Is host+port better? I'm curious about the > choice of "key" for request throttling. Correct, the "key" is host+port+path. This is what has been previously used in widely deployed backoff implementations such as Toolbar and Desktop. It is generally the right key, fine-grained enough not to immediately start backing off a whole host on any problems from that hostname (consider e.g. www.google.com, with tons of services behind it), but coarse-grained enough that if the whole host really is down, then each of the limited number of keys will quite quickly converge to all back off. That being said, there are edge cases where this is not optimal. E.g. Sanjeev has said that the cloud print service may choose to encode job IDs in the URI, rather than as query parameters, in which case there would be multiple thousands of keys for their service, rather than the ~5 they have today. For that reason we plan to allow setting a value in the URLRequestContext to override the key that should be used. But for clients such as web pages and extensions that wouldn't have access to this interface, I believe the host+port+path is the best key we can choose. Cheers, Jói On Wed, Nov 24, 2010 at 5:14 AM, <willchan@chromium.org> wrote: > After reading it some more, I'm stepping back and questioning the whole > RequestThrottlerEntry stuff in the first place. It doesn't seem to have any > way > to handle cancellations of URLFetchers/URLRequests. Also, just to double > check, > the throttling is done on a host+port+path basis, but not the query, right? > Is > this because we assume that a service is identifier by a unique > host+port+path? > Is this a reasonable assumption? Is host+port better? I'm curious about > the > choice of "key" for request throttling. > > One thing that seems weird to me about the design is that the URLFetchers > have > to request a "slot" in advance. If a URLFetcher is cancelled, the slots are > not > updated. If there are two URLFetchers for the exact same cacheable URL, > then > the second URLFetcher cannot immediately read the cached response of the > first > one. It needs to wait until its throttled delay to finally read the cached > response. Is this intended? > > Let me know if I'm misreading something. I'm also fine with you saying that > my > points are all correct but you'd rather land the initial implementation and > work > towards cleaning it up later. But I'm trying to figure out if the > functionality > makes sense. It seems like it might be better off for the request to submit > itself to the manager. If the request gets cancelled/deleted, it can > unregister > itself from the manager. The manager would be in charge of starting up the > jobs > for the requests at the appropriate throttled intervals, and if the requests > are > for the same URLs, it can satisfy both of them at the same time. Also, if > the > requests have different priority levels, then the manager can respect them > in > terms of when to start the requests. With this reservation of time slot > approach, there's no way to let a higher priority request move in front of a > low > priority request that reserved its slot earlier. > > I've delayed this changelist long enough, but I want to get some clarity on > the > future direction of this code and if the questions are raised are actual > issues > or if they're addressed in some way I haven't seen. > > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > File net/url_request/request_throttler_header_adapter.h (right): > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > net/url_request/request_throttler_header_adapter.h:10: #include > "base/scoped_ptr.h" > Don't you need ref_counted.h for scoped_refptr instead? > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > net/url_request/request_throttler_header_adapter.h:11: #include > "net/http/http_response_headers.h" > Can net::HttpResponseHeaders be forward declared instead? > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/url_reques... > File net/url_request/url_request_http_job.cc (right): > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/url_reques... > net/url_request/url_request_http_job.cc:629: int return_value; > rv is sufficiently common in the network stack that unless we wanted to > change the rest of the network stack, it's better to simply be > consistent and continue using rv here to mean return_value. i > understand why you changed it, but it's inconsistent with network stack > code. > > http://codereview.chromium.org/4194001/ >
On 2010/11/24 00:50:26, willchan wrote: > Yeah, I realized it's not yzshen's fault, since he inherited the code. I > realize my tone was very harsh when I wrote the inline comments, so I tried > to soften it a bit when I wrote the meta-comment at the end. But I do want > to emphasize the importance of appropriate memory management. I'm spending > the next X quarters undoing most of the net/ refcounting to fix a gazillion > bugs/crashes, so I take every opportunity to emphasize in code reviews that > refcounting is usually the wrong choice. > The refcounting originally came about at my request while the intern was writing this code. At that point he was handing out pointers to entries in an LRU cache that was seemingly used on multiple threads. I tasked him with understanding the threading model of the net module, but I probably should have delved into it myself.
Hi, William. Thanks! On Wed, Nov 24, 2010 at 2:14 AM, <willchan@chromium.org> wrote: > After reading it some more, I'm stepping back and questioning the whole > RequestThrottlerEntry stuff in the first place. It doesn't seem to have > any way > to handle cancellations of URLFetchers/URLRequests. Also, just to double > check, > the throttling is done on a host+port+path basis, but not the query, right? > Is > this because we assume that a service is identifier by a unique > host+port+path? > Is this a reasonable assumption? Is host+port better? I'm curious about > the > choice of "key" for request throttling. > I think Joi has answered this question. > One thing that seems weird to me about the design is that the URLFetchers > have > to request a "slot" in advance. If a URLFetcher is cancelled, the slots > are not > updated. If there are two URLFetchers for the exact same cacheable URL, > then > the second URLFetcher cannot immediately read the cached response of the > first > one. It needs to wait until its throttled delay to finally read the cached > response. Is this intended? > (1) The mechanism of reserving a slot when starting a URLFetcher exists before this change (with some difference, I will get to the details below). And the original logic doesn't handle cancellation of the slot, either. I am not saying that the original logic is perfect. We probably would like to improve it during making such a big change. But I would like to put it here as some background information. (2) The reservation serves the following purpose: Consider the following scenario: the current time is time_A, and exponential back-off time for http://foobar.com is 1000 sec later (time_B). 500 URLFetchers try to start a request for http://foobar in [time_A, time_B). In this scenario, if they ask throttler entry "when should I start the request", and the entry answers "time_B", then at exact time_B, all the 500 URLFetchers will actually start a URLRequest. I don't think this is desirable. That is why we need sliding window and reservation. For the first 20 URLFetchers, the entry will answer "time_B", for the next 20 ones, it will answer "time_B + 2 seconds"... > If a URLFetcher is cancelled, the slots are not updated. That is true. > If there are two URLFetchers for the exact same cacheable URL, then > the second URLFetcher cannot immediately read the cached response of the first one The sliding window is bigger than that. According to the current parameters, we allow 20 requests to the same URL in 2 seconds. (3) As I mentioned in (1), this CL makes some modification to the back-off behavior of URLFetcher. A URLFetcher may retry, and thus send out multiple URLRequests in its lifetime. Back-off behavior before this CL: Consider the sliding window constraint for the first URLRequest, and consider the exponential back-off for all the subsequent ones. Back-off behavior in this CL: Consider both the exponential back-off and sliding window for all the URLRequests. I think the new behavior is more consistent. Let me know if I'm misreading something. I'm also fine with you saying that > my > points are all correct but you'd rather land the initial implementation and > work > towards cleaning it up later. But I'm trying to figure out if the > functionality > makes sense. It seems like it might be better off for the request to > submit > itself to the manager. If the request gets cancelled/deleted, it can > unregister > itself from the manager. The manager would be in charge of starting up the > jobs > for the requests at the appropriate throttled intervals, and if the > requests are > for the same URLs, it can satisfy both of them at the same time. The sliding window doesn't serialize all requests to the same URL. Instead, it makes sure we don't send too many requests for the same URL in a short period of time. Also, if the > requests have different priority levels, then the manager can respect them > in > terms of when to start the requests. With this reservation of time slot > approach, there's no way to let a higher priority request move in front of > a low > priority request that reserved its slot earlier. > I have not doubt that the solution you proposed will work. In that way, the manager is much more heavy-weight, and becomes a "task scheduler". But I think the original design is to make the throttler manager a light-weight "bookkeeper". I would suggest a face-to-face discussion about this, if you would spare me some time. > > I've delayed this changelist long enough, but I want to get some clarity on > the > future direction of this code and if the questions are raised are actual > issues > or if they're addressed in some way I haven't seen. > > > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > File net/url_request/request_throttler_header_adapter.h (right): > > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > net/url_request/request_throttler_header_adapter.h:10: #include > "base/scoped_ptr.h" > Don't you need ref_counted.h for scoped_refptr instead? > > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... > net/url_request/request_throttler_header_adapter.h:11: #include > "net/http/http_response_headers.h" > Can net::HttpResponseHeaders be forward declared instead? > > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/url_reques... > File net/url_request/url_request_http_job.cc (right): > > > http://codereview.chromium.org/4194001/diff/234001/net/url_request/url_reques... > net/url_request/url_request_http_job.cc:629: int return_value; > rv is sufficiently common in the network stack that unless we wanted to > change the rest of the network stack, it's better to simply be > consistent and continue using rv here to mean return_value. i > understand why you changed it, but it's inconsistent with network stack > code. > > > http://codereview.chromium.org/4194001/ > -- Best regards, Yuzhu Shen.
http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... File net/url_request/request_throttler_header_adapter.h (right): http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_header_adapter.h:10: #include "base/scoped_ptr.h" On 2010/11/24 10:14:50, willchan wrote: > Don't you need ref_counted.h for scoped_refptr instead? Done. http://codereview.chromium.org/4194001/diff/234001/net/url_request/request_th... net/url_request/request_throttler_header_adapter.h:11: #include "net/http/http_response_headers.h" On 2010/11/24 10:14:50, willchan wrote: > Can net::HttpResponseHeaders be forward declared instead? Done. http://codereview.chromium.org/4194001/diff/234001/net/url_request/url_reques... File net/url_request/url_request_http_job.cc (right): http://codereview.chromium.org/4194001/diff/234001/net/url_request/url_reques... net/url_request/url_request_http_job.cc:629: int return_value; On 2010/11/24 10:14:50, willchan wrote: > rv is sufficiently common in the network stack that unless we wanted to change > the rest of the network stack, it's better to simply be consistent and continue > using rv here to mean return_value. i understand why you changed it, but it's > inconsistent with network stack code. Done.
I've spoken to yzshen about this change. I'm fine with landing it now and iterating on it. I think we need to do some major changes to how the code is structured. I'm not completely sure I've flushed out all the bugs, so we might run into issues for M9. It'll be great if we can get some NetLog coverage here so we can debug any issues with about:net-internals. LGTM, but with the understanding we need to make some potentially big changes. I will meet with yzshen next week in MV to discuss steps forward.
Hi, willchan. Thanks! On Wed, Nov 24, 2010 at 1:10 PM, <willchan@chromium.org> wrote: > I've spoken to yzshen about this change. I'm fine with landing it now and > iterating on it. I think we need to do some major changes to how the code > is > structured. I'm not completely sure I've flushed out all the bugs, so we > might > run into issues for M9. It'll be great if we can get some NetLog coverage > here > so we can debug any issues with about:net-internals. > I will work on it right away. > > LGTM, but with the understanding we need to make some potentially big > changes. > I will meet with yzshen next week in MV to discuss steps forward. > > > http://codereview.chromium.org/4194001/ > BTW: eroman is on vacation now. I have talked with him and he thinks I could proceeds to land this CL with willchan's LGTM. -- Best regards, Yuzhu Shen. |
