[net] HostResolverImpl + DnsTransaction + DnsConfigService = Asynchronous DNS ready for experiments.

net/base/host_resolver_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/host_resolver_impl.h"
 
 #if defined(OS_WIN)
 #include <Winsock2.h>
 #elif defined(OS_POSIX)
 #include <netdb.h>
 #endif
 
 #include <cmath>
 #include <utility>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/debug/debugger.h"
 #include "base/debug/stack_trace.h"
 #include "base/message_loop_proxy.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
+#include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
 #include "base/threading/worker_pool.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
+#include "net/base/address_family.h"
 #include "net/base/address_list.h"
 #include "net/base/address_list_net_log_param.h"
 #include "net/base/dns_reloader.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/host_resolver_proc.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/base/net_util.h"
+#include "net/dns/dns_config_service.h"
+#include "net/dns/dns_protocol.h"
+#include "net/dns/dns_response.h"
+#include "net/dns/dns_session.h"
+#include "net/dns/dns_transaction.h"
+#include "net/socket/client_socket_factory.h"
 
 #if defined(OS_WIN)
 #include "net/base/winsock_init.h"
 #endif
 
 namespace net {
 
 namespace {
 
 // Limit the size of hostnames that will be resolved to combat issues in
@@ skipping 257 matching lines @@
   request_net_log.AddEvent(NetLog::TYPE_CANCELLED, NULL);
   request_net_log.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_REQUEST, NULL);
   source_net_log.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL, NULL);
 }
 
 //-----------------------------------------------------------------------------
 
 // Keeps track of the highest priority.
 class PriorityTracker {
  public:
-  PriorityTracker()
-      : highest_priority_(IDLE), total_count_(0) {
+  PriorityTracker(RequestPriority priority)

[cbentzel, 2012/02/14 13:08:26]

explicit

+      : highest_priority_(priority), total_count_(0) {

[cbentzel, 2012/02/14 13:08:26]

This was a little tricky - I expected total_count_ and counts_[priority] to both
be 1. Once I looked at how the AddRequest was made common for the
new-job/existing-job case it became clear why this was needed, but may benefit
from a note.

     memset(counts_, 0, sizeof(counts_));
   }
 
   RequestPriority highest_priority() const {
     return highest_priority_;
   }
 
   size_t total_count() const {
     return total_count_;
   }
@@ skipping 22 matching lines @@
   }
 
  private:
   RequestPriority highest_priority_;
   size_t total_count_;
   size_t counts_[NUM_PRIORITIES];
 };
 
 //-----------------------------------------------------------------------------
 
+// Convenience wrapper for PrioritizedDispatcher::Handle so that we don't forget
+// to update it when cancelling or changing the priority.
+// TODO(szym): consider making PrioritizedDispatcher::Handle into this.
+class DispatcherHandle {
+ public:
+  DispatcherHandle() : dispatcher_(NULL) {}
+  DispatcherHandle(PrioritizedDispatcher* dispatcher,
+                   const PrioritizedDispatcher::Handle& handle)
+      : dispatcher_(dispatcher), handle_(handle) {
+    if (handle_.is_null())
+      dispatcher_ = NULL;
+  }
+
+  bool is_null() const {
+    return dispatcher_ == NULL;
+  }
+
+  void Reset() {
+    dispatcher_ = NULL;
+    handle_ = PrioritizedDispatcher::Handle();
+  }
+
+  void ChangePriority(RequestPriority priority) {
+    DCHECK(!is_null());
+    handle_ = dispatcher_->ChangePriority(handle_, priority);
+  }
+
+  void Cancel() {
+    DCHECK(!is_null());
+    dispatcher_->Cancel(handle_);
+    Reset();
+  }
+
+ private:
+  PrioritizedDispatcher* dispatcher_;
+  PrioritizedDispatcher::Handle handle_;
+};
+
+//-----------------------------------------------------------------------------
+
 HostResolver* CreateHostResolver(size_t max_concurrent_resolves,
                                  size_t max_retry_attempts,
-                                 bool use_cache,
+                                 HostCache* cache,
+                                 scoped_ptr<DnsConfigService> config_service,
                                  NetLog* net_log) {
   if (max_concurrent_resolves == HostResolver::kDefaultParallelism)
     max_concurrent_resolves = kDefaultMaxProcTasks;
 
   // TODO(szym): Add experiments with reserved slots for higher priority
   // requests.
+  // TODO(szym): Add tighter limits for proc_dispatcher_

[cbentzel, 2012/02/14 13:12:06]

Why do you need this TODO? In this case it will only be using the limits
specified for the regular dispatcher_, and these are the same limits as before.

Also - at one point I thought we had different queues for
speculative/non-speculative DNS queries. Should we do some reserved slots for
non-idle cases? 

[szym, 2012/02/14 15:56:24]

I need this TODO to remember that even if I start using proc_dispatcher_, it
will never queue a job unless the its limits are tighter than those of
dispatcher_. Right now proc_dispatcher_ is not used, and I needed to put some
limits in there. I'll change it to PrioritizedDispatcher::Limits(NUM_PRIORITIES,
0);
 
