shill: simplify PortalDetector start and stop trial
This is a reland of crrev/c/2760640 Change-Id
I4c4b4734324370669bfb0debb32b52e7b592419e. In this new version,
CleanupTrial() is called before invoking |portal_result_callback_| and
the ordering is unchanged.
This patch folds together multiple functions calling into each other
when a portal detection trial is started or stopped. The result is less
code and a logical flow that is easier to follow.
Also, this patch ensures that a metric event is recorded if the http
probe fails to start. Previously no event would be recorded in that
case.
BUG=b:182745554
TEST=unit tests. Checked portal validation on WiFi and Ethernet.
Change-Id: I0caf600eb82dd97544d3b82b1d558b40d12d8a23
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2790193
Tested-by: Hugo Benichi <hugobenichi@google.com>
Commit-Queue: Hugo Benichi <hugobenichi@google.com>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
diff --git a/shill/portal_detector.cc b/shill/portal_detector.cc
index c5389ec..f6b6dc8 100644
--- a/shill/portal_detector.cc
+++ b/shill/portal_detector.cc
@@ -77,20 +77,6 @@
Stop();
}
-bool PortalDetector::StartAfterDelay(const PortalDetector::Properties& props,
- int delay_seconds) {
- SLOG(connection_.get(), 3) << "In " << __func__;
-
- if (!StartTrial(props, delay_seconds * 1000)) {
- return false;
- }
- // The attempt_start_time_ is calculated based on the current time and
- // |delay_seconds|. This is used to determine if a portal detection attempt
- // is in progress.
- UpdateAttemptTime(delay_seconds);
- return true;
-}
-
const string PortalDetector::PickHttpProbeUrl(const Properties& props) {
if (attempt_count_ == 0 || props.fallback_http_url_strings.empty()) {
return props.http_url_string;
@@ -99,8 +85,8 @@
0, props.fallback_http_url_strings.size() - 1)];
}
-bool PortalDetector::StartTrial(const Properties& props,
- int start_delay_milliseconds) {
+bool PortalDetector::StartAfterDelay(const PortalDetector::Properties& props,
+ int delay_seconds) {
SLOG(connection_.get(), 3) << "In " << __func__;
logging_tag_ = connection_->interface_name() + " " +
@@ -116,6 +102,7 @@
<< props.http_url_string;
return false;
}
+
if (!https_url.ParseFromString(https_url_string_)) {
LOG(ERROR) << "Failed to parse HTTPS probe URL string: "
<< props.https_url_string;
@@ -139,17 +126,15 @@
dispatcher_, LoggingTag() + " HTTPS probe", iface, src_address,
dns_list, allow_non_google_https);
}
- StartTrialAfterDelay(start_delay_milliseconds);
- return true;
-}
-
-void PortalDetector::StartTrialAfterDelay(int start_delay_milliseconds) {
- SLOG(connection_.get(), 4)
- << "In " << __func__ << " delay = " << start_delay_milliseconds << "ms.";
trial_.Reset(
Bind(&PortalDetector::StartTrialTask, weak_ptr_factory_.GetWeakPtr()));
dispatcher_->PostDelayedTask(FROM_HERE, trial_.callback(),
- start_delay_milliseconds);
+ delay_seconds * 1000);
+ // The attempt_start_time_ is calculated based on the current time and
+ // |delay_seconds|. This is used to determine if a portal detection attempt
+ // is in progress.
+ UpdateAttemptTime(delay_seconds);
+ return true;
}
void PortalDetector::StartTrialTask() {
@@ -167,7 +152,8 @@
if (http_result != HttpRequest::kResultInProgress) {
// If the http probe fails to start, complete the trial with a failure
// Result for https.
- LOG(ERROR) << LoggingTag() << ": HTTP probe failed to start";
+ LOG(ERROR) << LoggingTag()
+ << ": HTTP probe failed to start. Aborting trial.";
CompleteTrial(PortalDetector::GetPortalResultForRequestResult(http_result),
Result(Phase::kContent, Status::kFailure));
return;
@@ -187,6 +173,8 @@
https_result_ =
std::make_unique<Result>(GetPortalResultForRequestResult(https_result));
LOG(ERROR) << LoggingTag() << ": HTTPS probe failed to start";
+ // To find the portal sign-in url, wait for the HTTP probe to complete
+ // before completing the trial and calling |portal_result_callback_|.
}
is_active_ = true;
}
@@ -201,7 +189,10 @@
<< ", status=" << http_result.status
<< ". HTTPS probe: phase=" << https_result.phase
<< ", status=" << https_result.status;
- CompleteAttempt(http_result, https_result);
+ http_result.num_attempts = attempt_count_;
+ metrics_->NotifyPortalDetectionMultiProbeResult(http_result, https_result);
+ CleanupTrial();
+ portal_result_callback_.Run(http_result, https_result);
}
void PortalDetector::CleanupTrial() {
@@ -227,14 +218,6 @@
https_request_.reset();
}
-void PortalDetector::CompleteRequest() {
- if (https_result_ && http_result_) {
- metrics_->NotifyPortalDetectionMultiProbeResult(*http_result_,
- *https_result_);
- CompleteTrial(*http_result_.get(), *https_result_.get());
- }
-}
-
void PortalDetector::HttpRequestSuccessCallback(
std::shared_ptr<brillo::http::Response> response) {
// TODO(matthewmwang): check for 0 length data as well
@@ -265,8 +248,8 @@
LOG(INFO) << LoggingTag() << ": HTTP probe response code=" << status_code
<< " status=" << http_result_->status;
http_result_->status_code = status_code;
-
- CompleteRequest();
+ if (https_result_)
+ CompleteTrial(*http_result_, *https_result_);
}
void PortalDetector::HttpsRequestSuccessCallback(
@@ -280,7 +263,8 @@
LOG(INFO) << LoggingTag() << ": HTTPS probe response code=" << status_code
<< " status=" << probe_status;
https_result_ = std::make_unique<Result>(Phase::kContent, probe_status);
- CompleteRequest();
+ if (http_result_)
+ CompleteTrial(*http_result_, *https_result_);
}
void PortalDetector::HttpRequestErrorCallback(HttpRequest::Result result) {
@@ -289,7 +273,8 @@
LOG(INFO) << LoggingTag()
<< ": HTTP probe failed with phase=" << http_result_.get()->phase
<< " status=" << http_result_.get()->status;
- CompleteRequest();
+ if (https_result_)
+ CompleteTrial(*http_result_, *https_result_);
}
void PortalDetector::HttpsRequestErrorCallback(HttpRequest::Result result) {
@@ -298,19 +283,14 @@
LOG(INFO) << LoggingTag()
<< ": HTTPS probe failed with phase=" << https_result_.get()->phase
<< " status=" << https_result_.get()->status;
- CompleteRequest();
+ if (http_result_)
+ CompleteTrial(*http_result_, *https_result_);
}
bool PortalDetector::IsInProgress() {
return is_active_;
}
-void PortalDetector::CompleteAttempt(Result http_result, Result https_result) {
- http_result.num_attempts = attempt_count_;
- CleanupTrial();
- portal_result_callback_.Run(http_result, https_result);
-}
-
void PortalDetector::UpdateAttemptTime(int delay_seconds) {
time_->GetTimeMonotonic(&attempt_start_time_);
struct timeval delay_timeval = {delay_seconds, 0};
diff --git a/shill/portal_detector.h b/shill/portal_detector.h
index 5413d40..6baf972 100644
--- a/shill/portal_detector.h
+++ b/shill/portal_detector.h
@@ -148,6 +148,7 @@
//
// As each attempt completes the callback handed to the constructor will
// be called.
+ // TODO(b/184036481): Change |delay_seconds| type to base::TimeDelay.
virtual bool StartAfterDelay(const Properties& props, int delay_seconds);
// End the current portal detection process if one exists, and do not call
@@ -185,29 +186,10 @@
// to keep attempts spaced by the right duration in the event of a retry.
void UpdateAttemptTime(int delay_seconds);
- // Called after each trial to return |http_result| and |https_result| after
- // attempting to determine connectivity status.
- void CompleteAttempt(Result http_result, Result https_result);
-
- // Start a trial with the supplied delay in ms.
- void StartTrialAfterDelay(int start_delay_milliseconds);
-
- // Start a portal detection test. Returns true if |props.http_url_string| and
- // |props.https_url_string| correctly parse as URLs. Returns false (and does
- // not start) if they fail to parse.
- //
- // After a trial completes, the callback supplied in the constructor is
- // called.
- bool StartTrial(const Properties& props, int start_delay_milliseconds);
-
// Internal method used to start the actual connectivity trial, called after
// the start delay completes.
void StartTrialTask();
- // Called after a request finishes. Will call CompleteTrial once all probes
- // are done.
- void CompleteRequest();
-
// Callback used to return data read from the HTTP HttpRequest.
void HttpRequestSuccessCallback(
std::shared_ptr<brillo::http::Response> response);
@@ -222,7 +204,8 @@
// Callback used to return the error from the HTTPS HttpRequest.
void HttpsRequestErrorCallback(HttpRequest::Result result);
- // Internal method used to clean up state and call CompleteAttempt.
+ // Called after each trial to return |http_result| and |https_result| after
+ // attempting to determine connectivity status.
void CompleteTrial(Result http_result, Result https_result);
// Internal method used to cancel the timeout timer and stop an active
diff --git a/shill/portal_detector_test.cc b/shill/portal_detector_test.cc
index 11c2f54..542ca1d 100644
--- a/shill/portal_detector_test.cc
+++ b/shill/portal_detector_test.cc
@@ -389,7 +389,7 @@
HttpRequest::kResultDNSFailure);
PortalDetector::Result https_result = PortalDetector::Result(
PortalDetector::Phase::kContent, PortalDetector::Status::kFailure);
- portal_detector()->CompleteAttempt(r, https_result);
+ portal_detector()->CompleteTrial(r, https_result);
init_delay *= 2;
}
portal_detector()->Stop();
