Remove ChromeOS specific retry logic
This CL remove the ChromeOS specific HTTP retries logic. Instead, it's migrating it to the URLLoader retry logic. Every platform will retry 3 times when the connection failed. The logic within the URLLoader is used instead of custom logic. bug: 1442469 Change-Id: I1b7fe9e54560e4bf9ff24c1b2940a30ebed3b8ae Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4739037 Reviewed-by: Roger Tawa <rogerta@chromium.org> Commit-Queue: Etienne Bergeron <etienneb@chromium.org> Cr-Commit-Position: refs/heads/main@{#1183224}
This commit is contained in:

committed by
Chromium LUCI CQ

parent
b22627d0d9
commit
bf3870bf87
@ -282,6 +282,13 @@ void PingRlzServer(std::string url,
|
||||
auto url_loader = network::SimpleURLLoader::Create(
|
||||
std::move(resource_request), traffic_annotation);
|
||||
|
||||
constexpr int kMaxNetworkRetries = 3;
|
||||
url_loader->SetRetryOptions(
|
||||
kMaxNetworkRetries,
|
||||
network::SimpleURLLoader::RetryMode::RETRY_ON_5XX |
|
||||
network::SimpleURLLoader::RETRY_ON_NETWORK_CHANGE |
|
||||
network::SimpleURLLoader::RETRY_ON_NAME_NOT_RESOLVED);
|
||||
|
||||
// Pass ownership of the loader to the bound function. Otherwise the load will
|
||||
// be canceled when the SimpleURLLoader object is destroyed.
|
||||
auto* url_loader_ptr = url_loader.get();
|
||||
|
@ -421,11 +421,7 @@ bool SendFinancialPing(Product product,
|
||||
const char* product_lang,
|
||||
bool exclude_machine_id,
|
||||
const bool skip_time_check) {
|
||||
// Create the financial ping request. To support ChromeOS retries, the
|
||||
// same request needs to be sent out each time in order to preserve the
|
||||
// machine id. Not doing so could cause the RLZ server to over count
|
||||
// ChromeOS machines under some network error conditions (for example,
|
||||
// the request is properly received but the response it not).
|
||||
// Create the financial ping request.
|
||||
std::string request;
|
||||
if (!FinancialPing::FormRequest(product, access_points, product_signature,
|
||||
product_brand, product_id, product_lang,
|
||||
@ -438,59 +434,17 @@ bool SendFinancialPing(Product product,
|
||||
|
||||
// Send out the ping, update the last ping time irrespective of success.
|
||||
FinancialPing::UpdateLastPingTime(product);
|
||||
|
||||
SYSLOG(INFO) << "Attempting to send RLZ ping brand=" << product_brand;
|
||||
std::string response;
|
||||
|
||||
#if BUILDFLAG(IS_CHROMEOS_ASH)
|
||||
const net::BackoffEntry::Policy policy = {
|
||||
0, // Number of initial errors to ignore.
|
||||
static_cast<int>(base::Seconds(5).InMilliseconds()), // Initial delay.
|
||||
2, // Factor to increase delay.
|
||||
0.1, // Delay fuzzing.
|
||||
static_cast<int>(base::Minutes(5).InMilliseconds()), // Maximum delay.
|
||||
-1, // Time to keep entries. -1 == never discard.
|
||||
};
|
||||
net::BackoffEntry backoff(&policy);
|
||||
|
||||
const int kMaxRetryCount = 3;
|
||||
FinancialPing::PingResponse res = FinancialPing::PING_FAILURE;
|
||||
while (backoff.failure_count() < kMaxRetryCount) {
|
||||
// Write to syslog that an RLZ ping is being attempted. This is
|
||||
// purposefully done via syslog so that admin and/or end users can monitor
|
||||
// RLZ activity from this machine. If RLZ is turned off in crosh, these
|
||||
// messages will be absent.
|
||||
SYSLOG(INFO) << "Attempting to send RLZ ping brand=" << product_brand;
|
||||
|
||||
res = FinancialPing::PingServer(request.c_str(), &response);
|
||||
if (res != FinancialPing::PING_FAILURE)
|
||||
break;
|
||||
|
||||
backoff.InformOfRequest(false);
|
||||
if (backoff.ShouldRejectRequest()) {
|
||||
SYSLOG(INFO) << "Failed sending RLZ ping - retrying in "
|
||||
<< backoff.GetTimeUntilRelease().InSeconds() << " seconds";
|
||||
}
|
||||
|
||||
base::PlatformThread::Sleep(backoff.GetTimeUntilRelease());
|
||||
}
|
||||
|
||||
FinancialPing::PingResponse res =
|
||||
FinancialPing::PingServer(request.c_str(), &response);
|
||||
if (res != FinancialPing::PING_SUCCESSFUL) {
|
||||
if (res == FinancialPing::PING_FAILURE) {
|
||||
SYSLOG(INFO) << "Failed sending RLZ ping after " << kMaxRetryCount
|
||||
<< " tries";
|
||||
} else { // res == FinancialPing::PING_SHUTDOWN
|
||||
SYSLOG(INFO) << "Failed sending RLZ ping due to chrome shutdown";
|
||||
}
|
||||
SYSLOG(INFO) << "Failed sending RLZ";
|
||||
return false;
|
||||
}
|
||||
|
||||
SYSLOG(INFO) << "Succeeded in sending RLZ ping";
|
||||
#else
|
||||
FinancialPing::PingResponse res =
|
||||
FinancialPing::PingServer(request.c_str(), &response);
|
||||
if (res != FinancialPing::PING_SUCCESSFUL)
|
||||
return false;
|
||||
#endif
|
||||
|
||||
return ParsePingResponse(product, response.c_str());
|
||||
}
|
||||
|
||||
|
@ -159,6 +159,8 @@ bool RLZ_LIB_API ParseFinancialPingResponse(Product product,
|
||||
// exclude_machine_id : Whether the Machine ID should be explicitly excluded
|
||||
// based on the products privacy policy.
|
||||
//
|
||||
// SendFinancialPing() will retry (best effort) the request on failures.
|
||||
//
|
||||
// Returns true on successful ping and response, false otherwise.
|
||||
// Access: HKCU write.
|
||||
bool RLZ_LIB_API SendFinancialPing(Product product,
|
||||
|
@ -56,6 +56,31 @@
|
||||
#include "rlz/chromeos/lib/rlz_value_store_chromeos.h"
|
||||
#endif
|
||||
|
||||
namespace {
|
||||
const char kProductSignature[] = "swg";
|
||||
const char kProductBrand[] = "GGLA";
|
||||
const char kProductId[] = "SwgProductId1234";
|
||||
const char kProductLang[] = "en-UK";
|
||||
|
||||
const rlz_lib::AccessPoint kAccessPoints[] = {rlz_lib::IETB_SEARCH_BOX,
|
||||
rlz_lib::NO_ACCESS_POINT,
|
||||
rlz_lib::NO_ACCESS_POINT};
|
||||
|
||||
const char kDefaultGoodPingResponse[] =
|
||||
"version: 3.0.914.7250\r\n"
|
||||
"url: "
|
||||
"http://www.corp.google.com/~av/45/opt/SearchWithGoogleUpdate.exe\r\n"
|
||||
"launch-action: custom-action\r\n"
|
||||
"launch-target: SearchWithGoogleUpdate.exe\r\n"
|
||||
"signature: c08a3f4438e1442c4fe5678ee147cf6c5516e5d62bb64e\r\n"
|
||||
"rlz: 1R1_____en__252\r\n"
|
||||
"rlzXX: 1R1_____en__250\r\n"
|
||||
"rlzT4 1T4_____en__251\r\n"
|
||||
"rlzT4: 1T4_____en__252\r\n"
|
||||
"rlz\r\n"
|
||||
"crc32: D6FD55A3";
|
||||
} // namespace
|
||||
|
||||
class MachineDealCodeHelper
|
||||
#if BUILDFLAG(IS_WIN)
|
||||
: public rlz_lib::MachineDealCode
|
||||
@ -77,34 +102,44 @@ class MachineDealCodeHelper
|
||||
|
||||
class RlzLibTest : public RlzLibTestBase {
|
||||
protected:
|
||||
void FakeGoodPingResponse(rlz_lib::Product product,
|
||||
const rlz_lib::AccessPoint* access_points,
|
||||
const char* product_signature,
|
||||
const char* product_brand,
|
||||
const char* product_id,
|
||||
const char* product_lang,
|
||||
bool exclude_machine_id,
|
||||
network::TestURLLoaderFactory* url_loader_factory) {
|
||||
const char kGoodPingResponses[] =
|
||||
"version: 3.0.914.7250\r\n"
|
||||
"url: "
|
||||
"http://www.corp.google.com/~av/45/opt/SearchWithGoogleUpdate.exe\r\n"
|
||||
"launch-action: custom-action\r\n"
|
||||
"launch-target: SearchWithGoogleUpdate.exe\r\n"
|
||||
"signature: c08a3f4438e1442c4fe5678ee147cf6c5516e5d62bb64e\r\n"
|
||||
"rlz: 1R1_____en__252\r\n"
|
||||
"rlzXX: 1R1_____en__250\r\n"
|
||||
"rlzT4 1T4_____en__251\r\n"
|
||||
"rlzT4: 1T4_____en__252\r\n"
|
||||
"rlz\r\n"
|
||||
"crc32: D6FD55A3";
|
||||
void RunUntilIdle() { task_environment_.RunUntilIdle(); }
|
||||
|
||||
bool SendFinancialPing(rlz_lib::Product product, bool exclude_machine_id) {
|
||||
bool success = rlz_lib::SendFinancialPing(
|
||||
product, kAccessPoints, kProductSignature, kProductBrand, kProductId,
|
||||
kProductLang, exclude_machine_id,
|
||||
/* skip_time_check */ true);
|
||||
RunUntilIdle();
|
||||
return success;
|
||||
}
|
||||
|
||||
void FakePingResponse(rlz_lib::Product product,
|
||||
bool exclude_machine_id,
|
||||
net::HttpStatusCode http_status_code,
|
||||
std::string content,
|
||||
network::TestURLLoaderFactory* url_loader_factory) {
|
||||
std::string request;
|
||||
EXPECT_TRUE(rlz_lib::FinancialPing::FormRequest(
|
||||
product, access_points, product_signature, product_brand, product_id,
|
||||
product_lang, exclude_machine_id, &request));
|
||||
product, kAccessPoints, kProductSignature, kProductBrand, kProductId,
|
||||
kProductLang, exclude_machine_id, &request));
|
||||
std::string url = base::StringPrintf(
|
||||
"https://%s%s", rlz_lib::kFinancialServer, request.c_str());
|
||||
url_loader_factory->AddResponse(url, kGoodPingResponses);
|
||||
url_loader_factory->AddResponse(url, content, http_status_code);
|
||||
}
|
||||
|
||||
void FakeGoodPingResponse(rlz_lib::Product product,
|
||||
bool exclude_machine_id,
|
||||
network::TestURLLoaderFactory* url_loader_factory) {
|
||||
FakePingResponse(product, exclude_machine_id, net::HttpStatusCode::HTTP_OK,
|
||||
kDefaultGoodPingResponse, url_loader_factory);
|
||||
}
|
||||
|
||||
void FakeBadPingResponse(rlz_lib::Product product,
|
||||
bool exclude_machine_id,
|
||||
net::HttpStatusCode http_status_code,
|
||||
network::TestURLLoaderFactory* url_loader_factory) {
|
||||
FakePingResponse(product, exclude_machine_id, http_status_code, "",
|
||||
url_loader_factory);
|
||||
}
|
||||
|
||||
base::test::TaskEnvironment task_environment_;
|
||||
@ -595,20 +630,43 @@ TEST_F(RlzLibTest, SendFinancialPing) {
|
||||
EXPECT_TRUE(rlz_lib::RecordProductEvent(rlz_lib::TOOLBAR_NOTIFIER,
|
||||
rlz_lib::IE_HOME_PAGE, rlz_lib::INSTALL));
|
||||
|
||||
rlz_lib::AccessPoint points[] =
|
||||
{rlz_lib::IETB_SEARCH_BOX, rlz_lib::NO_ACCESS_POINT,
|
||||
rlz_lib::NO_ACCESS_POINT};
|
||||
// Excluding machine id from requests so that a stable URL is used and
|
||||
// this test can use TestURLLoaderFactory.
|
||||
FakeGoodPingResponse(rlz_lib::TOOLBAR_NOTIFIER,
|
||||
/* exclude_machine_id */ true, &test_url_loader_factory);
|
||||
// On success, only one request should happen.
|
||||
size_t expected_amount_of_request = 1;
|
||||
EXPECT_TRUE(SendFinancialPing(rlz_lib::TOOLBAR_NOTIFIER,
|
||||
/* exclude_machine_id */ true));
|
||||
EXPECT_EQ(test_url_loader_factory.total_requests(),
|
||||
expected_amount_of_request);
|
||||
|
||||
// Excluding machine id from requests so that a stable URL is used and
|
||||
// this test can use TestURLLoaderFactory.
|
||||
FakeGoodPingResponse(rlz_lib::TOOLBAR_NOTIFIER, points, "swg", "GGLA",
|
||||
"SwgProductId1234", "en-UK",
|
||||
/* exclude_machine_id */ true, &test_url_loader_factory);
|
||||
FakeBadPingResponse(rlz_lib::TOOLBAR_NOTIFIER,
|
||||
/* exclude_machine_id */ true,
|
||||
net::HttpStatusCode::HTTP_FORBIDDEN,
|
||||
&test_url_loader_factory);
|
||||
// For an error HTTP_FORBIDDEN, the URLLoader should not retries.
|
||||
expected_amount_of_request += 1;
|
||||
EXPECT_FALSE(SendFinancialPing(rlz_lib::TOOLBAR_NOTIFIER,
|
||||
/* exclude_machine_id */ true));
|
||||
EXPECT_EQ(test_url_loader_factory.total_requests(),
|
||||
expected_amount_of_request);
|
||||
|
||||
rlz_lib::SendFinancialPing(rlz_lib::TOOLBAR_NOTIFIER, points, "swg", "GGLA",
|
||||
"SwgProductId1234", "en-UK",
|
||||
/* exclude_machine_id */ true,
|
||||
/* skip_time_check */ true);
|
||||
// Excluding machine id from requests so that a stable URL is used and
|
||||
// this test can use TestURLLoaderFactory.
|
||||
FakeBadPingResponse(rlz_lib::TOOLBAR_NOTIFIER,
|
||||
/* exclude_machine_id */ true,
|
||||
net::HttpStatusCode::HTTP_SERVICE_UNAVAILABLE,
|
||||
&test_url_loader_factory);
|
||||
// For an error HTTP_SERVICE_UNAVAILABLE, the URLLoader should try once and
|
||||
// then retry 3 times.
|
||||
expected_amount_of_request += 4;
|
||||
EXPECT_FALSE(SendFinancialPing(rlz_lib::TOOLBAR_NOTIFIER,
|
||||
/* exclude_machine_id */ true));
|
||||
EXPECT_EQ(test_url_loader_factory.total_requests(),
|
||||
expected_amount_of_request);
|
||||
}
|
||||
|
||||
void ResetURLLoaderFactory() {
|
||||
@ -632,19 +690,13 @@ TEST_F(RlzLibTest, SendFinancialPingDuringShutdown) {
|
||||
URLLoaderFactoryRAII set_factory(
|
||||
test_url_loader_factory.GetSafeWeakWrapper().get());
|
||||
|
||||
rlz_lib::AccessPoint points[] =
|
||||
{rlz_lib::IETB_SEARCH_BOX, rlz_lib::NO_ACCESS_POINT,
|
||||
rlz_lib::NO_ACCESS_POINT};
|
||||
rlz_lib::test::ResetSendFinancialPingInterrupted();
|
||||
EXPECT_FALSE(rlz_lib::test::WasSendFinancialPingInterrupted());
|
||||
|
||||
io_thread.task_runner()->PostTask(FROM_HERE,
|
||||
base::BindOnce(&ResetURLLoaderFactory));
|
||||
|
||||
rlz_lib::SendFinancialPing(rlz_lib::TOOLBAR_NOTIFIER, points, "swg", "GGLA",
|
||||
"SwgProductId1234", "en-UK",
|
||||
/* exclude_machine_id */ false,
|
||||
/* skip_time_check */ true);
|
||||
SendFinancialPing(rlz_lib::TOOLBAR_NOTIFIER, /* exclude_machine_id */ false);
|
||||
|
||||
EXPECT_TRUE(rlz_lib::test::WasSendFinancialPingInterrupted());
|
||||
rlz_lib::test::ResetSendFinancialPingInterrupted();
|
||||
|
Reference in New Issue
Block a user