Even before the big refactor of HostResolverImpl, the is_speculative flag wasn't
used to determine the priority of the Job. There was the JobPool facility, but
with only one instance, there really wasn't any reservation in place.

I think it might be time for an experiment with tiered limits. We'd need to
check the call-sites to Resolve to see if request priority is properly set.

 
   PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, max_concurrent_resolves);
 
   HostResolverImpl* resolver = new HostResolverImpl(
-      use_cache ? HostCache::CreateDefaultCache() : NULL,
+      cache,
+      limits,
       limits,
       HostResolverImpl::ProcTaskParams(NULL, max_retry_attempts),
+      config_service.Pass(),
       net_log);
 
   return resolver;
 }
 
 }  // anonymous namespace
 
 //-----------------------------------------------------------------------------
 
 HostResolver* CreateSystemHostResolver(size_t max_concurrent_resolves,
                                        size_t max_retry_attempts,
                                        NetLog* net_log) {
   return CreateHostResolver(max_concurrent_resolves,
                             max_retry_attempts,
-                            true /* use_cache */,
+                            HostCache::CreateDefaultCache(),
+                            scoped_ptr<DnsConfigService>(NULL),
                             net_log);
 }
 
 HostResolver* CreateNonCachingSystemHostResolver(size_t max_concurrent_resolves,
                                                  size_t max_retry_attempts,
                                                  NetLog* net_log) {
   return CreateHostResolver(max_concurrent_resolves,
                             max_retry_attempts,
-                            false /* use_cache */,
+                            NULL,
+                            scoped_ptr<DnsConfigService>(NULL),
                             net_log);
 }
 
