Skip to content

Commit

Permalink
Remove cancellation logic for HostResolver requests
Browse files Browse the repository at this point in the history 
Requests (ResolveHostRequest and ProbeRequest) are associated with a
Context, to which they hold a non-owning pointer.  This context must
exist when the Request is created, but the Request can outlive its
Context, and its state must change to avoid attempting to use the
expired pointer.

Additionally, some ResolveHostRequests are associated with a Job, and
both hold non-owning pointers to the other.  Either can outlive the
other, so either must be notified when the other is destroyed, especially
because they both have cleanup actions that are triggered.

Currently, ContextHostResolvers maintain a non-owning list of all Requests
for the context, in order to notify them when the context is destroyed.
Requests then cancel their Jobs if necessary.  HostResolverManager
also maintains a (non-owning) list of active contexts and an (owning)
map of Jobs, keyed by context.

This change removes the need for explicit cancellation of Requests.
Instead, context shutdown destroys all the relevant Jobs, which notify
their affected Requests to perform cleanup actions.  The remaining
Requests only hold a WeakPtr to the context, and have no cleanup actions
to take, so they do not need to be notified at all.  This reduces
complexity and enables the use of Jobs that do not have a Request.

As a side effect, calls to Request::Start() after the context is
destroyed now return ERR_CONTEXT_SHUT_DOWN instead of
ERR_NAME_NOT_RESOLVED.

Bug: 1200908
Change-Id: I43941aa981660b95a86842833fd9c94f56d1cdab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3293904
Reviewed-by: Eric Orth <ericorth@chromium.org>
Commit-Queue: Ben Schwartz <bemasc@chromium.org>
Cr-Commit-Position: refs/heads/main@{#949222}
  • Loading branch information
Benjamin M. Schwartz authored and Chromium LUCI CQ committed Dec 7, 2021
1 parent 3eb8c24 commit 4a56172
Show file tree
Hide file tree
Showing 9 changed files with 229 additions and 598 deletions.
289 changes: 17 additions & 272 deletions net/dns/context_host_resolver.cc
View file
Expand Up @@ -9,7 +9,6 @@
#include <vector>

#include "base/check_op.h"
#include "base/memory/raw_ptr.h"
#include "base/no_destructor.h"
#include "base/strings/string_piece.h"
#include "base/time/tick_clock.h"
Expand All @@ -30,236 +29,6 @@

namespace net {

// Wrapper of ResolveHostRequests that on destruction will remove itself from
// |ContextHostResolver::handed_out_requests_|.
class ContextHostResolver::WrappedRequest {
public:
WrappedRequest(ContextHostResolver* resolver, bool shutting_down)
: resolver_(resolver), shutting_down_(shutting_down) {
DCHECK_CALLED_ON_VALID_SEQUENCE(resolver_->sequence_checker_);
}

WrappedRequest(const WrappedRequest&) = delete;
WrappedRequest& operator=(const WrappedRequest&) = delete;

virtual ~WrappedRequest() { DetachFromResolver(); }

void Cancel() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
OnShutdown();
DetachFromResolver();
}

void OnShutdown() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

// Cannot destroy |inner_request_| because it is still allowed to call
// Get...Results() methods if the request was already complete.
if (inner_request())
inner_request()->Cancel();

shutting_down_ = true;

// Not clearing |resolver_| so that early shutdown can be differentiated in
// Start() from full cancellation on resolver destruction.
}

virtual HostResolverManager::CancellableRequest* inner_request() = 0;

ContextHostResolver* resolver() { return resolver_; }
bool shutting_down() { return shutting_down_; }

private:
void DetachFromResolver() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (resolver_) {
DCHECK_EQ(1u, resolver_->handed_out_requests_.count(this));
resolver_->handed_out_requests_.erase(this);
resolver_ = nullptr;
}
}

// Resolver is expected to call Cancel() on destruction, clearing the pointer
// before it becomes invalid.
raw_ptr<ContextHostResolver> resolver_;
bool shutting_down_ = false;

SEQUENCE_CHECKER(sequence_checker_);
};

