Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2690393005:
Cap the delay time for the main job when racing with QUIC for delayed (Closed)
|
Created:
3 years, 10 months ago by Zhongyi Shi Modified:
3 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCap the delay time for the main job when racing with QUIC for delayed
tcp case so that if http_server_properties has cached a inappropriate
srtt will not block the main job for a extremely long time.
BUG=691688
Review-Url: https://codereview.chromium.org/2690393005
Cr-Commit-Position: refs/heads/master@{#451066}
Committed: https://chromium.googlesource.com/chromium/src/+/326a7fdb3a840dd926b4b4ff14ca7fe24d87ffc0
Patch Set 1 #
Total comments: 2
Patch Set 2 : add comments #
Total comments: 9
Patch Set 3 : use test task runner with a mock timer to verify ResumeMainJob(s) are invoked at the right time #
Total comments: 2
Patch Set 4 : remove invoke methods on Resume() expectations #
Total comments: 3
Patch Set 5 : sync with master #
Messages
Total messages: 28 (11 generated)
Patchset #1 (id:1) has been deleted
zhongyi@chromium.org changed reviewers: + jri@chromium.org, rch@chromium.org
Let me know if the 3 seconds makes sense in this case.
lgtm One nit, but LGTM otherwise. https://codereview.chromium.org/2690393005/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2690393005/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:989: job_controller_, base::TimeDelta::FromSeconds(3))), Readability nit: Can you add a kMaxDelayTimeForMainJob constant, initialized to 3, at the top of this test and use the constant?
https://codereview.chromium.org/2690393005/diff/20001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2690393005/diff/20001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:989: job_controller_, base::TimeDelta::FromSeconds(3))), On 2017/02/15 22:08:26, Jana wrote: > Readability nit: Can you add a kMaxDelayTimeForMainJob constant, initialized to > 3, at the top of this test and use the constant? Done.
Thanks for doing this. Jana: are you cool with a 3 second limit here? https://codereview.chromium.org/2690393005/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2690393005/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:31: // The max time to delay the main job if is delayed TCP case. nit: // The maximum time to wait for the alternate job to complete before resuming the main job. https://codereview.chromium.org/2690393005/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:32: const base::TimeDelta kMaxDelayTimeForMainJob = base::TimeDelta::FromSeconds(3); Alas, this is a static initializer which is prohibited in chrome. The only global const objects we can have must be POD types. So perhaps: const int kMaxDelayTimeForMainJobSecs = 3; https://codereview.chromium.org/2690393005/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:667: delay > kMaxDelayTimeForMainJob ? kMaxDelayTimeForMainJob : delay; can you do: main_job_wait_time_ = std::min(delay, kMaxDelayTimeForMainJob); https://codereview.chromium.org/2690393005/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2690393005/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:995: job_controller_, kMaxDelayTimeForMainJob)), I don't' think I understand how this tests that the resume task is posted to be 3 sec in the future and not 100 sec in the future. Can you explain? https://codereview.chromium.org/2690393005/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:1000: } To confirm, this test fails with the old code?
Ryan, given that we'd want to make sure to give time for handshake retransmissions, 3 seconds seems reasonable.
Using test task runner with a mock timer to verify the ResumeMainJob is invoked at the right time, ptal! https://codereview.chromium.org/2690393005/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2690393005/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:31: // The max time to delay the main job if is delayed TCP case. On 2017/02/15 22:26:55, Ryan Hamilton wrote: > nit: // The maximum time to wait for the alternate job to complete before > resuming the main job. Done. https://codereview.chromium.org/2690393005/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:32: const base::TimeDelta kMaxDelayTimeForMainJob = base::TimeDelta::FromSeconds(3); On 2017/02/15 22:26:55, Ryan Hamilton wrote: > Alas, this is a static initializer which is prohibited in chrome. The only > global const objects we can have must be POD types. So perhaps: > > const int kMaxDelayTimeForMainJobSecs = 3; Done. https://codereview.chromium.org/2690393005/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller.cc:667: delay > kMaxDelayTimeForMainJob ? kMaxDelayTimeForMainJob : delay; On 2017/02/15 22:26:55, Ryan Hamilton wrote: > can you do: > > main_job_wait_time_ = std::min(delay, kMaxDelayTimeForMainJob); Done. https://codereview.chromium.org/2690393005/diff/40001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2690393005/diff/40001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:1000: } On 2017/02/15 22:26:55, Ryan Hamilton wrote: > To confirm, this test fails with the old code? Yup.
lgtm NICE! The tests are so much clearer now. Thanks! https://codereview.chromium.org/2690393005/diff/60001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2690393005/diff/60001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:930: base::TimeDelta::FromMicroseconds(15)))); You could probably drop the Invoke() part of this if you wanted to since it's testing private behavior which isn't important.
Thanks for helping the code review, landing now. https://codereview.chromium.org/2690393005/diff/60001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2690393005/diff/60001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:930: base::TimeDelta::FromMicroseconds(15)))); On 2017/02/15 23:37:57, Ryan Hamilton wrote: > You could probably drop the Invoke() part of this if you wanted to since it's > testing private behavior which isn't important. Done.
The CQ bit was checked by zhongyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jri@chromium.org, rch@chromium.org Link to the patchset: https://codereview.chromium.org/2690393005/#ps80001 (title: "remove invoke methods on Resume() expectations")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2690393005/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (left): https://codereview.chromium.org/2690393005/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:927: &JobControllerPeer::VerifyWaitingTimeForMainJob, job_controller_, Do we need this method any more? If not, feel free to land a followup to nuke it.
https://codereview.chromium.org/2690393005/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (left): https://codereview.chromium.org/2690393005/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:927: &JobControllerPeer::VerifyWaitingTimeForMainJob, job_controller_, On 2017/02/16 00:30:58, Ryan Hamilton wrote: > Do we need this method any more? If not, feel free to land a followup to nuke > it. Not any more! I'll do a clean up CL to change those tests to use test task runner.
lgtm https://codereview.chromium.org/2690393005/diff/80001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job_controller_unittest.cc (left): https://codereview.chromium.org/2690393005/diff/80001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job_controller_unittest.cc:927: &JobControllerPeer::VerifyWaitingTimeForMainJob, job_controller_, On 2017/02/16 00:33:38, Zhongyi Shi wrote: > On 2017/02/16 00:30:58, Ryan Hamilton wrote: > > Do we need this method any more? If not, feel free to land a followup to nuke > > it. > > Not any more! I'll do a clean up CL to change those tests to use test task > runner. \o/
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by zhongyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jri@chromium.org, rch@chromium.org Link to the patchset: https://codereview.chromium.org/2690393005/#ps100001 (title: "sync with master")
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by zhongyi@chromium.org
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": 100001, "attempt_start_ts": 1487269698448520,
"parent_rev": "7f21fa5131ea7281d0b990b461eb244574c3c9d4", "commit_rev":
"326a7fdb3a840dd926b4b4ff14ca7fe24d87ffc0"}
Message was sent while issue was closed.
Description was changed from ========== Cap the delay time for the main job when racing with QUIC for delayed tcp case so that if http_server_properties has cached a inappropriate srtt will not block the main job for a extremely long time. BUG=691688 ========== to ========== Cap the delay time for the main job when racing with QUIC for delayed tcp case so that if http_server_properties has cached a inappropriate srtt will not block the main job for a extremely long time. BUG=691688 Review-Url: https://codereview.chromium.org/2690393005 Cr-Commit-Position: refs/heads/master@{#451066} Committed: https://chromium.googlesource.com/chromium/src/+/326a7fdb3a840dd926b4b4ff14ca... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/326a7fdb3a840dd926b4b4ff14ca... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