+HostResolver* CreateAsyncHostResolver(size_t max_concurrent_resolves,
+                                      size_t max_retry_attempts,
+                                      NetLog* net_log) {
+  scoped_ptr<DnsConfigService> config_service
+      = DnsConfigService::CreateSystemService();

[mmenke, 2012/02/14 18:46:00]

nit:  Put the equals sign on the previous line (Can't find a single instance
where we don't do that in net/, and only one in chrome/)

+  config_service->Watch();
+  return CreateHostResolver(max_concurrent_resolves,
+                            max_retry_attempts,
+                            HostCache::CreateDefaultCache(),
+                            config_service.Pass(),
+                            net_log);
+}
+
 //-----------------------------------------------------------------------------
 
 // Holds the data for a request that could not be completed synchronously.
 // It is owned by a Job. Canceled Requests are only marked as canceled rather
 // than removed from the Job's |requests_| list.
 class HostResolverImpl::Request {
  public:
   Request(const BoundNetLog& source_net_log,
           const BoundNetLog& request_net_log,
           const RequestInfo& info,
@@ skipping 83 matching lines @@
 // resolution (OnLookupComplete) is completed or not. If the original attempt
 // hasn't completed, then we start another attempt for host resolution. We take
 // the results from the first attempt that finishes and ignore the results from
 // all other attempts.
 //
 // TODO(szym): Move to separate source file for testing and mocking.
 //
 class HostResolverImpl::ProcTask
     : public base::RefCountedThreadSafe<HostResolverImpl::ProcTask> {
  public:
-  typedef base::Callback<void(int, int, const AddressList&)> Callback;
+  typedef base::Callback<void(int net_error,
+                              int os_error,
+                              const AddressList& addr_list)> Callback;
 
   ProcTask(const Key& key,
            const ProcTaskParams& params,
            const Callback& callback,
            const BoundNetLog& job_net_log)
       : key_(key),
         params_(params),
         callback_(callback),
         origin_loop_(base::MessageLoopProxy::current()),
         attempt_number_(0),
@@ skipping 365 matching lines @@
   AddressList results_;
 
   BoundNetLog net_log_;
 
   DISALLOW_COPY_AND_ASSIGN(ProcTask);
 };
 
 //-----------------------------------------------------------------------------
 
 // Represents a request to the worker pool for a "probe for IPv6 support" call.
+//
+// TODO(szym): This could also be replaced with PostTaskAndReply and Callbacks.
 class HostResolverImpl::IPv6ProbeJob
     : public base::RefCountedThreadSafe<HostResolverImpl::IPv6ProbeJob> {
  public:
   explicit IPv6ProbeJob(HostResolverImpl* resolver)
       : resolver_(resolver),
         origin_loop_(base::MessageLoopProxy::current()) {
     DCHECK(resolver);
   }
 
   void Start() {
@@ skipping 47 matching lines @@
   HostResolverImpl* resolver_;
 
   // Used to post ourselves onto the origin thread.
   scoped_refptr<base::MessageLoopProxy> origin_loop_;
 
   DISALLOW_COPY_AND_ASSIGN(IPv6ProbeJob);
 };
 
 //-----------------------------------------------------------------------------
 
+// Resolves the hostname using DnsTransaction.
+// TODO(szym): This could be moved to separate source file as well.
+class HostResolverImpl::DnsTask {
+ public:
+  typedef base::Callback<void(int net_error,
+                              const AddressList& addr_list,
+                              base::TimeDelta ttl)> Callback;
+
+  DnsTask(DnsTransactionFactory* factory,
+          const Key& key,
+          const Callback& callback,
+          const BoundNetLog& job_net_log)
+      : callback_(callback), net_log_(job_net_log) {
+    DCHECK(factory);
+    DCHECK(!callback.is_null());
+
+    // For now we treat ADDRESS_FAMILY_UNSPEC as if it was IPV4.
+    uint16 qtype = (key.address_family == ADDRESS_FAMILY_IPV6)
+                   ? dns_protocol::kTypeAAAA
+                   : dns_protocol::kTypeA;
+    // TODO(szym): Implement "happy eyeballs".
+    transaction_ = factory->CreateTransaction(
+        key.hostname,
+        qtype,
+        base::Bind(&DnsTask::OnTransactionComplete, base::Unretained(this)),
+        net_log_);
+    DCHECK(transaction_.get());
+  }
+
+  int Start() {
+    net_log_.BeginEvent(
+        NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK,
+        new NetLogSourceParameter("source_dependency",
+                                  transaction_->net_log().source()));
+    return transaction_->Start();
+  }
+
+  void OnTransactionComplete(DnsTransaction* transaction,
+                             int net_error,
+                             const DnsResponse* response) {
+    // TODO(szym): Record performance histograms.
+    if (net_error == OK) {
+      AddressList addr_list;
+      base::TimeDelta ttl;
+      if (response->ParseAddressList(&addr_list, &ttl)) {
+        net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK,
+                          new AddressListNetLogParam(addr_list));
+        callback_.Run(net_error, addr_list, ttl);
+        return;
+      }
+      net_error = ERR_DNS_MALFORMED_RESPONSE;
+    }
+    callback_.Run(net_error, AddressList(), base::TimeDelta());
+    net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HOST_RESOLVER_IMPL_DNS_TASK,
+                                      net_error);
+  }
+
+ private:
+  // The listener to the results of this DnsTask.
+  Callback callback_;
+
+  const BoundNetLog net_log_;
+
+  scoped_ptr<DnsTransaction> transaction_;
+};
+
+//-----------------------------------------------------------------------------
+
 // Aggregates all Requests for the same Key. Dispatched via PriorityDispatch.
 // Spawns ProcTask when started.

[cbentzel, 2012/02/14 13:08:26]

The comment "Spawns ProcTask when started" is no longer completely accurate.

 class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
  public:
   // Creates new job for |key| where |request_net_log| is bound to the
   // request that spawned it.
   Job(HostResolverImpl* resolver,
       const Key& key,
-      const BoundNetLog& request_net_log)
+      const BoundNetLog& request_net_log,
+      RequestPriority priority)
       : resolver_(resolver->AsWeakPtr()),
         key_(key),
+        priority_tracker_(priority),
         had_non_speculative_request_(false),
         net_log_(BoundNetLog::Make(request_net_log.net_log(),
                                    NetLog::SOURCE_HOST_RESOLVER_IMPL_JOB)),
         net_error_(ERR_IO_PENDING),
         os_error_(0) {
     request_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_CREATE_JOB, NULL);
 
     net_log_.BeginEvent(
         NetLog::TYPE_HOST_RESOLVER_IMPL_JOB,
         make_scoped_refptr(new JobCreationParameters(
             key_.hostname, request_net_log.source())));
   }
 
   virtual ~Job() {
     if (net_error_ == ERR_IO_PENDING) {
       if (is_running()) {
         DCHECK_EQ(ERR_IO_PENDING, net_error_);
         proc_task_->Cancel();

[cbentzel, 2012/02/14 13:08:26]

BUG: This needs to deal with the case where is_running() is true but it's in the
middle of dns_task_

Perhaps share code with Abort().

[szym, 2012/02/14 15:56:24]

Good catch. Needs: "if (proc_task_.get())". 

         proc_task_ = NULL;
         net_error_ = ERR_ABORTED;
       } else {
         net_log_.AddEvent(NetLog::TYPE_CANCELLED, NULL);
         net_error_ = OK;  // For NetLog.
       }
 
       for (RequestsList::const_iterator it = requests_.begin();
            it != requests_.end(); ++it) {
         Request* req = *it;
         if (req->was_canceled())
           continue;
         DCHECK_EQ(this, req->job());
         LogCancelRequest(req->source_net_log(), req->request_net_log(),
                          req->info());
       }
     }
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB,
                                       net_error_);
     STLDeleteElements(&requests_);
   }
 
