Commit f975321f authored by Eric Orth's avatar Eric Orth Committed by Chromium LUCI CQ

Factor out DnsConfigService::Watcher logic

Similar logic is currently duplicated between DnsConfigServiceWin and
DnsConfigServicePosix, and there would be even more duplication if
DCSPosix is further split by platforms (as I intend to do in a
subsequent CL).

Some logic for handling the detected-change notifications also moved
from the (now-shared) Watcher to DnsConfigService, as the logic wasn't
really part of "watching" but more handling.

Bug: 1157492
Change-Id: Ie813e5d9085cb4a109f7cad9b28d351562352b2e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2616421Reviewed-by: default avatarMatt Menke <mmenke@chromium.org>
Commit-Queue: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842578}
parent 7e1a2003
......@@ -6,8 +6,14 @@
#include <string>
#include "base/bind.h"
#include "base/check_op.h"
#include "base/location.h"
#include "base/logging.h"
#include "base/notreached.h"
#include "base/optional.h"
#include "base/threading/sequenced_task_runner_handle.h"
#include "base/time/time.h"
namespace net {
......@@ -15,12 +21,14 @@ namespace net {
const base::TimeDelta DnsConfigService::kInvalidationTimeout =
base::TimeDelta::FromMilliseconds(150);
DnsConfigService::DnsConfigService()
DnsConfigService::DnsConfigService(
base::Optional<base::TimeDelta> config_change_delay)
: watch_failed_(false),
have_config_(false),
have_hosts_(false),
need_update_(false),
last_sent_empty_(true) {
last_sent_empty_(true),
config_change_delay_(config_change_delay) {
DETACH_FROM_SEQUENCE(sequence_checker_);
}
......@@ -33,7 +41,8 @@ void DnsConfigService::ReadConfig(const CallbackType& callback) {
DCHECK(!callback.is_null());
DCHECK(callback_.is_null());
callback_ = callback;
ReadNow();
ReadConfigNow();
ReadHostsNow();
}
void DnsConfigService::WatchConfig(const CallbackType& callback) {
......@@ -42,7 +51,8 @@ void DnsConfigService::WatchConfig(const CallbackType& callback) {
DCHECK(callback_.is_null());
callback_ = callback;
watch_failed_ = !StartWatching();
ReadNow();
ReadConfigNow();
ReadHostsNow();
}
void DnsConfigService::RefreshConfig() {
......@@ -50,6 +60,27 @@ void DnsConfigService::RefreshConfig() {
NOTREACHED();
}
DnsConfigService::Watcher::Watcher(DnsConfigService* service)
: service_(service) {}
DnsConfigService::Watcher::~Watcher() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
}
void DnsConfigService::Watcher::OnConfigChanged(bool succeeded) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
service_->OnConfigChanged(succeeded);
}
void DnsConfigService::Watcher::OnHostsChanged(bool succeeded) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
service_->OnHostsChanged(succeeded);
}
void DnsConfigService::Watcher::CheckOnCorrectSequence() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
}
void DnsConfigService::InvalidateConfig() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!have_config_)
......@@ -135,4 +166,37 @@ void DnsConfigService::OnCompleteConfig() {
}
}
void DnsConfigService::OnConfigChanged(bool succeeded) {
if (config_change_delay_) {
// Ignore transient flutter of config source by delaying the signal a bit.
base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
FROM_HERE,
base::BindOnce(&DnsConfigService::OnConfigChangedDelayed,
weak_factory_.GetWeakPtr(), succeeded),
config_change_delay_.value());
} else {
OnConfigChangedDelayed(succeeded);
}
}
void DnsConfigService::OnHostsChanged(bool succeeded) {
InvalidateHosts();
if (succeeded) {
ReadHostsNow();
} else {
LOG(ERROR) << "DNS hosts watch failed.";
watch_failed_ = true;
}
}
void DnsConfigService::OnConfigChangedDelayed(bool succeeded) {
InvalidateConfig();
if (succeeded) {
ReadConfigNow();
} else {
LOG(ERROR) << "DNS config watch failed.";
watch_failed_ = true;
}
}
} // namespace net
......@@ -9,6 +9,8 @@
#include <memory>
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "base/optional.h"
#include "base/sequence_checker.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
......@@ -38,7 +40,13 @@ class NET_EXPORT_PRIVATE DnsConfigService {
// reading system DNS settings is not supported on the current platform.
static std::unique_ptr<DnsConfigService> CreateSystemService();
DnsConfigService();
// On detecting config change, will post and wait `config_change_delay` before
// triggering refreshes. Will trigger refreshes synchronously on nullopt.
// Useful for platforms where multiple changes may be made and detected before
// the config is stabilized and ready to be read.
explicit DnsConfigService(
base::Optional<base::TimeDelta> config_change_delay =
base::TimeDelta::FromMilliseconds(50));
virtual ~DnsConfigService();
// Attempts to read the configuration. Will run |callback| when succeeded.
......@@ -56,9 +64,47 @@ class NET_EXPORT_PRIVATE DnsConfigService {
// network-stack-external notifications of DNS config changes.
virtual void RefreshConfig();
void set_watch_failed_for_testing(bool watch_failed) {
watch_failed_ = watch_failed;
}
protected:
// Watcher to observe for changes to DNS config or HOSTS (via overriding
// `Watch()` with platform specifics) and trigger necessary refreshes on
// changes.
class Watcher {
public:
// `service` is expected to own the created Watcher and thus stay valid for
// the lifetime of the created Watcher.
explicit Watcher(DnsConfigService* service);
virtual ~Watcher();
Watcher(const Watcher&) = delete;
Watcher& operator=(const Watcher&) = delete;
virtual bool Watch() = 0;
protected:
// Hooks for detected changes. `succeeded` false to indicate that there was
// an error watching for the change.
void OnConfigChanged(bool succeeded);
void OnHostsChanged(bool succeeded);
void CheckOnCorrectSequence();
private:
void OnConfigChangedDelayed(bool success);
// Back pointer. `this` is expected to be owned by `service_`, making this
// raw pointer safe.
DnsConfigService* const service_;
SEQUENCE_CHECKER(sequence_checker_);
};
// Immediately attempts to read the current configuration.
virtual void ReadNow() = 0;
virtual void ReadConfigNow() = 0;
virtual void ReadHostsNow() = 0;
// Registers system watchers. Returns true iff succeeds.
virtual bool StartWatching() = 0;
......@@ -72,8 +118,6 @@ class NET_EXPORT_PRIVATE DnsConfigService {
// Called with new hosts. Rest of the config is assumed unchanged.
void OnHostsRead(const DnsHosts& hosts);
void set_watch_failed(bool value) { watch_failed_ = value; }
SEQUENCE_CHECKER(sequence_checker_);
private:
......@@ -83,6 +127,12 @@ class NET_EXPORT_PRIVATE DnsConfigService {
// Called when the config becomes complete. Stops the timer.
void OnCompleteConfig();
// Hooks for Watcher change notifications. `succeeded` false to indicate that
// there was an error watching for the change.
void OnConfigChanged(bool succeeded);
void OnHostsChanged(bool succeeded);
void OnConfigChangedDelayed(bool succeeded);
CallbackType callback_;
DnsConfig dns_config_;
......@@ -99,9 +149,13 @@ class NET_EXPORT_PRIVATE DnsConfigService {
// Set when |timer_| expires.
bool last_sent_empty_;
const base::Optional<base::TimeDelta> config_change_delay_;
// Started in Invalidate*, cleared in On*Read.
base::OneShotTimer timer_;
base::WeakPtrFactory<DnsConfigService> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(DnsConfigService);
};
......
......@@ -15,7 +15,11 @@ namespace internal {
DnsConfigServiceFuchsia::DnsConfigServiceFuchsia() = default;
DnsConfigServiceFuchsia::~DnsConfigServiceFuchsia() = default;
void DnsConfigServiceFuchsia::ReadNow() {
void DnsConfigServiceFuchsia::ReadConfigNow() {
// TODO(crbug.com/950717): Implement this method.
}
void DnsConfigServiceFuchsia::ReadHostsNow() {
// TODO(crbug.com/950717): Implement this method.
}
......
......@@ -21,7 +21,8 @@ class NET_EXPORT_PRIVATE DnsConfigServiceFuchsia : public DnsConfigService {
protected:
// DnsConfigService overrides.
void ReadNow() override;
void ReadConfigNow() override;
void ReadHostsNow() override;
bool StartWatching() override;
private:
......
......@@ -15,9 +15,9 @@
#include "base/lazy_instance.h"
#include "base/location.h"
#include "base/logging.h"
#include "base/sequence_checker.h"
#include "base/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 "build/build_config.h"
#include "net/base/ip_address.h"
......@@ -224,12 +224,15 @@ bool ReadDnsConfig(DnsConfig* dns_config) {
} // namespace
class DnsConfigServicePosix::Watcher {
class DnsConfigServicePosix::Watcher : public DnsConfigService::Watcher {
public:
explicit Watcher(DnsConfigServicePosix* service) : service_(service) {}
~Watcher() = default;
explicit Watcher(DnsConfigServicePosix* service)
: DnsConfigService::Watcher(service) {}
~Watcher() override = default;
bool Watch() override {
CheckOnCorrectSequence();
bool Watch() {
bool success = true;
if (!config_watcher_.Watch(base::BindRepeating(&Watcher::OnConfigChanged,
base::Unretained(this)))) {
......@@ -239,10 +242,11 @@ class DnsConfigServicePosix::Watcher {
// Hosts file should never change on Android or iOS (and watching it on Android
// is problematic; see http://crbug.com/600442), so don't watch it there.
#if !defined(OS_ANDROID) && !defined(OS_IOS)
if (!hosts_watcher_.Watch(base::FilePath(service_->file_path_hosts_),
base::FilePathWatcher::Type::kNonRecursive,
base::BindRepeating(&Watcher::OnHostsChanged,
base::Unretained(this)))) {
if (!hosts_watcher_.Watch(
base::FilePath(kFilePathHosts),
base::FilePathWatcher::Type::kNonRecursive,
base::BindRepeating(&Watcher::OnHostsFilePathWatcherChange,
base::Unretained(this)))) {
LOG(ERROR) << "DNS hosts watch failed to start.";
success = false;
}
......@@ -251,32 +255,17 @@ class DnsConfigServicePosix::Watcher {
}
private:
void OnConfigChanged(bool succeeded) {
// Ignore transient flutter of resolv.conf by delaying the signal a bit.
const base::TimeDelta kDelay = base::TimeDelta::FromMilliseconds(50);
base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
FROM_HERE,
base::BindOnce(&Watcher::OnConfigChangedDelayed,
weak_factory_.GetWeakPtr(), succeeded),
kDelay);
}
void OnConfigChangedDelayed(bool succeeded) {
service_->OnConfigChanged(succeeded);
}
void OnHostsChanged(const base::FilePath& path, bool error) {
service_->OnHostsChanged(!error);
#if !defined(OS_ANDROID) && !defined(OS_IOS)
void OnHostsFilePathWatcherChange(const base::FilePath& path, bool error) {
OnHostsChanged(!error);
}
#endif // !defined(OS_ANDROID) && !defined(OS_IOS)
DnsConfigServicePosix* const service_;
DnsConfigWatcher config_watcher_;
#if !defined(OS_ANDROID) && !defined(OS_IOS)
base::FilePathWatcher hosts_watcher_;
#endif // !defined(OS_ANDROID) && !defined(OS_IOS)
base::WeakPtrFactory<Watcher> weak_factory_{this};
DISALLOW_COPY_AND_ASSIGN(Watcher);
};
......@@ -321,9 +310,7 @@ class DnsConfigServicePosix::ConfigReader : public SerialWorker {
class DnsConfigServicePosix::HostsReader : public SerialWorker {
public:
explicit HostsReader(DnsConfigServicePosix* service)
: service_(service),
file_path_hosts_(service->file_path_hosts_),
success_(false) {
: service_(service), success_(false) {
// Allow execution on another thread; nothing thread-specific about
// constructor.
DETACH_FROM_SEQUENCE(sequence_checker_);
......@@ -335,7 +322,7 @@ class DnsConfigServicePosix::HostsReader : public SerialWorker {
void DoWork() override {
base::ScopedBlockingCall scoped_blocking_call(
FROM_HERE, base::BlockingType::MAY_BLOCK);
success_ = ParseHostsFile(file_path_hosts_, &hosts_);
success_ = ParseHostsFile(base::FilePath(kFilePathHosts), &hosts_);
}
void OnWorkFinished() override {
......@@ -350,8 +337,6 @@ class DnsConfigServicePosix::HostsReader : public SerialWorker {
// DoWork(), since service may be destroyed while SerialWorker is running
// on worker thread.
DnsConfigServicePosix* const service_;
// Hosts file path to parse.
const base::FilePath file_path_hosts_;
// Written in DoWork, read in OnWorkFinished, no locking necessary.
DnsHosts hosts_;
bool success_;
......@@ -359,8 +344,7 @@ class DnsConfigServicePosix::HostsReader : public SerialWorker {
DISALLOW_COPY_AND_ASSIGN(HostsReader);
};
DnsConfigServicePosix::DnsConfigServicePosix()
: file_path_hosts_(kFilePathHosts) {
DnsConfigServicePosix::DnsConfigServicePosix() {
// Allow constructing on one thread and living on another.
DETACH_FROM_SEQUENCE(sequence_checker_);
}
......@@ -373,11 +357,15 @@ DnsConfigServicePosix::~DnsConfigServicePosix() {
void DnsConfigServicePosix::RefreshConfig() {
InvalidateConfig();
InvalidateHosts();
ReadNow();
ReadConfigNow();
ReadHostsNow();
}
void DnsConfigServicePosix::ReadNow() {
void DnsConfigServicePosix::ReadConfigNow() {
config_reader_->WorkNow();
}
void DnsConfigServicePosix::ReadHostsNow() {
hosts_reader_->WorkNow();
}
......@@ -388,26 +376,6 @@ bool DnsConfigServicePosix::StartWatching() {
return watcher_->Watch();
}
void DnsConfigServicePosix::OnConfigChanged(bool succeeded) {
InvalidateConfig();
if (succeeded) {
config_reader_->WorkNow();
} else {
LOG(ERROR) << "DNS config watch failed.";
set_watch_failed(true);
}
}
void DnsConfigServicePosix::OnHostsChanged(bool succeeded) {
InvalidateHosts();
if (succeeded) {
hosts_reader_->WorkNow();
} else {
LOG(ERROR) << "DNS hosts watch failed.";
set_watch_failed(true);
}
}
void DnsConfigServicePosix::CreateReaders() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(!config_reader_);
......
......@@ -41,7 +41,8 @@ class NET_EXPORT_PRIVATE DnsConfigServicePosix : public DnsConfigService {
protected:
// DnsConfigService:
void ReadNow() override;
void ReadConfigNow() override;
void ReadHostsNow() override;
bool StartWatching() override;
// Create |config_reader_| and |hosts_reader_|.
......@@ -54,12 +55,7 @@ class NET_EXPORT_PRIVATE DnsConfigServicePosix : public DnsConfigService {
class ConfigReader;
class HostsReader;
void OnConfigChanged(bool succeeded);
void OnHostsChanged(bool succeeded);
std::unique_ptr<Watcher> watcher_;
// Allow a mock hosts file for testing purposes.
const base::FilePath::CharType* file_path_hosts_;
scoped_refptr<ConfigReader> config_reader_;
scoped_refptr<HostsReader> hosts_reader_;
......
......@@ -187,7 +187,7 @@ TEST_F(DnsConfigServiceTest, WatchFailure) {
EXPECT_TRUE(last_config_.Equals(config1));
// Simulate watch failure.
service_->set_watch_failed(true);
service_->set_watch_failed_for_testing(true);
service_->InvalidateConfig();
WaitForConfig(TestTimeouts::action_timeout());
EXPECT_FALSE(last_config_.Equals(config1));
......
......@@ -574,14 +574,18 @@ bool ConvertSettingsToDnsConfig(const DnsSystemSettings& settings,
// Watches registry and HOSTS file for changes. Must live on a sequence which
// allows IO.
class DnsConfigServiceWin::Watcher
: public NetworkChangeNotifier::IPAddressObserver {
: public NetworkChangeNotifier::IPAddressObserver,
public DnsConfigService::Watcher {
public:
explicit Watcher(DnsConfigServiceWin* service) : service_(service) {}
explicit Watcher(DnsConfigServiceWin* service)
: DnsConfigService::Watcher(service) {}
~Watcher() override { NetworkChangeNotifier::RemoveIPAddressObserver(this); }
bool Watch() {
RegistryWatcher::CallbackType callback = base::BindRepeating(
&DnsConfigServiceWin::OnConfigChanged, base::Unretained(service_));
bool Watch() override {
CheckOnCorrectSequence();
RegistryWatcher::CallbackType callback =
base::BindRepeating(&Watcher::OnConfigChanged, base::Unretained(this));
bool success = true;
......@@ -603,10 +607,10 @@ class DnsConfigServiceWin::Watcher
dnscache_watcher_.Watch(kDnscachePath, callback);
policy_watcher_.Watch(kPolicyPath, callback);
if (!hosts_watcher_.Watch(GetHostsPath(),
base::FilePathWatcher::Type::kNonRecursive,
base::BindRepeating(&Watcher::OnHostsChanged,
base::Unretained(this)))) {
if (!hosts_watcher_.Watch(
GetHostsPath(), base::FilePathWatcher::Type::kNonRecursive,
base::BindRepeating(&Watcher::OnHostsFilePathWatcherChange,
base::Unretained(this)))) {
LOG(ERROR) << "DNS hosts watch failed to start.";
success = false;
} else {
......@@ -617,20 +621,18 @@ class DnsConfigServiceWin::Watcher
}
private:
void OnHostsChanged(const base::FilePath& path, bool error) {
void OnHostsFilePathWatcherChange(const base::FilePath& path, bool error) {
if (error)
NetworkChangeNotifier::RemoveIPAddressObserver(this);
service_->OnHostsChanged(!error);
OnHostsChanged(!error);
}
// NetworkChangeNotifier::IPAddressObserver:
void OnIPAddressChanged() override {
// Need to update non-loopback IP of local host.
service_->OnHostsChanged(true);
OnHostsChanged(true);
}
DnsConfigServiceWin* service_;
RegistryWatcher tcpip_watcher_;
RegistryWatcher tcpip6_watcher_;
RegistryWatcher dnscache_watcher_;
......@@ -716,7 +718,8 @@ class DnsConfigServiceWin::HostsReader : public SerialWorker {
DISALLOW_COPY_AND_ASSIGN(HostsReader);
};
DnsConfigServiceWin::DnsConfigServiceWin() {
DnsConfigServiceWin::DnsConfigServiceWin()
: DnsConfigService(base::nullopt /* config_change_delay */) {
// Allow constructing on one sequence and living on another.
DETACH_FROM_SEQUENCE(sequence_checker_);
}
......@@ -728,8 +731,11 @@ DnsConfigServiceWin::~DnsConfigServiceWin() {
hosts_reader_->Cancel();
}
void DnsConfigServiceWin::ReadNow() {
void DnsConfigServiceWin::ReadConfigNow() {
config_reader_->WorkNow();
}
void DnsConfigServiceWin::ReadHostsNow() {
hosts_reader_->WorkNow();
}
......@@ -743,25 +749,6 @@ bool DnsConfigServiceWin::StartWatching() {
return watcher_->Watch();
}
void DnsConfigServiceWin::OnConfigChanged(bool succeeded) {
InvalidateConfig();
config_reader_->WorkNow();
if (!succeeded) {
LOG(ERROR) << "DNS config watch failed.";
set_watch_failed(true);
}
}
void DnsConfigServiceWin::OnHostsChanged(bool succeeded) {
InvalidateHosts();
if (succeeded) {
hosts_reader_->WorkNow();
} else {
LOG(ERROR) << "DNS hosts watch failed.";
set_watch_failed(true);
}
}
} // namespace internal
// static
......
......@@ -137,12 +137,10 @@ class NET_EXPORT_PRIVATE DnsConfigServiceWin : public DnsConfigService {
class HostsReader;
// DnsConfigService:
void ReadNow() override;
void ReadConfigNow() override;
void ReadHostsNow() override;
bool StartWatching() override;
void OnConfigChanged(bool succeeded);
void OnHostsChanged(bool succeeded);
std::unique_ptr<Watcher> watcher_;
scoped_refptr<ConfigReader> config_reader_;
scoped_refptr<HostsReader> hosts_reader_;
......
......@@ -6,7 +6,8 @@
namespace net {
TestDnsConfigService::TestDnsConfigService() = default;
TestDnsConfigService::TestDnsConfigService()
: DnsConfigService(base::nullopt /* config_change_delay */) {}
TestDnsConfigService::~TestDnsConfigService() = default;
......
......@@ -20,7 +20,8 @@ class TestDnsConfigService : public DnsConfigService {
TestDnsConfigService();
~TestDnsConfigService() override;
void ReadNow() override {}
void ReadConfigNow() override {}
void ReadHostsNow() override {}
bool StartWatching() override;
// Expose the protected methods to this test suite.
......@@ -36,10 +37,6 @@ class TestDnsConfigService : public DnsConfigService {
DnsConfigService::OnHostsRead(hosts);
}
void set_watch_failed(bool value) {
DnsConfigService::set_watch_failed(value);
}
void RefreshConfig() override;
void SetConfigForRefresh(DnsConfig config) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment