Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1169163002:
Add timeout check for connectivity_checker. (Closed)
|
Created:
5 years, 6 months ago by wzhong Modified:
5 years, 6 months ago CC:
chromium-reviews, gunsch+watch_chromium.org, cbentzel+watch_chromium.org, lcwu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd timeout check for connectivity_checker.
In case that no response has been received within 3 seconds,
cancel the current request and start a new one.
BUG=internal b/21298386
Committed: https://crrev.com/cb79e44ab1af04eec4cdeb6520c394bee307ae92
Cr-Commit-Position: refs/heads/master@{#333634}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Add timeout check for connectivity_checker. #
Total comments: 3
Patch Set 3 : Add timeout check for connectivity_checker. #
Total comments: 4
Patch Set 4 : Add timeout check for connectivity_checker. #
Total comments: 4
Patch Set 5 : Add timeout check for connectivity_checker #Messages
Total messages: 25 (3 generated)
wzhong@chromium.org changed reviewers: + byungchul@chromium.org, derekjchow@chromium.org, gunsch@chromium.org
https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... chromecast/net/connectivity_checker_impl.cc:177: if (request_index_ == request_index && !url_request_.get()) { If the Check fails (returns 404), this function is still called. Consequently, OnURLRequestError is called twice. Please return early if request_index != request_index_ or consider using cancellable closure.
https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... chromecast/net/connectivity_checker_impl.cc:178: LOG(INFO) << "OnUrlRequestTimeOut, request has completed"; Is this going to result in a log on every successful connectivity check?
https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... chromecast/net/connectivity_checker_impl.cc:108: ++request_index_; Instead of using a request_index, could we store url_request_.get() in the closure? When the closure is called, can we compare the URLRequest pointers? https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... chromecast/net/connectivity_checker_impl.cc:178: LOG(INFO) << "OnUrlRequestTimeOut, request has completed"; Change to VLOG(3) or remove, since we expected the request to complete.
https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... chromecast/net/connectivity_checker_impl.cc:108: ++request_index_; On 2015/06/09 17:59:01, derekjchow1 wrote: > Instead of using a request_index, could we store url_request_.get() in the > closure? When the closure is called, can we compare the URLRequest pointers? We could, but in general I want to avoid using scoped_refptr when scope_ptr suffices. https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... chromecast/net/connectivity_checker_impl.cc:177: if (request_index_ == request_index && !url_request_.get()) { On 2015/06/09 17:53:08, derekjchow1 wrote: > If the Check fails (returns 404), this function is still called. Consequently, > OnURLRequestError is called twice. Please return early if request_index != > request_index_ or consider using cancellable closure. Acknowledged. https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... chromecast/net/connectivity_checker_impl.cc:178: LOG(INFO) << "OnUrlRequestTimeOut, request has completed"; On 2015/06/09 17:59:01, derekjchow1 wrote: > Change to VLOG(3) or remove, since we expected the request to complete. Currently problem is too few logging in connectivity_checker, so we don't know what is happening and it is difficult to diagnose problem like b/21298386 reported from feedback. The rate is limited anyway. I don't mind removing it later once we have much fewer "reconnect me" problems.
https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... chromecast/net/connectivity_checker_impl.cc:178: LOG(INFO) << "OnUrlRequestTimeOut, request has completed"; How is it rate limited? It looks like it will occur once every connectivity check, which is once per second?
https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... chromecast/net/connectivity_checker_impl.cc:178: LOG(INFO) << "OnUrlRequestTimeOut, request has completed"; On 2015/06/09 18:13:35, gunsch wrote: > How is it rate limited? It looks like it will occur once every connectivity > check, which is once per second? Less. If a connectivity check succeeds, it won't preform another until the IP changes, or a resource load fails. In the normal case, this will only occur at startup. However, if the internet/wifi connection becomes flaky, these message may start to show up alot...
On 2015/06/09 18:24:15, derekjchow1 wrote: > https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... > File chromecast/net/connectivity_checker_impl.cc (right): > > https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... > chromecast/net/connectivity_checker_impl.cc:178: LOG(INFO) << > "OnUrlRequestTimeOut, request has completed"; > On 2015/06/09 18:13:35, gunsch wrote: > > How is it rate limited? It looks like it will occur once every connectivity > > check, which is once per second? > > Less. If a connectivity check succeeds, it won't preform another until the IP > changes, or a resource load fails. In the normal case, this will only occur at > startup. > > However, if the internet/wifi connection becomes flaky, these message may start > to show up alot... On a second thought - we should enable more verbose logging for connectivity_checking temporarily in command line switch. And this should be VLOG().
https://codereview.chromium.org/1169163002/diff/20001/chromecast/net/connecti... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/20001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:143: url_request_.reset(NULL); // URLRequest::Cancel() is called in destructor. increase request_index_? then, you don't need line 179~182. And, CancelableCallback is preferred. https://codereview.chromium.org/1169163002/diff/20001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:159: LOG(ERROR) << "OnSSLCertificateError"; ditto
https://codereview.chromium.org/1169163002/diff/20001/chromecast/net/connecti... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/20001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:143: url_request_.reset(NULL); // URLRequest::Cancel() is called in destructor. On 2015/06/09 20:48:26, byungchul wrote: > increase request_index_? then, you don't need line 179~182. > > And, CancelableCallback is preferred. Just realized that need to increment for all overrides. Agreed. Given that, CancelableCallback is preferred.
Patchset #3 uses CancelableCallback. https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/1/chromecast/net/connectivity... chromecast/net/connectivity_checker_impl.cc:177: if (request_index_ == request_index && !url_request_.get()) { On 2015/06/09 17:53:08, derekjchow1 wrote: > If the Check fails (returns 404), this function is still called. Consequently, > OnURLRequestError is called twice. Please return early if request_index != > request_index_ or consider using cancellable closure. Done.
https://codereview.chromium.org/1169163002/diff/40001/chromecast/net/connecti... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/40001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:191: url_request_.reset(NULL); // URLRequest::Cancel() is called in destructor. timeout_.Cancel(); here as well.
https://codereview.chromium.org/1169163002/diff/40001/chromecast/net/connecti... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/40001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:171: ++check_errors_; Should this case also cancel the timeout?
https://codereview.chromium.org/1169163002/diff/40001/chromecast/net/connecti... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/40001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:171: ++check_errors_; On 2015/06/09 22:19:11, gunsch wrote: > Should this case also cancel the timeout? Not override. Already called at callsite. https://codereview.chromium.org/1169163002/diff/40001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:191: url_request_.reset(NULL); // URLRequest::Cancel() is called in destructor. On 2015/06/09 22:19:07, derekjchow1 wrote: > timeout_.Cancel(); here as well. Done.
lgtm % Derek
lgtm, make sure gunsch@ and byungchul@ sign off as well.
https://codereview.chromium.org/1169163002/diff/60001/chromecast/net/connecti... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/60001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:71: timeout_.Cancel(); not necessary. Will be done at the end of dtor. https://codereview.chromium.org/1169163002/diff/60001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:189: if (url_request_.get()) { fast return on false condition
https://codereview.chromium.org/1169163002/diff/60001/chromecast/net/connecti... File chromecast/net/connectivity_checker_impl.cc (right): https://codereview.chromium.org/1169163002/diff/60001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:71: timeout_.Cancel(); On 2015/06/09 22:45:23, byungchul wrote: > not necessary. Will be done at the end of dtor. Done. https://codereview.chromium.org/1169163002/diff/60001/chromecast/net/connecti... chromecast/net/connectivity_checker_impl.cc:189: if (url_request_.get()) { On 2015/06/09 22:45:23, byungchul wrote: > fast return on false condition Done.
lgtm
The CQ bit was checked by wzhong@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derekjchow@chromium.org, gunsch@chromium.org Link to the patchset: https://codereview.chromium.org/1169163002/#ps80001 (title: "Add timeout check for connectivity_checker")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1169163002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/cb79e44ab1af04eec4cdeb6520c394bee307ae92 Cr-Commit-Position: refs/heads/master@{#333634} |