-  HostResolverImpl* resolver() const {
-    return resolver_;
-  }
-
   RequestPriority priority() const {
     return priority_tracker_.highest_priority();
   }
 
   // Number of non-canceled requests in |requests_|.
   size_t num_active_requests() const {
     return priority_tracker_.total_count();
   }
 
   const Key& key() const {
     return key_;
   }
 
   int net_error() const {
     return net_error_;
   }
 
-  // Used by HostResolverImpl with |dispatcher_|.
-  const PrioritizedDispatcher::Handle& handle() const {
-    return handle_;
+  void AddToDispatcher(PrioritizedDispatcher* dispatcher) {
+    DCHECK(handle_.is_null());
+    handle_ = DispatcherHandle(dispatcher, dispatcher->Add(this, priority()));
   }
 
-  void set_handle(const PrioritizedDispatcher::Handle& handle) {
-    handle_ = handle;
+  bool IsWaitingInDispatch() const {
+    return !handle_.is_null();
   }
 
   // The Job will own |req| and destroy it in ~Job.

[cbentzel, 2012/02/14 13:08:26]

Nit: change this to take a scoped_ptr<Request> argument instead?

   void AddRequest(Request* req) {
     DCHECK_EQ(key_.hostname, req->info().hostname());
 
     req->set_job(this);
     requests_.push_back(req);
 
     priority_tracker_.Add(req->info().priority());
 
     req->request_net_log().AddEvent(
         NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_ATTACH,
         make_scoped_refptr(new NetLogSourceParameter(
             "source_dependency", net_log_.source())));
 
     net_log_.AddEvent(
         NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_REQUEST_ATTACH,
         make_scoped_refptr(new JobAttachParameters(
             req->request_net_log().source(), priority())));
 
     // TODO(szym): Check if this is still needed.
     if (!req->info().is_speculative()) {
       had_non_speculative_request_ = true;
       if (proc_task_)
         proc_task_->set_had_non_speculative_request();
     }
+
+    if (!handle_.is_null())
+      handle_.ChangePriority(priority());
   }
 
   void CancelRequest(Request* req) {
     DCHECK_EQ(key_.hostname, req->info().hostname());
     DCHECK(!req->was_canceled());
     // Don't remove it from |requests_| just mark it canceled.
     req->MarkAsCanceled();
     LogCancelRequest(req->source_net_log(), req->request_net_log(),
                      req->info());
     priority_tracker_.Remove(req->info().priority());
     net_log_.AddEvent(
         NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_REQUEST_DETACH,
         make_scoped_refptr(new JobAttachParameters(
             req->request_net_log().source(), priority())));
+
+    if (!handle_.is_null()) {
+      if (num_active_requests() > 0) {
+        handle_.ChangePriority(priority());
+      } else {
+        handle_.Cancel();
+      }
+    }
   }
 
   // Aborts and destroys the job, completes all requests as aborted.
   void Abort() {
     // Job should only be aborted if it's running.
     DCHECK(is_running());

[cbentzel, 2012/02/14 13:08:26]

Should this remove itself from the PrioritizedDispatcher if !handle_.is_null()

[szym, 2012/02/14 15:56:24]

If is_running() then handle_ should be null. (A running Job cannot be queued.) I
will add a DCHECK for that.

-    proc_task_->Cancel();
-    proc_task_ = NULL;
+    if (proc_task_.get()) {
+      proc_task_->Cancel();
+      proc_task_ = NULL;
+    }
+    dns_task_.reset();
     net_error_ = ERR_ABORTED;
     os_error_ = 0;
-    CompleteRequests(AddressList());
+    CompleteRequests(AddressList(), base::TimeDelta());
+  }
+
+  bool is_dns_running() const {
+    return dns_task_.get() != NULL;
+  }
+
+  bool is_proc_running() const {
+    return proc_task_.get() != NULL;
   }
 
   bool is_running() const {
-    return proc_task_.get() != NULL;
+    return is_dns_running() || is_proc_running();
   }
 
   // Called by HostResolverImpl when this job is evicted due to queue overflow.
   void OnEvicted() {
     // Must not be running.
     DCHECK(!is_running());
-    handle_ = PrioritizedDispatcher::Handle();
+    handle_.Reset();
 
     net_log_.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_EVICTED, NULL);
 
     // This signals to CompleteRequests that this job never ran.
     net_error_ = ERR_HOST_RESOLVER_QUEUE_TOO_LARGE;
     os_error_ = 0;
-    CompleteRequests(AddressList());
+    CompleteRequests(AddressList(), base::TimeDelta());
   }
 
   // PriorityDispatch::Job interface.
   virtual void Start() OVERRIDE {
     DCHECK(!is_running());
-    handle_ = PrioritizedDispatcher::Handle();
+    handle_.Reset();
 
     net_log_.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_STARTED, NULL);
 
+    if (resolver_->dns_transaction_factory_.get()) {
+      StartDnsTask();
+    } else {
+      StartProcTask();
+    }
+  }
+
+ private:
+  void StartProcTask() {
     proc_task_ = new ProcTask(
         key_,
         resolver_->proc_params_,
         base::Bind(&Job::OnProcTaskComplete, base::Unretained(this)),
         net_log_);
 
     if (had_non_speculative_request_)
       proc_task_->set_had_non_speculative_request();
     // Start() could be called from within Resolve(), hence it must NOT directly
     // call OnProcTaskComplete, for example, on synchronous failure.
     proc_task_->Start();
   }
 
