Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2898983006:
Fix and refactor HttpServerPropertiesImpl's alternative services brokenness expiration behavior (Closed)
DescriptionFix and refactor HttpServerPropertiesImpl's alternative services brokenness expiration behavior.
One of HttpServerPropertiesImpl's responsibilities is to store HTTP alternative services that have failed and keeps track of their time-until-retry (i.e. when their brokenness expires). The broken alt-svcs and their respective expiration times are kept in a list ordered by expiration time. When the expiration task runs, it drops items from the front of the list until the expiration time of the head alt-svc is no longer in the past. Turns out this list was kept in insertion order as opposed to expiration-sorted order. This change keeps this queue in expiration-sorted order, as expected.
The additional logic added to implement the above change led to a refactor of HttpServerPropertiesImpl: its broken alt-svc logic is put in its own class, BrokenAlternativeServices.
BUG=724302
Review-Url: https://codereview.chromium.org/2898983006
Cr-Commit-Position: refs/heads/master@{#476475}
Committed: https://chromium.googlesource.com/chromium/src/+/64ccc57c1a20a8b2804df8a4e9be42b33b5b8f61
Patch Set 1 #
Total comments: 26
Patch Set 2 : Renamed HttpBrokenAlternativeServicesManager to BrokenAlternativeServices #Patch Set 3 : Fixed Ryan's comments #Patch Set 4 : Fixed some mistakes in unit-test code #
Total comments: 46
Patch Set 5 : Fixed rch's comments from ps4 #Patch Set 6 : Added checks for contents of expired_alt_svcs_ in BrokenAlternativeServicesTest #
Total comments: 34
Patch Set 7 : Fixed rch's comments from ps6 #
Total comments: 4
Patch Set 8 : Fixed rch's comments from ps7 #Patch Set 9 : Inlined all uses of BROKEN_ALT_SVC_EXPIRATION_DELAY. Unrolled loops to reduce logic in BrokenAltern… #
Total comments: 16
Patch Set 10 : Rebase #Patch Set 11 : Fixed cherie's comments from ps9; added BrokenAlternativeServicesTest.ScheduleExpireTaskAfterExpire #
Dependent Patchsets: Messages
Total messages: 48 (29 generated)
Description was changed from ========== git cl format Added more unit-tests for HttpBrokenAlternativeServicesManager Added http_broken_alternative_services_manager, currently adding unittests. Removed commented-out old code Moved these HttpServerPropertiesImpl members: broken_alternative_services_list_, ...map_, recently_broken_alternative_services_, and clock_ ...into their own class HttpBrokenAlternativeServicesManager. Switched to using base::OneShotTimer for scheduling/cancelling expiration tasks. Fixed HttpServerPropertiesImpl to insert broken alt-svcs into sorted position in the expiration list. Updated how expiration tasks are scheduled: they're scheduled for the first alt-svc in the list that doesn't already have one scheduled. Added unit-test that reproduces the unsorted alt svc expiration queue bug; added some plumbing necessary for this test in HttpServerPropertiesImpl Cleaned up HttpServerPropertiesImpl no-arg constructor with delegate constructor Added TestClock memeber and changed impl_ to use &test_clock_ in HttpServerPropertiesImplTest Added TestClock : HttpServerPropertiesImpl::Clock class to HttpServerPropertiesImpl unit-test. Added HttpServerPropertiesImpl::Clock interface test dependency injection. Added HttpServerPropertiesImpl::DefaultClock that just calls TimeTicks::Now() BUG= ========== to ========== Fix and refactor HttpServerPropertiesImpl's alternative services brokenness expiration behavior. HttpServerPropertiesImpl is used to store HTTP alternative services that have failed and keeps track of their time-until-retry (i.e. when their brokenness expires). The broken alt-svcs and their respective expiration times are kept in a list ordered by expiration times. When the expiration task runs, it drops items from the front of the list until the expiration time of the head alt-svc is no longer in the past. Turns out this list was kept in insertion order as opposed to sorted order by expiration. This change keeps this queue in sorted order, as expected. The additional logic added to implement the above change lead to a refactor of HttpServerPropertiesImpl: its broken alt-svc logic is put in its own class, HttpBrokenAlternativeServicesManager. BUG=724302 ==========
wangyix@chromium.org changed reviewers: + rch@chromium.org, zhongyi@chromium.org
PTAL
Looking good! A bunch of nits and I need to spend a bit more time verifying the test, but this is in good shape. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... File net/http/http_broken_alternative_services_manager.h (right): https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:23: class NET_EXPORT HttpBrokenAlternativeServicesManager { nit: This class name is a bit of a mouthful, how 'bout just "BrokenAlternativeServices". Please add a simple class comment which describes what it does (you'll probably want to mention expiration and backoff, for example). I think this can be NET_EXPORT_PRIVATE instead of NET_EXPORT since I don't think we'll have consumers outside of net/ https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:32: class NET_EXPORT DefaultClock : public Clock { nit: You could consider adding static method to clock: static Clock* GetDefaultClockInstance(); And then hide DefaultClock in the .cc file. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:48: const AlternativeService& expired_alternative_service) = 0; Should we also have virtual ~Delegate() {}? https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:51: HttpBrokenAlternativeServicesManager(Delegate* delegate, Clock* clock); nit: Comment, please, and note the ownership/lifetime requirements of both arguments. (Namely, that they both must outlive this. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:55: void MarkAlternativeServiceBroken( nit: comments for all these methods, please. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:66: int broken_count); Can we just put this in a test.cc file instead? https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:69: friend class HttpServerPropertiesImplPeer; We should see if we can get rid of this friend-ness in a follow up. Perhaps add a TODO. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:71: struct BrokenAltSvcExpireInfo { nit: comment. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:75: AlternativeService alternative_service; nit: newline before https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:98: Delegate* delegate_; nit: // Unowned. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:108: base::OneShotTimer timer_; nit: how 'bout expiration_timer_? https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:112: DISALLOW_COPY_AND_ASSIGN(HttpBrokenAlternativeServicesManager); nit: I think the new hotness is using = delete for the copy constructor and assignment operator. https://codereview.chromium.org/2898983006/diff/1/net/http/http_server_proper... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2898983006/diff/1/net/http/http_server_proper... net/http/http_server_properties_impl.cc:636: } nit: would you mind moving this back to where the old version of this method was to minimize the diffs?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
PTAL https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... File net/http/http_broken_alternative_services_manager.h (right): https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:23: class NET_EXPORT HttpBrokenAlternativeServicesManager { On 2017/05/25 03:03:50, Ryan Hamilton wrote: > nit: This class name is a bit of a mouthful, how 'bout just > "BrokenAlternativeServices". > > Please add a simple class comment which describes what it does (you'll probably > want to mention expiration and backoff, for example). > > I think this can be NET_EXPORT_PRIVATE instead of NET_EXPORT since I don't think > we'll have consumers outside of net/ Done. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:32: class NET_EXPORT DefaultClock : public Clock { On 2017/05/25 03:03:50, Ryan Hamilton wrote: > nit: You could consider adding static method to clock: > > static Clock* GetDefaultClockInstance(); > > And then hide DefaultClock in the .cc file. Done. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:48: const AlternativeService& expired_alternative_service) = 0; On 2017/05/25 03:03:50, Ryan Hamilton wrote: > Should we also have virtual ~Delegate() {}? Done. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:51: HttpBrokenAlternativeServicesManager(Delegate* delegate, Clock* clock); On 2017/05/25 03:03:50, Ryan Hamilton wrote: > nit: Comment, please, and note the ownership/lifetime requirements of both > arguments. (Namely, that they both must outlive this. Done. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:55: void MarkAlternativeServiceBroken( On 2017/05/25 03:03:50, Ryan Hamilton wrote: > nit: comments for all these methods, please. Done. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:66: int broken_count); On 2017/05/25 03:03:50, Ryan Hamilton wrote: > Can we just put this in a test.cc file instead? Done. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:69: friend class HttpServerPropertiesImplPeer; On 2017/05/25 03:03:50, Ryan Hamilton wrote: > We should see if we can get rid of this friend-ness in a follow up. Perhaps add > a TODO. Done. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:71: struct BrokenAltSvcExpireInfo { On 2017/05/25 03:03:50, Ryan Hamilton wrote: > nit: comment. Done. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:75: AlternativeService alternative_service; On 2017/05/25 03:03:50, Ryan Hamilton wrote: > nit: newline before Done. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:98: Delegate* delegate_; On 2017/05/25 03:03:50, Ryan Hamilton wrote: > nit: // Unowned. Done. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:108: base::OneShotTimer timer_; On 2017/05/25 03:03:50, Ryan Hamilton wrote: > nit: how 'bout expiration_timer_? Done. https://codereview.chromium.org/2898983006/diff/1/net/http/http_broken_altern... net/http/http_broken_alternative_services_manager.h:112: DISALLOW_COPY_AND_ASSIGN(HttpBrokenAlternativeServicesManager); On 2017/05/25 03:03:50, Ryan Hamilton wrote: > nit: I think the new hotness is using = delete for the copy constructor and > assignment operator. Done. https://codereview.chromium.org/2898983006/diff/1/net/http/http_server_proper... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2898983006/diff/1/net/http/http_server_proper... net/http/http_server_properties_impl.cc:636: } On 2017/05/25 03:03:50, Ryan Hamilton wrote: > nit: would you mind moving this back to where the old version of this method was > to minimize the diffs? Done.
Looking good, though I discovered that we can probably just use base::TickClock to make life simpler. Sorry I didn't notice that class before. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... File net/http/broken_alternative_services.cc (right): https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:38: ~DefaultClock() override {} Do you need these since they're default? https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:44: void operator=(const DefaultClock&) = delete; these don't need to be made private since they're deleted, but since this class has no members, I don't think it's worth deleting these constructors. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:51: return DefaultClock::GetInstance(); I would just inline the base::Singleton<> call here since it's still hidden in the .cc file https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:73: } I think I'd just DCHECK this. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:117: return false; I suspect we don't need this since it's already forbidden to put unknown protocols in the list, right? https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:142: bool BrokenAlternativeServices ::AddToBrokenAlternativeServiceListAndMap( nit: no space before "::" https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:154: auto list_it = broken_alternative_service_list_.end(); Is it safe to do -- on an end() iterator? I think you might want to use rbegin and rend to loop over the list in reverse order. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:161: } Could std::lower_bound be used here? https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:175: ; nit: remove this? https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:187: delegate_->OnExpireBrokenAlternativeService(expired_alternative_service); not that it super matters, but since On...() takes a const&, if you call this method with it->alternative_service (before you erase the iterator, obviously :>) then you can avoid a copy. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... File net/http/broken_alternative_services.h (right): https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:24: // alternative services that have been marked as broken. The brokenness of an nit: this class doesn't know about HttpServerProperties, so it's best not to reference it from the comment. // This class tracks HTTP alternative services that have been marked as broken. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:34: static Clock* GetDefaultClockInstance(); Can you add a comment (which mentions the ownership) https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:36: virtual base::TimeTicks Now() const = 0; This is a pretty obvious method, but please comment anyway. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:44: const AlternativeService& expired_alternative_service) = 0; Please comment to mention when this method is called. (this is also obvious, but still :>) https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:49: // service expires; must not be nullptr. // |delegate| will be notified when a broken alternative service expires. It must not be null. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:51: // expiration of broken alternative services. Can clock be null? https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:133: Clock* clock_; // Unowned nit: move this up next to delegate_ since they're both passed in. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:142: void operator=(const BrokenAlternativeServices&) = delete; these are more commonly found up near the constructors to make it obvious that they're deleted. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... File net/http/broken_alternative_services_unittest.cc (right): https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:54: ::testing::AssertionResult MarkAlternativeServiceBrokenAndLetExpireNTimes( This method is quite complicated. It seems like it's only called from 2 places. I think we can just inline in with some simplifications when it's used. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:62: test_task_runner_); Is this required? From reading the class, I don't understand if it's required. Actually, from reading this more, I realize that TestMockTimeTaskRunner has a GetMockTickClock method for vending a clock that's in sync with the runner. Which pointed out that there is now a base::TicksClock class. yay! This was news to me. Perhaps we can use this instead of rolling our own? https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:97: BrokenAlternativeServices services_; nit: how 'bout "broken_services_"
Fixed rch's comments from ps4, PTAL https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... File net/http/broken_alternative_services.cc (right): https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:38: ~DefaultClock() override {} On 2017/05/26 03:52:08, Ryan Hamilton wrote: > Do you need these since they're default? Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:44: void operator=(const DefaultClock&) = delete; On 2017/05/26 03:52:08, Ryan Hamilton wrote: > these don't need to be made private since they're deleted, but since this class > has no members, I don't think it's worth deleting these constructors. Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:51: return DefaultClock::GetInstance(); On 2017/05/26 03:52:08, Ryan Hamilton wrote: > I would just inline the base::Singleton<> call here since it's still hidden in > the .cc file Tried this and got compiler error: 'get' is a private member of 'base::Singleton<DefaultClock> https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:73: } On 2017/05/26 03:52:08, Ryan Hamilton wrote: > I think I'd just DCHECK this. Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:117: return false; On 2017/05/26 03:52:08, Ryan Hamilton wrote: > I suspect we don't need this since it's already forbidden to put unknown > protocols in the list, right? Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:142: bool BrokenAlternativeServices ::AddToBrokenAlternativeServiceListAndMap( On 2017/05/26 03:52:08, Ryan Hamilton wrote: > nit: no space before "::" Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:154: auto list_it = broken_alternative_service_list_.end(); On 2017/05/26 03:52:08, Ryan Hamilton wrote: > Is it safe to do -- on an end() iterator? I think you might want to use rbegin > and rend to loop over the list in reverse order. The reason I used an iterator instead of reverse_iterator is because list::insert does not work with a reverse_iterator. About decrementing end(), I found this, it looks like it's okay? https://stackoverflow.com/questions/5322104/how-portable-is-end-iterator-decr... https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:161: } On 2017/05/26 03:52:08, Ryan Hamilton wrote: > Could std::lower_bound be used here? Looks like std::lower_bound was designed for containers with random access iterators (it internally does binary search). This would be very inefficient for std::list https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:175: ; On 2017/05/26 03:52:07, Ryan Hamilton wrote: > nit: remove this? Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:187: delegate_->OnExpireBrokenAlternativeService(expired_alternative_service); On 2017/05/26 03:52:08, Ryan Hamilton wrote: > not that it super matters, but since On...() takes a const&, if you call this > method with it->alternative_service (before you erase the iterator, obviously > :>) then you can avoid a copy. Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... File net/http/broken_alternative_services.h (right): https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:24: // alternative services that have been marked as broken. The brokenness of an On 2017/05/26 03:52:08, Ryan Hamilton wrote: > nit: this class doesn't know about HttpServerProperties, so it's best not to > reference it from the comment. > > // This class tracks HTTP alternative services that have been marked as broken. Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:34: static Clock* GetDefaultClockInstance(); On 2017/05/26 03:52:09, Ryan Hamilton wrote: > Can you add a comment (which mentions the ownership) Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:36: virtual base::TimeTicks Now() const = 0; On 2017/05/26 03:52:08, Ryan Hamilton wrote: > This is a pretty obvious method, but please comment anyway. Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:44: const AlternativeService& expired_alternative_service) = 0; On 2017/05/26 03:52:08, Ryan Hamilton wrote: > Please comment to mention when this method is called. > > (this is also obvious, but still :>) Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:49: // service expires; must not be nullptr. On 2017/05/26 03:52:09, Ryan Hamilton wrote: > // |delegate| will be notified when a broken alternative service expires. It > must not be null. Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:51: // expiration of broken alternative services. On 2017/05/26 03:52:08, Ryan Hamilton wrote: > Can clock be null? Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:133: Clock* clock_; // Unowned On 2017/05/26 03:52:08, Ryan Hamilton wrote: > nit: move this up next to delegate_ since they're both passed in. Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.h:142: void operator=(const BrokenAlternativeServices&) = delete; On 2017/05/26 03:52:09, Ryan Hamilton wrote: > these are more commonly found up near the constructors to make it obvious that > they're deleted. Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... File net/http/broken_alternative_services_unittest.cc (right): https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:54: ::testing::AssertionResult MarkAlternativeServiceBrokenAndLetExpireNTimes( On 2017/05/26 03:52:09, Ryan Hamilton wrote: > This method is quite complicated. It seems like it's only called from 2 places. > I think we can just inline in with some simplifications when it's used. Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:62: test_task_runner_); On 2017/05/26 03:52:09, Ryan Hamilton wrote: > Is this required? From reading the class, I don't understand if it's required. > > Actually, from reading this more, I realize that TestMockTimeTaskRunner has a > GetMockTickClock method for vending a clock that's in sync with the runner. > Which pointed out that there is now a base::TicksClock class. yay! This was news > to me. Perhaps we can use this instead of rolling our own? Done. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:97: BrokenAlternativeServices services_; On 2017/05/26 03:52:09, Ryan Hamilton wrote: > nit: how 'bout "broken_services_" Done.
Lookin' good. The main code looks done modulo a few nits. A couple of comments on the new tests/ https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... File net/http/broken_alternative_services.cc (right): https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:51: return DefaultClock::GetInstance(); On 2017/05/27 01:20:08, wangyix1 wrote: > On 2017/05/26 03:52:08, Ryan Hamilton wrote: > > I would just inline the base::Singleton<> call here since it's still hidden in > > the .cc file > > Tried this and got compiler error: > 'get' is a private member of 'base::Singleton<DefaultClock> *mind blown* https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:154: auto list_it = broken_alternative_service_list_.end(); On 2017/05/27 01:20:09, wangyix1 wrote: > On 2017/05/26 03:52:08, Ryan Hamilton wrote: > > Is it safe to do -- on an end() iterator? I think you might want to use rbegin > > and rend to loop over the list in reverse order. > > The reason I used an iterator instead of reverse_iterator is because > list::insert does not work with a reverse_iterator. Ah, I see. Fwiw, reverse_iterator has a base() method. > About decrementing end(), I found this, it looks like it's okay? > https://stackoverflow.com/questions/5322104/how-portable-is-end-iterator-decr... Cool. Looks good. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:161: } On 2017/05/27 01:20:10, wangyix1 wrote: > On 2017/05/26 03:52:08, Ryan Hamilton wrote: > > Could std::lower_bound be used here? > > Looks like std::lower_bound was designed for containers with random access > iterators (it internally does binary search). This would be very inefficient for > std::list Fair enough. Interestingly it appears to be merely linear: The number of comparisons performed is logarithmic in the distance between first and last (At most log 2(last - first) + O(1) comparisons). However, for non-RandomAccessIterators, the number of iterator increments is linear. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... File net/http/broken_alternative_services.cc (right): https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services.cc:102: return; nit: since we can't mark unknown as broken it doesn't make sense to confirm it. So how 'bout DCHECK(!unknown)? https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... File net/http/broken_alternative_services.h (right): https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services.h:58: // Marks |alternative_service| as broken until after some expiration delay nit: newlines before each of these method comments. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services.h:79: // Returns true MarkAlternativeServiceRecentlyBroken(alternative_service) was nit: I think "if" is missing from this comment? https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services.h:110: // Inserts |alternative_service| and its |expiration| time into nit: newlines before these comments. a bit more vertical whitespace would be nice. (Here and below) https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... File net/http/broken_alternative_services_unittest.cc (right): https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:19: base::TimeDelta::FromSeconds(300), base::TimeDelta::FromSeconds(600), Since these are all more than 60 seconds, how about using FromMinutes? https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:116: alternative_service)); nit: no need to test this since you verified the initial state in earlier tests. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:120: test_task_runner_); Can you remove this ScopedContext? (and the nesting) https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:123: EXPECT_TRUE(broken_services_.IsAlternativeServiceBroken(alternative_service)); ditto https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:131: test_task_runner_->FastForwardBy(BROKEN_ALT_SVC_EXPIRE_DELAYS[0] - can you inline FromMinutes(5) instead of using this global variable to make the test more obvious. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:134: // Ensure |alternative_service| is still marked broken. nit: might want to check that there is still a posted task. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:143: EXPECT_FALSE( nit: might want to check that there are no posted tasks. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:144: broken_services_.IsAlternativeServiceBroken(alternative_service)); It should now be RecentlyBroken, right? Can you verify this? https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:147: } Nice test! Very clear. Let's add another test which explicitly tests the back off, and the backoff limit. MarkBroken, expire, MarkBroken, <verify backoff > expire MarkBroken, <verify backoff > expire ... MarkBroken, <verify no more backoff > expire https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:166: test_task_runner_); ditto about the scoped context https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:170: broken_services_.IsAlternativeServiceBroken(alternative_service1)); nit: no need to re-verify this since you've done so in previous tests. (here and in the rest of this loop.) I think you'll end up with: broken_services_.MarkAlternativeServiceBroken(alternative_service1); test_task_runner_->FastForwardBy(base::TimeDelta::FromMinutes(5)); broken_services_.MarkAlternativeServiceBroken(alternative_service1); test_task_runner_->FastForwardBy(base::TimeDelta::FromMinutes(10)); broken_services_.MarkAlternativeServiceBroken(alternative_service1); test_task_runner_->FastForwardBy(base::TimeDelta::FromMinutes(20)); https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:193: test_task_runner_); can you remove this?
Patchset #7 (id:240001) has been deleted
Fixed comments from ps6, PTAL. https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... File net/http/broken_alternative_services.cc (right): https://codereview.chromium.org/2898983006/diff/180001/net/http/broken_altern... net/http/broken_alternative_services.cc:161: } On 2017/05/27 13:28:15, Ryan Hamilton wrote: > On 2017/05/27 01:20:10, wangyix1 wrote: > > On 2017/05/26 03:52:08, Ryan Hamilton wrote: > > > Could std::lower_bound be used here? > > > > Looks like std::lower_bound was designed for containers with random access > > iterators (it internally does binary search). This would be very inefficient > for > > std::list > > Fair enough. Interestingly it appears to be merely linear: > > The number of comparisons performed is logarithmic in the distance between first > and last (At most log > 2(last - first) + O(1) comparisons). However, for non-RandomAccessIterators, the > number of iterator increments is linear. Oh interesting. If it's linear, it should be fine to use std::lower_bound then. However, I assume it will do a linear search from the beginning, whereas my code does it from the end, which I think is preferable. What do you think? https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... File net/http/broken_alternative_services.cc (right): https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services.cc:102: return; On 2017/05/27 13:28:15, Ryan Hamilton wrote: > nit: since we can't mark unknown as broken it doesn't make sense to confirm it. > So how 'bout DCHECK(!unknown)? Done. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... File net/http/broken_alternative_services.h (right): https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services.h:58: // Marks |alternative_service| as broken until after some expiration delay On 2017/05/27 13:28:16, Ryan Hamilton wrote: > nit: newlines before each of these method comments. Discussed; will ignore comment. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services.h:79: // Returns true MarkAlternativeServiceRecentlyBroken(alternative_service) was On 2017/05/27 13:28:16, Ryan Hamilton wrote: > nit: I think "if" is missing from this comment? Done. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services.h:110: // Inserts |alternative_service| and its |expiration| time into On 2017/05/27 13:28:16, Ryan Hamilton wrote: > nit: newlines before these comments. a bit more vertical whitespace would be > nice. (Here and below) Discussed; will ignore comment. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... File net/http/broken_alternative_services_unittest.cc (right): https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:19: base::TimeDelta::FromSeconds(300), base::TimeDelta::FromSeconds(600), On 2017/05/27 13:28:16, Ryan Hamilton wrote: > Since these are all more than 60 seconds, how about using FromMinutes? Done. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:116: alternative_service)); On 2017/05/27 13:28:16, Ryan Hamilton wrote: > nit: no need to test this since you verified the initial state in earlier tests. Done. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:120: test_task_runner_); On 2017/05/27 13:28:16, Ryan Hamilton wrote: > Can you remove this ScopedContext? (and the nesting) Done. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:123: EXPECT_TRUE(broken_services_.IsAlternativeServiceBroken(alternative_service)); On 2017/05/27 13:28:16, Ryan Hamilton wrote: > ditto Done. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:131: test_task_runner_->FastForwardBy(BROKEN_ALT_SVC_EXPIRE_DELAYS[0] - On 2017/05/27 13:28:16, Ryan Hamilton wrote: > can you inline FromMinutes(5) instead of using this global variable to make the > test more obvious. Done. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:134: // Ensure |alternative_service| is still marked broken. On 2017/05/27 13:28:16, Ryan Hamilton wrote: > nit: might want to check that there is still a posted task. Done. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:143: EXPECT_FALSE( On 2017/05/27 13:28:16, Ryan Hamilton wrote: > nit: might want to check that there are no posted tasks. Done. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:144: broken_services_.IsAlternativeServiceBroken(alternative_service)); On 2017/05/27 13:28:16, Ryan Hamilton wrote: > It should now be RecentlyBroken, right? Can you verify this? Done. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:147: } On 2017/05/27 13:28:16, Ryan Hamilton wrote: > Nice test! Very clear. > > Let's add another test which explicitly tests the back off, and the backoff > limit. > > MarkBroken, expire, > MarkBroken, <verify backoff > expire > MarkBroken, <verify backoff > expire > ... > MarkBroken, <verify no more backoff > expire Done. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:166: test_task_runner_); On 2017/05/27 13:28:16, Ryan Hamilton wrote: > ditto about the scoped context Done. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:170: broken_services_.IsAlternativeServiceBroken(alternative_service1)); On 2017/05/27 13:28:16, Ryan Hamilton wrote: > nit: no need to re-verify this since you've done so in previous tests. (here and > in the rest of this loop.) I think you'll end up with: > > broken_services_.MarkAlternativeServiceBroken(alternative_service1); > test_task_runner_->FastForwardBy(base::TimeDelta::FromMinutes(5)); > broken_services_.MarkAlternativeServiceBroken(alternative_service1); > test_task_runner_->FastForwardBy(base::TimeDelta::FromMinutes(10)); > broken_services_.MarkAlternativeServiceBroken(alternative_service1); > test_task_runner_->FastForwardBy(base::TimeDelta::FromMinutes(20)); Done. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:193: test_task_runner_); On 2017/05/27 13:28:16, Ryan Hamilton wrote: > can you remove this? Done.
Sweet. Almost there. One comment fix and a couple of test simplifications. https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... File net/http/broken_alternative_services_unittest.cc (right): https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:147: } On 2017/05/30 21:18:18, wangyix1 wrote: > On 2017/05/27 13:28:16, Ryan Hamilton wrote: > > Nice test! Very clear. > > > > Let's add another test which explicitly tests the back off, and the backoff > > limit. > > > > MarkBroken, expire, > > MarkBroken, <verify backoff > expire > > MarkBroken, <verify backoff > expire > > ... > > MarkBroken, <verify no more backoff > expire > > Done. Thanks for adding the test. Can we unroll the loop and inline the time periods involved to make the logic simpler? (See tott #338 https://docs.google.com/document/d/1VbCdizaXK8xDTKD8Z-IP24BeW8-UYklnuG3leU2M4...) (Can you also inline other uses of BROKEN_ALT_SVC_EXPIRE_DELAYS?) https://codereview.chromium.org/2898983006/diff/260001/net/http/broken_altern... File net/http/broken_alternative_services.h (right): https://codereview.chromium.org/2898983006/diff/260001/net/http/broken_altern... net/http/broken_alternative_services.h:81: // called afterwards. It is also true if a service was marked broken and has expired? https://codereview.chromium.org/2898983006/diff/260001/net/http/broken_altern... File net/http/broken_alternative_services_unittest.cc (right): https://codereview.chromium.org/2898983006/diff/260001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:43: // |test_task_runner_|. nice comment!
Inlined BROKEN_ALT_SVC_EXPIRE_DELAYS and unrolled loops to reduce logic in unit test. PTAL https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... File net/http/broken_alternative_services_unittest.cc (right): https://codereview.chromium.org/2898983006/diff/220001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:147: } On 2017/05/30 21:32:28, Ryan Hamilton wrote: > On 2017/05/30 21:18:18, wangyix1 wrote: > > On 2017/05/27 13:28:16, Ryan Hamilton wrote: > > > Nice test! Very clear. > > > > > > Let's add another test which explicitly tests the back off, and the backoff > > > limit. > > > > > > MarkBroken, expire, > > > MarkBroken, <verify backoff > expire > > > MarkBroken, <verify backoff > expire > > > ... > > > MarkBroken, <verify no more backoff > expire > > > > Done. > > Thanks for adding the test. Can we unroll the loop and inline the time periods > involved to make the logic simpler? > > (See tott #338 > https://docs.google.com/document/d/1VbCdizaXK8xDTKD8Z-IP24BeW8-UYklnuG3leU2M4...) > > (Can you also inline other uses of BROKEN_ALT_SVC_EXPIRE_DELAYS?) Done. https://codereview.chromium.org/2898983006/diff/260001/net/http/broken_altern... File net/http/broken_alternative_services.h (right): https://codereview.chromium.org/2898983006/diff/260001/net/http/broken_altern... net/http/broken_alternative_services.h:81: // called afterwards. On 2017/05/30 21:32:28, Ryan Hamilton wrote: > It is also true if a service was marked broken and has expired? Done. https://codereview.chromium.org/2898983006/diff/260001/net/http/broken_altern... File net/http/broken_alternative_services_unittest.cc (right): https://codereview.chromium.org/2898983006/diff/260001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:43: // |test_task_runner_|. On 2017/05/30 21:32:28, Ryan Hamilton wrote: > nice comment! Acknowledged.
lgtm
The CQ bit was checked by wangyix@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by wangyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org Link to the patchset: https://codereview.chromium.org/2898983006/#ps320001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Looks great! Just a couple of nits. https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... File net/http/broken_alternative_services.cc (right): https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services.cc:24: DCHECK(broken_count >= 0); nit: DCHECK_GE(borken_count, 0); https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... File net/http/broken_alternative_services.h (right): https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services.h:35: // Delegate to be used by owner so it can notified when the brokenness of an nit: be notified https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services.h:39: // Will be called when a broken alternative service's expiration time is nit: How about just "Called when a broken..."? https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services.h:47: // must not null. nit: not be null https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services.h:101: base::TimeTicks expiration; I guess this is the brokenness expiration time, right? Could you call this out explicitly in the comments on line #94. AlternativeServiceInfo also has a expiration field, which is the validity of an alternative service. https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... File net/http/broken_alternative_services_unittest.cc (right): https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:126: // Advance time by one time quantum nit: quantum. https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:332: Most of the tests currently schedule expiring task caused by MarkAlternativeServiceBroken. How about having one more simple test covering the logic that we post another expiring task when the previous fires but leaving alt svc pending expiring in the future? https://codereview.chromium.org/2898983006/diff/300001/net/http/http_server_p... File net/http/http_server_properties_impl.h (right): https://codereview.chromium.org/2898983006/diff/300001/net/http/http_server_p... net/http/http_server_properties_impl.h:124: // BrokenAlternativeServices::Delegate method nit: method.
The CQ bit was unchecked by wangyix@chromium.org
The CQ bit was checked by wangyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #11 (id:340001) has been deleted
Fixed zhongyi's comments from ps9. PTAL https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... File net/http/broken_alternative_services.cc (right): https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services.cc:24: DCHECK(broken_count >= 0); On 2017/05/31 18:54:14, Zhongyi Shi wrote: > nit: DCHECK_GE(borken_count, 0); Done. https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... File net/http/broken_alternative_services.h (right): https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services.h:35: // Delegate to be used by owner so it can notified when the brokenness of an On 2017/05/31 18:54:14, Zhongyi Shi wrote: > nit: be notified Done. https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services.h:39: // Will be called when a broken alternative service's expiration time is On 2017/05/31 18:54:14, Zhongyi Shi wrote: > nit: How about just "Called when a broken..."? Done. https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services.h:47: // must not null. On 2017/05/31 18:54:14, Zhongyi Shi wrote: > nit: not be null Done. https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services.h:101: base::TimeTicks expiration; On 2017/05/31 18:54:14, Zhongyi Shi wrote: > I guess this is the brokenness expiration time, right? Could you call this out > explicitly in the comments on line #94. AlternativeServiceInfo also has a > expiration field, which is the validity of an alternative service. Done. https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... File net/http/broken_alternative_services_unittest.cc (right): https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:126: // Advance time by one time quantum On 2017/05/31 18:54:15, Zhongyi Shi wrote: > nit: quantum. Done. https://codereview.chromium.org/2898983006/diff/300001/net/http/broken_altern... net/http/broken_alternative_services_unittest.cc:332: On 2017/05/31 18:54:14, Zhongyi Shi wrote: > Most of the tests currently schedule expiring task caused by > MarkAlternativeServiceBroken. How about having one more simple test covering the > logic that we post another expiring task when the previous fires but leaving alt > svc pending expiring in the future? Done. https://codereview.chromium.org/2898983006/diff/300001/net/http/http_server_p... File net/http/http_server_properties_impl.h (right): https://codereview.chromium.org/2898983006/diff/300001/net/http/http_server_p... net/http/http_server_properties_impl.h:124: // BrokenAlternativeServices::Delegate method On 2017/05/31 18:54:15, Zhongyi Shi wrote: > nit: method. Done.
The CQ bit was checked by wangyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM! Thanks for fixing this!
Could you update your cl description as well? At least BrokenAlternativeServiceManager is not a thing in the final CL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix and refactor HttpServerPropertiesImpl's alternative services brokenness expiration behavior. HttpServerPropertiesImpl is used to store HTTP alternative services that have failed and keeps track of their time-until-retry (i.e. when their brokenness expires). The broken alt-svcs and their respective expiration times are kept in a list ordered by expiration times. When the expiration task runs, it drops items from the front of the list until the expiration time of the head alt-svc is no longer in the past. Turns out this list was kept in insertion order as opposed to sorted order by expiration. This change keeps this queue in sorted order, as expected. The additional logic added to implement the above change lead to a refactor of HttpServerPropertiesImpl: its broken alt-svc logic is put in its own class, HttpBrokenAlternativeServicesManager. BUG=724302 ========== to ========== Fix and refactor HttpServerPropertiesImpl's alternative services brokenness expiration behavior. HttpServerPropertiesImpl is used to store HTTP alternative services that have failed and keeps track of their time-until-retry (i.e. when their brokenness expires). The broken alt-svcs and their respective expiration times are kept in a list ordered by expiration times. When the expiration task runs, it drops items from the front of the list until the expiration time of the head alt-svc is no longer in the past. Turns out this list was kept in insertion order as opposed to sorted order by expiration. This change keeps this queue in sorted order, as expected. The additional logic added to implement the above change lead to a refactor of HttpServerPropertiesImpl: its broken alt-svc logic is put in its own class, BrokenAlternativeServices. BUG=724302 ==========
Description was changed from ========== Fix and refactor HttpServerPropertiesImpl's alternative services brokenness expiration behavior. HttpServerPropertiesImpl is used to store HTTP alternative services that have failed and keeps track of their time-until-retry (i.e. when their brokenness expires). The broken alt-svcs and their respective expiration times are kept in a list ordered by expiration times. When the expiration task runs, it drops items from the front of the list until the expiration time of the head alt-svc is no longer in the past. Turns out this list was kept in insertion order as opposed to sorted order by expiration. This change keeps this queue in sorted order, as expected. The additional logic added to implement the above change lead to a refactor of HttpServerPropertiesImpl: its broken alt-svc logic is put in its own class, BrokenAlternativeServices. BUG=724302 ========== to ========== Fix and refactor HttpServerPropertiesImpl's alternative services brokenness expiration behavior. One of HttpServerPropertiesImpl's responsibilities is to store HTTP alternative services that have failed and keeps track of their time-until-retry (i.e. when their brokenness expires). The broken alt-svcs and their respective expiration times are kept in a list ordered by expiration time. When the expiration task runs, it drops items from the front of the list until the expiration time of the head alt-svc is no longer in the past. Turns out this list was kept in insertion order as opposed to expiration-sorted order. This change keeps this queue in expiration-sorted order, as expected. The additional logic added to implement the above change led to a refactor of HttpServerPropertiesImpl: its broken alt-svc logic is put in its own class, BrokenAlternativeServices. BUG=724302 ==========
The CQ bit was checked by wangyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org Link to the patchset: https://codereview.chromium.org/2898983006/#ps360001 (title: "Fixed cherie's comments from ps9; added BrokenAlternativeServicesTest.ScheduleExpireTaskAfterExpire")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 360001, "attempt_start_ts": 1496358416177190,
"parent_rev": "3f7a5c750ed5f1373506b11003f1834bea0a1d99", "commit_rev":
"64ccc57c1a20a8b2804df8a4e9be42b33b5b8f61"}
Message was sent while issue was closed.
Description was changed from ========== Fix and refactor HttpServerPropertiesImpl's alternative services brokenness expiration behavior. One of HttpServerPropertiesImpl's responsibilities is to store HTTP alternative services that have failed and keeps track of their time-until-retry (i.e. when their brokenness expires). The broken alt-svcs and their respective expiration times are kept in a list ordered by expiration time. When the expiration task runs, it drops items from the front of the list until the expiration time of the head alt-svc is no longer in the past. Turns out this list was kept in insertion order as opposed to expiration-sorted order. This change keeps this queue in expiration-sorted order, as expected. The additional logic added to implement the above change led to a refactor of HttpServerPropertiesImpl: its broken alt-svc logic is put in its own class, BrokenAlternativeServices. BUG=724302 ========== to ========== Fix and refactor HttpServerPropertiesImpl's alternative services brokenness expiration behavior. One of HttpServerPropertiesImpl's responsibilities is to store HTTP alternative services that have failed and keeps track of their time-until-retry (i.e. when their brokenness expires). The broken alt-svcs and their respective expiration times are kept in a list ordered by expiration time. When the expiration task runs, it drops items from the front of the list until the expiration time of the head alt-svc is no longer in the past. Turns out this list was kept in insertion order as opposed to expiration-sorted order. This change keeps this queue in expiration-sorted order, as expected. The additional logic added to implement the above change led to a refactor of HttpServerPropertiesImpl: its broken alt-svc logic is put in its own class, BrokenAlternativeServices. BUG=724302 Review-Url: https://codereview.chromium.org/2898983006 Cr-Commit-Position: refs/heads/master@{#476475} Committed: https://chromium.googlesource.com/chromium/src/+/64ccc57c1a20a8b2804df8a4e9be... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:360001) as https://chromium.googlesource.com/chromium/src/+/64ccc57c1a20a8b2804df8a4e9be... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
