diff --git a/net/dns/dns_config_service.cc b/net/dns/dns_config_service.cc index 69aa66c6ddfef5..1aafa73664a41e 100644 --- a/net/dns/dns_config_service.cc +++ b/net/dns/dns_config_service.cc @@ -137,15 +137,17 @@ DnsConfigService::HostsReader::CreateWorkItem() { std::make_unique(hosts_file_path_)); } -void DnsConfigService::HostsReader::OnWorkFinished( +bool DnsConfigService::HostsReader::OnWorkFinished( std::unique_ptr serial_worker_work_item) { DCHECK(serial_worker_work_item); WorkItem* work_item = static_cast(serial_worker_work_item.get()); if (work_item->hosts_.has_value()) { service_->OnHostsRead(std::move(work_item->hosts_).value()); + return true; } else { LOG(WARNING) << "Failed to read DnsHosts."; + return false; } } diff --git a/net/dns/dns_config_service.h b/net/dns/dns_config_service.h index 7f11669430e43f..6afc7c06ddae2d 100644 --- a/net/dns/dns_config_service.h +++ b/net/dns/dns_config_service.h @@ -156,7 +156,7 @@ class NET_EXPORT_PRIVATE DnsConfigService { // SerialWorker: std::unique_ptr CreateWorkItem() override; - void OnWorkFinished( + bool OnWorkFinished( std::unique_ptr work_item) final; private: diff --git a/net/dns/dns_config_service_android.cc b/net/dns/dns_config_service_android.cc index 2110fc81ecbc96..f81856e50865ac 100644 --- a/net/dns/dns_config_service_android.cc +++ b/net/dns/dns_config_service_android.cc @@ -102,7 +102,7 @@ class DnsConfigServiceAndroid::ConfigReader : public SerialWorker { return std::make_unique(dns_server_getter_); } - void OnWorkFinished(std::unique_ptr + bool OnWorkFinished(std::unique_ptr serial_worker_work_item) override { DCHECK(serial_worker_work_item); DCHECK(!IsCancelled()); @@ -110,8 +110,10 @@ class DnsConfigServiceAndroid::ConfigReader : public SerialWorker { WorkItem* work_item = static_cast(serial_worker_work_item.get()); if (work_item->dns_config_.has_value()) { service_->OnConfigRead(std::move(work_item->dns_config_).value()); + return true; } else { LOG(WARNING) << "Failed to read DnsConfig."; + return false; } } diff --git a/net/dns/dns_config_service_linux.cc b/net/dns/dns_config_service_linux.cc index ffa7ad5ca0bbde..b762a2fc3ea27b 100644 --- a/net/dns/dns_config_service_linux.cc +++ b/net/dns/dns_config_service_linux.cc @@ -414,7 +414,7 @@ class DnsConfigServiceLinux::ConfigReader : public SerialWorker { return std::move(work_item_); } - void OnWorkFinished(std::unique_ptr + bool OnWorkFinished(std::unique_ptr serial_worker_work_item) override { DCHECK(serial_worker_work_item); DCHECK(!work_item_); @@ -423,8 +423,10 @@ class DnsConfigServiceLinux::ConfigReader : public SerialWorker { work_item_.reset(static_cast(serial_worker_work_item.release())); if (work_item_->dns_config_.has_value()) { service_->OnConfigRead(std::move(work_item_->dns_config_).value()); + return true; } else { LOG(WARNING) << "Failed to read DnsConfig."; + return false; } } diff --git a/net/dns/dns_config_service_posix.cc b/net/dns/dns_config_service_posix.cc index e59fec60e9d593..3cce5f20af78f4 100644 --- a/net/dns/dns_config_service_posix.cc +++ b/net/dns/dns_config_service_posix.cc @@ -182,7 +182,7 @@ class DnsConfigServicePosix::ConfigReader : public SerialWorker { return std::make_unique(); } - void OnWorkFinished(std::unique_ptr + bool OnWorkFinished(std::unique_ptr serial_worker_work_item) override { DCHECK(serial_worker_work_item); DCHECK(!IsCancelled()); @@ -190,8 +190,10 @@ class DnsConfigServicePosix::ConfigReader : public SerialWorker { WorkItem* work_item = static_cast(serial_worker_work_item.get()); if (work_item->dns_config_.has_value()) { service_->OnConfigRead(std::move(work_item->dns_config_).value()); + return true; } else { LOG(WARNING) << "Failed to read DnsConfig."; + return false; } } diff --git a/net/dns/dns_config_service_win.cc b/net/dns/dns_config_service_win.cc index 350b601fc0aa66..a3cab6e494d187 100644 --- a/net/dns/dns_config_service_win.cc +++ b/net/dns/dns_config_service_win.cc @@ -28,8 +28,6 @@ #include "base/synchronization/lock.h" #include "base/task/single_thread_task_runner.h" #include "base/threading/scoped_blocking_call.h" -#include "base/threading/sequenced_task_runner_handle.h" -#include "base/time/time.h" #include "base/win/registry.h" #include "base/win/scoped_handle.h" #include "base/win/windows_types.h" @@ -46,9 +44,6 @@ namespace internal { namespace { -// Interval between retries to parse config. Used only until parsing succeeds. -const int kRetryIntervalSeconds = 5; - // Registry key paths. const wchar_t kTcpipPath[] = L"SYSTEM\\CurrentControlSet\\Services\\Tcpip\\Parameters"; @@ -491,7 +486,8 @@ class DnsConfigServiceWin::Watcher // Reads config from registry and IpHelper. All work performed in ThreadPool. class DnsConfigServiceWin::ConfigReader : public SerialWorker { public: - explicit ConfigReader(DnsConfigServiceWin& service) : service_(&service) {} + explicit ConfigReader(DnsConfigServiceWin& service) + : SerialWorker(/*max_number_of_retries=*/3), service_(&service) {} ~ConfigReader() override {} // SerialWorker:: @@ -499,7 +495,7 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker { return std::make_unique(); } - void OnWorkFinished(std::unique_ptr + bool OnWorkFinished(std::unique_ptr serial_worker_work_item) override { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK(serial_worker_work_item); @@ -508,12 +504,10 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker { WorkItem* work_item = static_cast(serial_worker_work_item.get()); if (work_item->dns_config_.has_value()) { service_->OnConfigRead(std::move(work_item->dns_config_).value()); + return true; } else { LOG(WARNING) << "Failed to read DnsConfig."; - // Try again in a while in case DnsConfigWatcher missed the signal. - base::SequencedTaskRunnerHandle::Get()->PostDelayedTask( - FROM_HERE, base::BindOnce(&ConfigReader::WorkNow, AsWeakPtr()), - base::Seconds(kRetryIntervalSeconds)); + return false; } } diff --git a/net/dns/serial_worker.cc b/net/dns/serial_worker.cc index 980f68512b48fe..635a1a2afbe175 100644 --- a/net/dns/serial_worker.cc +++ b/net/dns/serial_worker.cc @@ -16,9 +16,25 @@ #include "base/task/thread_pool.h" #include "base/threading/sequenced_task_runner_handle.h" #include "base/threading/thread_task_runner_handle.h" +#include "base/timer/timer.h" +#include "net/base/backoff_entry.h" namespace net { +namespace { +// Default retry configuration. Only in effect if |max_number_of_retries| is +// greater than 0. +constexpr BackoffEntry::Policy kDefaultBackoffPolicy = { + 0, // Number of initial errors to ignore without backoff. + 5000, // Initial delay for backoff in ms: 5 seconds. + 2, // Factor to multiply for exponential backoff. + 0, // Fuzzing percentage. + -1, // No maximum delay. + -1, // Don't discard entry. + false // Don't use initial delay unless the last was an error. +}; +} // namespace + namespace { std::unique_ptr DoWork( std::unique_ptr work_item) { @@ -32,15 +48,28 @@ void SerialWorker::WorkItem::FollowupWork(base::OnceClosure closure) { std::move(closure).Run(); } -SerialWorker::SerialWorker() : state_(State::kIdle) {} +SerialWorker::SerialWorker(int max_number_of_retries, + const net::BackoffEntry::Policy* backoff_policy) + : state_(State::kIdle), + max_number_of_retries_(max_number_of_retries), + backoff_entry_(backoff_policy ? backoff_policy : &kDefaultBackoffPolicy) { +} SerialWorker::~SerialWorker() = default; void SerialWorker::WorkNow() { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + // Not a retry; reset failure count and cancel the pending retry (if any). + backoff_entry_.Reset(); + retry_timer_.Stop(); + WorkNowInternal(); +} + +void SerialWorker::WorkNowInternal() { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); switch (state_) { case State::kIdle: - // We are posting weak pointer to OnWorkJobFinished to avoid leak when + // We are posting weak pointer to OnWorkJobFinished to avoid a leak when // PostTaskAndReply fails to post task back to the original // task runner. In this case the callback is not destroyed, and the // weak reference allows SerialWorker instance to be deleted. @@ -95,7 +124,16 @@ void SerialWorker::OnFollowupWorkFinished(std::unique_ptr work_item) { return; case State::kWorking: state_ = State::kIdle; - OnWorkFinished(std::move(work_item)); + if (OnWorkFinished(std::move(work_item)) || + backoff_entry_.failure_count() >= max_number_of_retries_) { + backoff_entry_.Reset(); + } else { + backoff_entry_.InformOfRequest(/*succeeded=*/false); + + // Try again after a delay. + retry_timer_.Start(FROM_HERE, backoff_entry_.GetTimeUntilRelease(), + this, &SerialWorker::WorkNowInternal); + } return; case State::kPending: RerunWork(std::move(work_item)); @@ -119,6 +157,16 @@ void SerialWorker::RerunWork(std::unique_ptr work_item) { base::BindOnce(&SerialWorker::OnDoWorkFinished, AsWeakPtr())); } +const BackoffEntry& SerialWorker::GetBackoffEntryForTesting() const { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + return backoff_entry_; +} + +const base::OneShotTimer& SerialWorker::GetRetryTimerForTesting() const { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + return retry_timer_; +} + base::WeakPtr SerialWorker::AsWeakPtr() { return weak_factory_.GetWeakPtr(); } diff --git a/net/dns/serial_worker.h b/net/dns/serial_worker.h index 57fad932705ca0..b7076be9f8ed20 100644 --- a/net/dns/serial_worker.h +++ b/net/dns/serial_worker.h @@ -12,6 +12,8 @@ #include "base/memory/weak_ptr.h" #include "base/sequence_checker.h" #include "base/task/task_traits.h" +#include "base/timer/timer.h" +#include "net/base/backoff_entry.h" #include "net/base/net_export.h" namespace net { @@ -26,6 +28,10 @@ namespace net { // made to `OnWorkFinished()` the same `WorkItem` will be passed back to the // `ThreadPool`, and `DoWork()` will be called once more. // +// If |OnWorkFinished| returns a failure and |max_number_of_retries| +// is non-zero, retries will be scheduled according to the |backoff_policy|. +// A default backoff policy is used if one is not provided. +// // This behavior is designed for updating a result after some trigger, for // example reading a file once FilePathWatcher indicates it changed. // @@ -56,7 +62,9 @@ class NET_EXPORT_PRIVATE SerialWorker { virtual void FollowupWork(base::OnceClosure closure); }; - SerialWorker(); + explicit SerialWorker( + int max_number_of_retries = 0, + const net::BackoffEntry::Policy* backoff_policy = nullptr); SerialWorker(const SerialWorker&) = delete; SerialWorker& operator=(const SerialWorker&) = delete; @@ -70,6 +78,10 @@ class NET_EXPORT_PRIVATE SerialWorker { bool IsCancelled() const { return state_ == State::kCancelled; } + // Allows tests to inspect the current backoff/retry state. + const BackoffEntry& GetBackoffEntryForTesting() const; + const base::OneShotTimer& GetRetryTimerForTesting() const; + protected: // protected to allow sub-classing, but prevent deleting virtual ~SerialWorker(); @@ -78,7 +90,8 @@ class NET_EXPORT_PRIVATE SerialWorker { virtual std::unique_ptr CreateWorkItem() = 0; // Executed on origin thread after `WorkItem` completes. - virtual void OnWorkFinished(std::unique_ptr work_item) = 0; + // Must return true on success. + virtual bool OnWorkFinished(std::unique_ptr work_item) = 0; base::WeakPtr AsWeakPtr(); @@ -94,6 +107,8 @@ class NET_EXPORT_PRIVATE SerialWorker { kPending, // |WorkNow| while WORKING, must re-do work }; + void WorkNowInternal(); + // Called on the origin thread after `WorkItem::DoWork()` completes. void OnDoWorkFinished(std::unique_ptr work_item); @@ -104,6 +119,11 @@ class NET_EXPORT_PRIVATE SerialWorker { State state_; + // Max retries and backoff entry to control timing. + const int max_number_of_retries_; + BackoffEntry backoff_entry_; + base::OneShotTimer retry_timer_; + base::WeakPtrFactory weak_factory_{this}; }; diff --git a/net/dns/serial_worker_unittest.cc b/net/dns/serial_worker_unittest.cc index ea9d436588f3ec..e04b0f1428fc14 100644 --- a/net/dns/serial_worker_unittest.cc +++ b/net/dns/serial_worker_unittest.cc @@ -9,6 +9,7 @@ #include "base/bind.h" #include "base/callback.h" +#include "base/check.h" #include "base/location.h" #include "base/memory/raw_ptr.h" #include "base/run_loop.h" @@ -16,14 +17,35 @@ #include "base/synchronization/waitable_event.h" #include "base/task/current_thread.h" #include "base/task/single_thread_task_runner.h" +#include "base/test/simple_test_tick_clock.h" #include "base/threading/thread_restrictions.h" #include "base/threading/thread_task_runner_handle.h" +#include "base/time/time.h" +#include "base/timer/timer.h" +#include "net/base/backoff_entry.h" #include "net/test/test_with_task_environment.h" #include "testing/gtest/include/gtest/gtest.h" namespace net { namespace { +constexpr base::TimeDelta kBackoffInitialDelay = base::Milliseconds(100); +constexpr int kBackoffMultiplyFactor = 2; +constexpr int kMaxRetries = 3; + +static const BackoffEntry::Policy kTestBackoffPolicy = { + 0, // Number of initial errors to ignore without backoff. + static_cast( + kBackoffInitialDelay + .InMilliseconds()), // Initial delay for backoff in ms. + kBackoffMultiplyFactor, // Factor to multiply for exponential backoff. + 0, // Fuzzing percentage. + static_cast( + base::Seconds(1).InMilliseconds()), // Maximum time to delay requests + // in ms: 1 second. + -1, // Don't discard entry. + false // Don't use initial delay unless the last was an error. +}; class SerialWorkerTest : public TestWithTaskEnvironment { public: @@ -48,17 +70,20 @@ class SerialWorkerTest : public TestWithTaskEnvironment { raw_ptr test_; }; - explicit TestSerialWorker(SerialWorkerTest* t) : test_(t) {} + explicit TestSerialWorker(SerialWorkerTest* t) + : SerialWorker(/*max_number_of_retries=*/kMaxRetries, + &kTestBackoffPolicy), + test_(t) {} ~TestSerialWorker() override = default; std::unique_ptr CreateWorkItem() override { return std::make_unique(test_); } - void OnWorkFinished( + bool OnWorkFinished( std::unique_ptr work_item) override { - ASSERT_TRUE(test_); - test_->OnWorkFinished(); + CHECK(test_); + return test_->OnWorkFinished(); } private: @@ -76,6 +101,7 @@ class SerialWorkerTest : public TestWithTaskEnvironment { EXPECT_FALSE(work_running_) << "`DoWork()` is not called serially!"; work_running_ = true; } + num_work_calls_observed_++; BreakNow("OnWork"); { base::ScopedAllowBaseSyncPrimitivesForTesting @@ -102,11 +128,12 @@ class SerialWorkerTest : public TestWithTaskEnvironment { CompleteFollowup(); } - void OnWorkFinished() { + bool OnWorkFinished() { EXPECT_TRUE(task_runner_->BelongsToCurrentThread()); EXPECT_EQ(output_value_, input_value_); ++work_finished_calls_; BreakNow("OnWorkFinished"); + return on_work_finished_should_report_success_; } protected: @@ -136,8 +163,8 @@ class SerialWorkerTest : public TestWithTaskEnvironment { } SerialWorkerTest() - : input_value_(0), - output_value_(-1), + : TestWithTaskEnvironment( + base::test::TaskEnvironment::TimeSource::MOCK_TIME), work_allowed_(base::WaitableEvent::ResetPolicy::AUTOMATIC, base::WaitableEvent::InitialState::NOT_SIGNALED), work_called_(base::WaitableEvent::ResetPolicy::AUTOMATIC, @@ -173,9 +200,12 @@ class SerialWorkerTest : public TestWithTaskEnvironment { } // Input value read on WorkerPool. - int input_value_; + int input_value_ = 0; // Output value written on WorkerPool. - int output_value_; + int output_value_ = -1; + // The number of times we saw an OnWork call. + int num_work_calls_observed_ = 0; + bool on_work_finished_should_report_success_ = true; // read is called on WorkerPool so we need to synchronize with it. base::WaitableEvent work_allowed_; @@ -332,7 +362,7 @@ TEST_F(SerialWorkerTest, CancelDuringWork) { worker_->Cancel(); UnblockWork(); - base::RunLoop().RunUntilIdle(); + RunUntilIdle(); EXPECT_EQ(breakpoint_, "OnWork"); EXPECT_EQ(work_finished_calls_, 0); @@ -352,7 +382,7 @@ TEST_F(SerialWorkerTest, CancelDuringFollowup) { worker_->Cancel(); CompleteFollowup(); - base::RunLoop().RunUntilIdle(); + RunUntilIdle(); EXPECT_EQ(breakpoint_, "OnFollowup"); EXPECT_EQ(work_finished_calls_, 0); @@ -369,7 +399,7 @@ TEST_F(SerialWorkerTest, DeleteDuringWork) { worker_.reset(); UnblockWork(); - base::RunLoop().RunUntilIdle(); + RunUntilIdle(); EXPECT_EQ(breakpoint_, "OnWork"); EXPECT_EQ(work_finished_calls_, 0); @@ -389,7 +419,7 @@ TEST_F(SerialWorkerTest, DeleteDuringFollowup) { worker_.reset(); CompleteFollowup(); - base::RunLoop().RunUntilIdle(); + RunUntilIdle(); EXPECT_EQ(breakpoint_, "OnFollowup"); EXPECT_EQ(work_finished_calls_, 0); @@ -398,6 +428,120 @@ TEST_F(SerialWorkerTest, DeleteDuringFollowup) { EXPECT_TRUE(base::CurrentThread::Get()->IsIdleForTesting()); } +TEST_F(SerialWorkerTest, RetryAndThenSucceed) { + ASSERT_EQ(0, worker_->GetBackoffEntryForTesting().failure_count()); + + // Induce a failure. + on_work_finished_should_report_success_ = false; + ++input_value_; + worker_->WorkNow(); + RunUntilBreak("OnWork"); + UnblockWork(); + RunUntilBreak("OnFollowup"); + RunUntilBreak("OnWorkFinished"); + + // Confirm it failed and that a retry was scheduled. + ASSERT_EQ(1, worker_->GetBackoffEntryForTesting().failure_count()); + EXPECT_EQ(kBackoffInitialDelay, + worker_->GetBackoffEntryForTesting().GetTimeUntilRelease()); + + // Make the subsequent attempt succeed. + on_work_finished_should_report_success_ = true; + + RunUntilBreak("OnWork"); + UnblockWork(); + RunUntilBreak("OnFollowup"); + RunUntilBreak("OnWorkFinished"); + ASSERT_EQ(0, worker_->GetBackoffEntryForTesting().failure_count()); + + EXPECT_EQ(2, num_work_calls_observed_); + + // No more tasks should remain. + EXPECT_TRUE(base::CurrentThread::Get()->IsIdleForTesting()); +} + +TEST_F(SerialWorkerTest, ExternalWorkRequestResetsRetryState) { + ASSERT_EQ(0, worker_->GetBackoffEntryForTesting().failure_count()); + + // Induce a failure. + on_work_finished_should_report_success_ = false; + ++input_value_; + worker_->WorkNow(); + RunUntilBreak("OnWork"); + UnblockWork(); + RunUntilBreak("OnFollowup"); + RunUntilBreak("OnWorkFinished"); + + // Confirm it failed and that a retry was scheduled. + ASSERT_EQ(1, worker_->GetBackoffEntryForTesting().failure_count()); + EXPECT_TRUE(worker_->GetRetryTimerForTesting().IsRunning()); + EXPECT_EQ(kBackoffInitialDelay, + worker_->GetBackoffEntryForTesting().GetTimeUntilRelease()); + on_work_finished_should_report_success_ = true; + + // The retry state should be reset before we see OnWorkFinished. + worker_->WorkNow(); + ASSERT_EQ(0, worker_->GetBackoffEntryForTesting().failure_count()); + EXPECT_FALSE(worker_->GetRetryTimerForTesting().IsRunning()); + EXPECT_EQ(base::TimeDelta(), + worker_->GetBackoffEntryForTesting().GetTimeUntilRelease()); + RunUntilBreak("OnWork"); + UnblockWork(); + RunUntilBreak("OnFollowup"); + RunUntilBreak("OnWorkFinished"); + + // No more tasks should remain. + EXPECT_TRUE(base::CurrentThread::Get()->IsIdleForTesting()); +} + +TEST_F(SerialWorkerTest, MultipleFailureExponentialBackoff) { + ASSERT_EQ(0, worker_->GetBackoffEntryForTesting().failure_count()); + + // Induce a failure. + on_work_finished_should_report_success_ = false; + ++input_value_; + worker_->WorkNow(); + RunUntilBreak("OnWork"); + UnblockWork(); + RunUntilBreak("OnFollowup"); + RunUntilBreak("OnWorkFinished"); + + for (int retry_attempt_count = 1; retry_attempt_count <= kMaxRetries; + retry_attempt_count++) { + // Confirm it failed and that a retry was scheduled. + ASSERT_EQ(retry_attempt_count, + worker_->GetBackoffEntryForTesting().failure_count()); + EXPECT_TRUE(worker_->GetRetryTimerForTesting().IsRunning()); + base::TimeDelta expected_backoff_delay; + if (retry_attempt_count == 1) { + expected_backoff_delay = kBackoffInitialDelay; + } else { + expected_backoff_delay = kBackoffInitialDelay * kBackoffMultiplyFactor * + (retry_attempt_count - 1); + } + EXPECT_EQ(expected_backoff_delay, + worker_->GetBackoffEntryForTesting().GetTimeUntilRelease()) + << "retry_attempt_count=" << retry_attempt_count; + + // |on_work_finished_should_report_success_| is still false, so the retry + // will fail too + RunUntilBreak("OnWork"); + UnblockWork(); + RunUntilBreak("OnFollowup"); + RunUntilBreak("OnWorkFinished"); + } + + // The last retry attempt resets the retry state. + ASSERT_EQ(0, worker_->GetBackoffEntryForTesting().failure_count()); + EXPECT_FALSE(worker_->GetRetryTimerForTesting().IsRunning()); + EXPECT_EQ(base::TimeDelta(), + worker_->GetBackoffEntryForTesting().GetTimeUntilRelease()); + on_work_finished_should_report_success_ = true; + + // No more tasks should remain. + EXPECT_TRUE(base::CurrentThread::Get()->IsIdleForTesting()); +} + } // namespace } // namespace net