- private:
+  void StartDnsTask() {
+    dns_task_.reset(new DnsTask(
+        resolver_->dns_transaction_factory_.get(),
+        key_,
+        base::Bind(&Job::OnDnsTaskComplete, base::Unretained(this)),
+        net_log_));
+
+    int rv = dns_task_->Start();
+    if (rv != ERR_IO_PENDING) {
+      DCHECK_NE(OK, rv);
+      // TODO(szym): AddToDispatcher(resolver_->proc_dispatcher_);
+      StartProcTask();
+    }
+  }
+
   // Called by ProcTask when it completes.
   void OnProcTaskComplete(int net_error, int os_error,

[cbentzel, 2012/02/14 13:08:26]

Nit: probably easier to read if this were moved directly under StartProcTask.

-                          const AddressList& addrlist) {
-    DCHECK(is_running());
+                          const AddressList& addr_list) {
+    DCHECK(is_proc_running());
     proc_task_ = NULL;
     net_error_ = net_error;
     os_error_ = os_error;
 
-    // We are the only consumer of |addrlist|, so we can safely change the port
-    // without copy-on-write. This pays off, when job has only one request.
-    AddressList list = addrlist;
-    if (net_error == OK && !requests_.empty())
-      MutableSetPort(requests_.front()->info().port(), &list);
-    CompleteRequests(list);
+    base::TimeDelta ttl = base::TimeDelta::FromSeconds(0);

[cbentzel, 2012/02/14 13:08:26]

Should there be a constant for negative cache TTL? 

+    if (net_error == OK)
+      ttl = base::TimeDelta::FromSeconds(kCacheEntryTTLSeconds);
+    CompleteRequests(addr_list, ttl);
+  }
+
+  // Called by DnsTask when it completes.
+  void OnDnsTaskComplete(int net_error,
+                         const AddressList& addr_list,
+                         base::TimeDelta ttl) {
+    DCHECK(is_dns_running());
+    dns_task_.reset();
+
+    if (net_error != OK) {
+      // TODO(szym): Some net errors indicate lack of connectivity. Starting
+      // ProcTask in that case is a waste of time.
+      StartProcTask();
+      return;
+    }
+
+    net_error_ = OK;
+    os_error_ = 0;
+    CompleteRequests(addr_list, ttl);
   }
 
   // Completes all Requests. Calls OnJobFinished and deletes self.