class ContextHostResolver::WrappedResolveHostRequest
: public WrappedRequest,
public HostResolver::ResolveHostRequest {
public:
WrappedResolveHostRequest(
std::unique_ptr<HostResolverManager::CancellableResolveHostRequest>
request,
ContextHostResolver* resolver,
bool shutting_down)
: WrappedRequest(resolver, shutting_down),
inner_request_(std::move(request)) {}

WrappedResolveHostRequest(const WrappedResolveHostRequest&) = delete;
WrappedResolveHostRequest& operator=(const WrappedResolveHostRequest&) =
delete;

int Start(CompletionOnceCallback callback) override {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

if (!resolver()) {
// Parent resolver has been destroyed. HostResolver generally disallows
// calling Start() in this case, but this implementation returns
// ERR_FAILED to allow testing the case.
inner_request_ = nullptr;
resolve_error_info_ = ResolveErrorInfo(ERR_FAILED);
return ERR_NAME_NOT_RESOLVED;
}

if (shutting_down()) {
// Shutting down but the resolver is not yet destroyed.
inner_request_ = nullptr;
resolve_error_info_ = ResolveErrorInfo(ERR_CONTEXT_SHUT_DOWN);
return ERR_NAME_NOT_RESOLVED;
}

DCHECK(inner_request_);
return inner_request_->Start(std::move(callback));
}

const absl::optional<AddressList>& GetAddressResults() const override {
if (!inner_request_) {
static base::NoDestructor<absl::optional<AddressList>> nullopt_result;
return *nullopt_result;
}

return inner_request_->GetAddressResults();
}

absl::optional<std::vector<HostResolverEndpointResult>> GetEndpointResults()
const override {
if (!inner_request_)
return absl::nullopt;

return inner_request_->GetEndpointResults();
}

const absl::optional<std::vector<std::string>>& GetTextResults()
const override {
if (!inner_request_) {
static const base::NoDestructor<absl::optional<std::vector<std::string>>>
nullopt_result;
return *nullopt_result;
}

return inner_request_->GetTextResults();
}

const absl::optional<std::vector<HostPortPair>>& GetHostnameResults()
const override {
if (!inner_request_) {
static const base::NoDestructor<absl::optional<std::vector<HostPortPair>>>
nullopt_result;
return *nullopt_result;
}

return inner_request_->GetHostnameResults();
}

const absl::optional<std::vector<std::string>>& GetDnsAliasResults()
const override {
if (!inner_request_) {
static const base::NoDestructor<absl::optional<std::vector<std::string>>>
nullopt_result;
return *nullopt_result;
}

return inner_request_->GetDnsAliasResults();
}

net::ResolveErrorInfo GetResolveErrorInfo() const override {
if (!inner_request_) {
return resolve_error_info_;
}
return inner_request_->GetResolveErrorInfo();
}

const absl::optional<HostCache::EntryStaleness>& GetStaleInfo()
const override {
if (!inner_request_) {
static const absl::optional<HostCache::EntryStaleness> nullopt_result;
return nullopt_result;
}

return inner_request_->GetStaleInfo();
}

void ChangeRequestPriority(RequestPriority priority) override {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(inner_request_);

inner_request_->ChangeRequestPriority(priority);
}

HostResolverManager::CancellableRequest* inner_request() override {
return inner_request_.get();
}

private:
std::unique_ptr<HostResolverManager::CancellableResolveHostRequest>
inner_request_;

// Error info for a |inner_request_| that was destroyed before it started.
ResolveErrorInfo resolve_error_info_;

SEQUENCE_CHECKER(sequence_checker_);
};

class ContextHostResolver::WrappedProbeRequest
: public WrappedRequest,
public HostResolver::ProbeRequest {
public:
WrappedProbeRequest(
std::unique_ptr<HostResolverManager::CancellableProbeRequest>
inner_request,
ContextHostResolver* resolver,
bool shutting_down)
: WrappedRequest(resolver, shutting_down),
inner_request_(std::move(inner_request)) {}

WrappedProbeRequest(const WrappedProbeRequest&) = delete;
WrappedProbeRequest& operator=(const WrappedProbeRequest&) = delete;

int Start() override {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

if (!resolver()) {
// Parent resolver has been destroyed. HostResolver generally disallows
// calling Start() in this case, but this implementation returns
// ERR_FAILED to allow testing the case.
inner_request_ = nullptr;
return ERR_FAILED;
}

if (shutting_down()) {
// Shutting down but the resolver is not yet destroyed.
inner_request_ = nullptr;
return ERR_CONTEXT_SHUT_DOWN;
}

DCHECK(inner_request_);
return inner_request_->Start();
}

HostResolverManager::CancellableRequest* inner_request() override {
return inner_request_.get();
}

private:
std::unique_ptr<HostResolverManager::CancellableProbeRequest> inner_request_;

SEQUENCE_CHECKER(sequence_checker_);
};

ContextHostResolver::ContextHostResolver(
HostResolverManager* manager,
std::unique_ptr<ResolveContext> resolve_context)
Expand All @@ -278,24 +47,18 @@ ContextHostResolver::ContextHostResolver(
}

ContextHostResolver::~ContextHostResolver() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (owned_manager_)
DCHECK_EQ(owned_manager_.get(), manager_);

