Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925733003:
Report signling errors for register-support-host requests. (Closed)
|
Created:
3 years, 6 months ago by kelvinp Modified:
3 years, 6 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, chromoting-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReport signling errors for register-support-host requests.
Summary of changes:
Report an error message if:
1. SignalStrategy->SendStanza() fails.
2. There are no responses from the Bot 10 seconds after the request is
sent.
BUG=729854
Review-Url: https://codereview.chromium.org/2925733003
Cr-Commit-Position: refs/heads/master@{#478912}
Committed: https://chromium.googlesource.com/chromium/src/+/e7e7635584e929a13fbcb2cd87ed4779342e6342
Patch Set 1 #
Total comments: 13
Patch Set 2 : Reviewer's feedback #Patch Set 3 : Fix merge conflict #Patch Set 4 : Fix memory leak #
Messages
Total messages: 27 (17 generated)
Description was changed from ========== Report signling errors for register-support-host requests. BUG=729854 ========== to ========== Report signling errors for register-support-host requests. Summary of changes: 1. Return SIGNALING_ERROR if the SignalStrategy->SendStanza() fails. 2. Return SIGNALING_TIMEOUT if there is no signaling response after 10 seconds. BUG=729854 ==========
kelvinp@chromium.org changed reviewers: + sergeyu@chromium.org
PTAL https://codereview.chromium.org/2925733003/diff/1/remoting/host/it2me/it2me_h... File remoting/host/it2me/it2me_host_unittest.cc (right): https://codereview.chromium.org/2925733003/diff/1/remoting/host/it2me/it2me_h... remoting/host/it2me/it2me_host_unittest.cc:215: fake_client_signal_strategy_->ConnectTo(fake_signal_strategy.get()); Since now we report signaling errors properly, we would need to ensure the signaling channel is connected or else StartHost() would fail with SIGNALING_ERROR.
https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... File remoting/host/register_support_host_request.cc (right): https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... remoting/host/register_support_host_request.cc:48: const int kRegisterRequestTimeoutInSeconds = 10; nit: use constexpr base::TimeDelta: constexpr base::TimeDelta kRegisterRequestTimeout = base::TimeDelta::FromSeconds(10); https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... remoting/host/register_support_host_request.cc:80: if (!request_.get()) { nit: don't need .get() https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... remoting/host/register_support_host_request.cc:82: protocol::ErrorCodeToString(protocol::ErrorCode::SIGNALING_ERROR); error_message should a human-readable message that's logged in the console for debugging. https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... remoting/host/register_support_host_request.cc:162: protocol::ErrorCodeToString(protocol::ErrorCode::SIGNALING_TIMEOUT); same here. Use a readable message please. https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... remoting/host/register_support_host_request.cc:163: LOG(ERROR) << *error_message; I don't think we need to log the message here. Or at least move it to the ProcessResponse(), so it doesn't have to be duplicated for each error here. https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... File remoting/host/register_support_host_request_unittest.cc (right): https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... remoting/host/register_support_host_request_unittest.cc:97: mock_time_task_runner_->FastForwardBy(base::TimeDelta::FromSeconds(15)); Where is mock_time_task_runner_ defined?
Description was changed from ========== Report signling errors for register-support-host requests. Summary of changes: 1. Return SIGNALING_ERROR if the SignalStrategy->SendStanza() fails. 2. Return SIGNALING_TIMEOUT if there is no signaling response after 10 seconds. BUG=729854 ========== to ========== Report signling errors for register-support-host requests. Summary of changes: Report an error message if: 1. SignalStrategy->SendStanza() fails. 2. There are no responses from the Bot 10 seconds after the request is sent. BUG=729854 ==========
PTAL https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... File remoting/host/register_support_host_request.cc (right): https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... remoting/host/register_support_host_request.cc:48: const int kRegisterRequestTimeoutInSeconds = 10; On 2017/06/07 06:41:30, Sergey Ulanov wrote: > nit: use constexpr base::TimeDelta: > > constexpr base::TimeDelta kRegisterRequestTimeout = > base::TimeDelta::FromSeconds(10); Done. https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... remoting/host/register_support_host_request.cc:80: if (!request_.get()) { On 2017/06/07 06:41:29, Sergey Ulanov wrote: > nit: don't need .get() Done. https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... remoting/host/register_support_host_request.cc:82: protocol::ErrorCodeToString(protocol::ErrorCode::SIGNALING_ERROR); On 2017/06/07 06:41:29, Sergey Ulanov wrote: > error_message should a human-readable message that's logged in the console for > debugging. Done. https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... remoting/host/register_support_host_request.cc:162: protocol::ErrorCodeToString(protocol::ErrorCode::SIGNALING_TIMEOUT); On 2017/06/07 06:41:30, Sergey Ulanov wrote: > same here. Use a readable message please. Done. https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... remoting/host/register_support_host_request.cc:163: LOG(ERROR) << *error_message; On 2017/06/07 06:41:30, Sergey Ulanov wrote: > I don't think we need to log the message here. Or at least move it to the > ProcessResponse(), so it doesn't have to be duplicated for each error here. We log the error message in all other branches of this function though. Nevertheless, I moved the error message logging to ProcessResponse(). https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... File remoting/host/register_support_host_request_unittest.cc (right): https://codereview.chromium.org/2925733003/diff/1/remoting/host/register_supp... remoting/host/register_support_host_request_unittest.cc:97: mock_time_task_runner_->FastForwardBy(base::TimeDelta::FromSeconds(15)); On 2017/06/07 06:41:30, Sergey Ulanov wrote: > Where is mock_time_task_runner_ defined? Good catch. It is a bad merge =(
The CQ bit was checked by kelvinp@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: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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_...)
lgtm
The CQ bit was checked by kelvinp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2925733003/#ps40001 (title: "Fix merge conflict")
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kelvinp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2925733003/#ps60001 (title: "Fix memory leak")
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kelvinp@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": 60001, "attempt_start_ts": 1497331528608640,
"parent_rev": "beeedb8662da2bed75581e0d119d2666656a2edc", "commit_rev":
"d6abf7281453476b7e3942643151de1660a63092"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497331528608640,
"parent_rev": "08ef334a3dac9ac9faffc41dfd27a6a95ab0f878", "commit_rev":
"e7e7635584e929a13fbcb2cd87ed4779342e6342"}
Message was sent while issue was closed.
Description was changed from ========== Report signling errors for register-support-host requests. Summary of changes: Report an error message if: 1. SignalStrategy->SendStanza() fails. 2. There are no responses from the Bot 10 seconds after the request is sent. BUG=729854 ========== to ========== Report signling errors for register-support-host requests. Summary of changes: Report an error message if: 1. SignalStrategy->SendStanza() fails. 2. There are no responses from the Bot 10 seconds after the request is sent. BUG=729854 Review-Url: https://codereview.chromium.org/2925733003 Cr-Commit-Position: refs/heads/master@{#478912} Committed: https://chromium.googlesource.com/chromium/src/+/e7e7635584e929a13fbcb2cd87ed... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e7e7635584e929a13fbcb2cd87ed... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