-  void CompleteRequests(const AddressList& addrlist) {
+  void CompleteRequests(const AddressList& addr_list, base::TimeDelta ttl) {
     CHECK(resolver_);
 
+    // We are the only consumer of |addr_list|, so we can safely change the port
+    // without copy-on-write. This pays off, when job has only one request.
+    AddressList list = addr_list;
+    if (net_error_ == OK && !requests_.empty())
+      MutableSetPort(requests_.front()->info().port(), &list);
+
     // This job must be removed from resolver's |jobs_| now to make room for a
     // new job with the same key in case one of the OnComplete callbacks decides
     // to spawn one. Consequently, the job deletes itself when CompleteRequests
     // is done.
     scoped_ptr<Job> self_deleter(this);
-    resolver_->OnJobFinished(this, addrlist);
+    resolver_->OnJobFinished(this, addr_list, ttl);

[cbentzel, 2012/02/14 13:08:26]

Does this need to be called after deleting self? Will this Job still be in the
PrioritizedDispatcher?

[szym, 2012/02/14 15:56:24]

I'm not clear what you mean. After deleting self I cannot access |resolver_|.
The critical ordering is that the Job is removed from |HostResolverImpl::jobs_|
before running any of the Request callbacks.

On second point: this Job cannot be in the dispatcher. CompleteRequests applies
only if the Job has already been started, which means it is out of the
dispatcher.

[cbentzel, 2012/02/14 18:01:27]

It was the second point I was concerned about. Agree that it's out of the
dispatcher. If it did do after delete self, it would need to copy resolver_ to a
stack variable to prevent the use-after-free. 

 
     // Complete all of the requests that were attached to the job.
     for (RequestsList::const_iterator it = requests_.begin();
          it != requests_.end(); ++it) {
       Request* req = *it;
 
       if (req->was_canceled())
         continue;
 
       DCHECK_EQ(this, req->job());
       // Update the net log and notify registered observers.
       LogFinishRequest(req->source_net_log(), req->request_net_log(),
                        req->info(), net_error_, os_error_);
 
-      req->OnComplete(net_error_, addrlist);
+      req->OnComplete(net_error_, addr_list);
 
       // Check if the resolver was destroyed as a result of running the
       // callback. If it was, we could continue, but we choose to bail.
       if (!resolver_)
         return;
     }
   }
 
-  // Used to call OnJobFinished and RemoveJob.
+  // Used to call OnJobFinished.
   base::WeakPtr<HostResolverImpl> resolver_;
 
   Key key_;
 
   // Tracks the highest priority across |requests_|.
   PriorityTracker priority_tracker_;
 
   bool had_non_speculative_request_;
 
   BoundNetLog net_log_;
 
   // Store result here in case the job fails fast in Resolve().
   int net_error_;
   int os_error_;
 
-  // A ProcTask created and started when this Job is dispatched..
+  // Resolves the host using a HostResolverProc.
   scoped_refptr<ProcTask> proc_task_;
 
+  // Resolves the host using a DnsTransaction.
+  scoped_ptr<DnsTask> dns_task_;
+
   // All Requests waiting for the result of this Job. Some can be canceled.
   RequestsList requests_;
 
   // A handle used by HostResolverImpl in |dispatcher_|.
-  PrioritizedDispatcher::Handle handle_;
+  DispatcherHandle handle_;
 };
 
 //-----------------------------------------------------------------------------
 
 HostResolverImpl::ProcTaskParams::ProcTaskParams(
     HostResolverProc* resolver_proc,
     size_t max_retry_attempts)
     : resolver_proc(resolver_proc),
       max_retry_attempts(max_retry_attempts),
       unresponsive_delay(base::TimeDelta::FromMilliseconds(6000)),
       retry_factor(2) {
 }
 
 HostResolverImpl::ProcTaskParams::~ProcTaskParams() {}
 
 HostResolverImpl::HostResolverImpl(
     HostCache* cache,
     const PrioritizedDispatcher::Limits& job_limits,
+    const PrioritizedDispatcher::Limits& proc_limits,
     const ProcTaskParams& proc_params,
+    scoped_ptr<DnsConfigService> dns_config_service,
     NetLog* net_log)
     : cache_(cache),
       dispatcher_(job_limits),
+      proc_dispatcher_(proc_limits),

[cbentzel, 2012/02/14 13:08:26]

Is proc_dispatcher_ even used? It looks like we just use dispatcher_ for all
scheduling.

It looks like there is a TODO for it. However, we'd need to add some state to
Job to know whether to do a DNS task or a Proc task. Also, we'd probably want to
go back up to HostResolverImpl if the DNS Job completes so it can be removed
from dispatcher_, and then a new Job added to prioritized_dispatcher_. This may
be a bit ugly because we'd need to transfer the requests over.