// No |resolve_context_| to deregister if OnShutdown() was already called.
if (resolve_context_)
manager_->DeregisterResolveContext(resolve_context_.get());

// Silently cancel all requests associated with this resolver.
while (!handed_out_requests_.empty())
(*handed_out_requests_.begin())->Cancel();
}

void ContextHostResolver::OnShutdown() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

for (auto* active_request : handed_out_requests_)
active_request->OnShutdown();

DCHECK(resolve_context_);
manager_->DeregisterResolveContext(resolve_context_.get());
resolve_context_.reset();
Expand All @@ -312,19 +75,13 @@ ContextHostResolver::CreateRequest(
absl::optional<ResolveHostParameters> optional_parameters) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

std::unique_ptr<HostResolverManager::CancellableResolveHostRequest>
inner_request;
if (!shutting_down_) {
inner_request = manager_->CreateRequest(
std::move(host), std::move(network_isolation_key),
std::move(source_net_log), std::move(optional_parameters),
resolve_context_.get(), resolve_context_->host_cache());
}

auto request = std::make_unique<WrappedResolveHostRequest>(
std::move(inner_request), this, shutting_down_);
handed_out_requests_.insert(request.get());
return request;
if (shutting_down_)
return HostResolver::CreateFailingRequest(ERR_CONTEXT_SHUT_DOWN);

return manager_->CreateRequest(
std::move(host), std::move(network_isolation_key),
std::move(source_net_log), std::move(optional_parameters),
resolve_context_.get(), resolve_context_->host_cache());
}

std::unique_ptr<HostResolver::ResolveHostRequest>
Expand All @@ -335,33 +92,22 @@ ContextHostResolver::CreateRequest(
const absl::optional<ResolveHostParameters>& optional_parameters) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

std::unique_ptr<HostResolverManager::CancellableResolveHostRequest>
inner_request;
if (!shutting_down_) {
inner_request = manager_->CreateRequest(
host, network_isolation_key, source_net_log, optional_parameters,
resolve_context_.get(), resolve_context_->host_cache());
}

auto request = std::make_unique<WrappedResolveHostRequest>(
std::move(inner_request), this, shutting_down_);
handed_out_requests_.insert(request.get());
return request;
if (shutting_down_)
return HostResolver::CreateFailingRequest(ERR_CONTEXT_SHUT_DOWN);

return manager_->CreateRequest(host, network_isolation_key, source_net_log,
optional_parameters, resolve_context_.get(),
resolve_context_->host_cache());
}

std::unique_ptr<HostResolver::ProbeRequest>
ContextHostResolver::CreateDohProbeRequest() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

std::unique_ptr<HostResolverManager::CancellableProbeRequest> inner_request;
if (!shutting_down_) {
inner_request = manager_->CreateDohProbeRequest(resolve_context_.get());
}
if (shutting_down_)
return HostResolver::CreateFailingProbeRequest(ERR_CONTEXT_SHUT_DOWN);

auto request = std::make_unique<WrappedProbeRequest>(std::move(inner_request),
this, shutting_down_);
handed_out_requests_.insert(request.get());
return request;
return manager_->CreateDohProbeRequest(resolve_context_.get());
}

std::unique_ptr<HostResolver::MdnsListener>
Expand All @@ -380,7 +126,6 @@ base::Value ContextHostResolver::GetDnsConfigAsValue() const {

void ContextHostResolver::SetRequestContext(
URLRequestContext* request_context) {
DCHECK(handed_out_requests_.empty());
DCHECK(!shutting_down_);
DCHECK(resolve_context_);
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
Expand Down
15 changes: 0 additions & 15 deletions net/dns/context_host_resolver.h
View file
Expand Up @@ -82,24 +82,9 @@ class NET_EXPORT ContextHostResolver : public HostResolver {
void SetProcParamsForTesting(const ProcTaskParams& proc_params);
void SetTickClockForTesting(const base::TickClock* tick_clock);

size_t GetNumActiveRequestsForTesting() const {
return handed_out_requests_.size();
}

private:
class WrappedRequest;
class WrappedResolveHostRequest;
class WrappedProbeRequest;

const raw_ptr<HostResolverManager> manager_;
std::unique_ptr<HostResolverManager> owned_manager_;

// Requests are expected to clear themselves from this set on destruction or
// cancellation. Requests in an early shutdown state (from
// HostResolver::OnShutdown()) are still in this set, so they can be notified
// on resolver destruction.
std::unordered_set<WrappedRequest*> handed_out_requests_;

std::unique_ptr<ResolveContext> resolve_context_;

// If true, the context is shutting down. Subsequent request Start() calls
Expand Down

0 comments on commit 4a56172

Please sign in to comment.