[szym, 2012/02/14 15:56:24]

Right now it is not. I'm okay removing it until we determine that we need
tighter limits for ProcTask than for DnsTask (which is expected, but we do not
yet have particular numbers in mind).
Not needed. A Job that has a DnsTask cannot be in any dispatcher. I'll add some
comments to make it more apparent.

[cbentzel, 2012/02/14 18:01:27]

I'd recommend removing it for now, as well as the additional set of limits
passed into HostResolverImpl.

       max_queued_jobs_(job_limits.total_jobs * 100u),
       proc_params_(proc_params),
       default_address_family_(ADDRESS_FAMILY_UNSPECIFIED),
+      dns_config_service_(dns_config_service.Pass()),
       ipv6_probe_monitoring_(false),
       additional_resolver_flags_(0),
       net_log_(net_log) {
 
   DCHECK_GE(dispatcher_.num_priorities(), static_cast<size_t>(NUM_PRIORITIES));
+  DCHECK_EQ(proc_dispatcher_.num_priorities(), dispatcher_.num_priorities());
 
   // Maximum of 4 retry attempts for host resolution.
   static const size_t kDefaultMaxRetryAttempts = 4u;
 
   if (proc_params_.max_retry_attempts == HostResolver::kDefaultRetryAttempts)
     proc_params_.max_retry_attempts = kDefaultMaxRetryAttempts;
 
 #if defined(OS_WIN)
   EnsureWinsockInit();
 #endif
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
   if (HaveOnlyLoopbackAddresses())
     additional_resolver_flags_ |= HOST_RESOLVER_LOOPBACK_ONLY;
 #endif
   NetworkChangeNotifier::AddIPAddressObserver(this);
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD)
 #if !defined(OS_ANDROID)
   EnsureDnsReloaderInit();
 #endif
   NetworkChangeNotifier::AddDNSObserver(this);
 #endif
+
+  if (dns_config_service_.get())
+    dns_config_service_->AddObserver(this);
 }
 
 HostResolverImpl::~HostResolverImpl() {
   DiscardIPv6ProbeJob();
 
   // This will also cancel all outstanding requests.
   STLDeleteValues(&jobs_);
 
   NetworkChangeNotifier::RemoveIPAddressObserver(this);
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD)
@@ skipping 33 matching lines @@
     return rv;
   }
 
   // Next we need to attach our request to a "job". This job is responsible for
   // calling "getaddrinfo(hostname)" on a worker thread.
 
   JobMap::iterator jobit = jobs_.find(key);
   Job* job;
   if (jobit == jobs_.end()) {
     // Create new Job.
-    job = new Job(this, key, request_net_log);
-    job->set_handle(dispatcher_.Add(job, info.priority()));
+    job = new Job(this, key, request_net_log, info.priority());
+    job->AddToDispatcher(&dispatcher_);
 
     // Check for queue overflow.
     if (dispatcher_.num_queued_jobs() > max_queued_jobs_) {
       Job* evicted = static_cast<Job*>(dispatcher_.EvictOldestLowest());
       DCHECK(evicted);
       if (evicted == job) {
         delete job;
         rv = ERR_HOST_RESOLVER_QUEUE_TOO_LARGE;
         LogFinishRequest(source_net_log, request_net_log, info, rv, 0);
         return rv;
       }
       evicted->OnEvicted();  // Deletes |evicted|.
     }
 
     jobs_.insert(jobit, std::make_pair(key, job));
   } else {
     job = jobit->second;
   }
 
   // Can't complete synchronously. Create and attach request.
   Request* req = new Request(source_net_log, request_net_log, info, callback,
                              addresses);
   job->AddRequest(req);
-  if (!job->handle().is_null())
-    job->set_handle(dispatcher_.ChangePriority(job->handle(), job->priority()));
   if (out_req)
     *out_req = reinterpret_cast<RequestHandle>(req);
 
   DCHECK_EQ(ERR_IO_PENDING, job->net_error());
   // Completion happens during Job::CompleteRequests().
   return ERR_IO_PENDING;
 }
 
 int HostResolverImpl::ResolveHelper(const Key& key,
                                     const RequestInfo& info,
@@ skipping 37 matching lines @@
 void HostResolverImpl::CancelRequest(RequestHandle req_handle) {
   DCHECK(CalledOnValidThread());
   Request* req = reinterpret_cast<Request*>(req_handle);
   DCHECK(req);
 
   Job* job = req->job();
   DCHECK(job);
 
   job->CancelRequest(req);
 
-  if (!job->handle().is_null()) {
-    // Still in queue.
-    if (job->num_active_requests()) {
-      job->set_handle(dispatcher_.ChangePriority(job->handle(),
-                                                 job->priority()));
+  if (job->num_active_requests() == 0) {
+    if (job->is_running()) {
+      // Job is running (and could be in CompleteRequests right now).
+      // But to be in Request::OnComplete we would have to have a non-canceled
+      // request. So it is safe to Abort it if it has no more active requests.
+      job->Abort();
     } else {
-      dispatcher_.Cancel(job->handle());
       RemoveJob(job);
       delete job;
     }
-  } else {
-    // Job is running (and could be in CompleteRequests right now).
-    // But to be in Request::OnComplete we would have to have a non-canceled
-    // request. So it is safe to Abort it if it has no more active requests.
-    if (!job->num_active_requests()) {
-      job->Abort();
-    }
   }
 }
 
 void HostResolverImpl::SetDefaultAddressFamily(AddressFamily address_family) {
   DCHECK(CalledOnValidThread());
   ipv6_probe_monitoring_ = false;
   DiscardIPv6ProbeJob();
   default_address_family_ = address_family;
 }
 
@@ skipping 54 matching lines @@
   if (!cache_entry)
     return false;
 
   request_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_CACHE_HIT, NULL);
   *net_error = cache_entry->error;
   if (*net_error == OK)
     *addresses = CreateAddressListUsingPort(cache_entry->addrlist, info.port());
   return true;
 }
 
-void HostResolverImpl::OnJobFinished(Job* job, const AddressList& addrlist) {
+void HostResolverImpl::OnJobFinished(Job* job,
+                                     const AddressList& addrlist,
+                                     base::TimeDelta ttl) {
   DCHECK(job);
-  DCHECK(job->handle().is_null());
+  DCHECK(!job->IsWaitingInDispatch());
   RemoveJob(job);
   if (job->net_error() == ERR_HOST_RESOLVER_QUEUE_TOO_LARGE)
     return;
 
   // Signal dispatcher that a slot has opened.
   dispatcher_.OnJobFinished();
   if (job->net_error() == ERR_ABORTED)
     return;
   // Write result to the cache.
   if (cache_.get()) {
-    base::TimeDelta ttl = base::TimeDelta::FromSeconds(0);
-    if (job->net_error() == OK)
-      ttl = base::TimeDelta::FromSeconds(kCacheEntryTTLSeconds);
     cache_->Set(job->key(), job->net_error(), addrlist,
                 base::TimeTicks::Now(), ttl);
   }
 }
 
 void HostResolverImpl::RemoveJob(Job* job) {
   DCHECK(job);
   jobs_.erase(job->key());
 }
 
@@ skipping 39 matching lines @@
     Job* job = it->second;
     // Advance the iterator before we might erase it.
     ++it;
     if (job->is_running()) {
       job->Abort();
       // Check if resolver was deleted in a request callback.
       if (!self)
         return;
     } else {
       // Keep it in |dispatch_|.
-      DCHECK(!job->handle().is_null());
+      DCHECK(job->IsWaitingInDispatch());
     }
   }
 }
 
 void HostResolverImpl::OnIPAddressChanged() {
   if (cache_.get())
     cache_->clear();
   if (ipv6_probe_monitoring_) {
     DiscardIPv6ProbeJob();
     ipv6_probe_job_ = new IPv6ProbeJob(this);
@@ skipping 16 matching lines @@
   // as NSCD's cache should be dropped automatically by the OS when
   // resolv.conf changes so we don't need to do anything to clear that cache.
   if (cache_.get())
     cache_->clear();
   // Existing jobs will have been sent to the original server so they need to
   // be aborted. TODO(Craig): Should these jobs be restarted?
   AbortAllInProgressJobs();
   // |this| may be deleted inside AbortAllInProgressJobs().
 }
 
+void HostResolverImpl::OnConfigChanged(const DnsConfig& dns_config) {
+  // We want a new factory in place, before we Abort running Jobs, so that the
+  // newly started jobs use the new factory.
+  bool had_factory = (dns_transaction_factory_.get() != NULL);
+  if (dns_config.IsValid()) {
+    dns_transaction_factory_ = DnsTransactionFactory::CreateFactory(
+        new DnsSession(dns_config,
+                       ClientSocketFactory::GetDefaultFactory(),
+                       base::Bind(&base::RandInt),
+                       net_log_));
+  } else {
+    dns_transaction_factory_.reset();
+  }
+  if (had_factory) {
+    OnDNSChanged();
+  }
+}
+
 }  // namespace net