Refactoring of job dispatch in HostResolverImpl in preparation for DnsClient.

net/base/host_resolver_impl.cc

 // Copyright (c) 2011 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 <algorithm>
 #include <cmath>
-#include <deque>
 #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/memory/weak_ptr.h"

[mmenke, 2011/12/21 16:22:58]

Since SupportsWeakPtr is defined in this file, this should be included in
host_resolver_impl.h rather than here.

[szym, 2011/12/28 01:24:10]

Done.

 #include "base/message_loop_proxy.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.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_list.h"
@@ skipping 101 matching lines @@
                                        NetLog* net_log) {
   // Maximum of 8 concurrent resolver threads.
   // Some routers (or resolvers) appear to start to provide host-not-found if
   // too many simultaneous resolutions are pending.  This number needs to be
   // further optimized, but 8 is what FF currently does.
   static const size_t kDefaultMaxJobs = 8u;
 
   if (max_concurrent_resolves == HostResolver::kDefaultParallelism)
     max_concurrent_resolves = kDefaultMaxJobs;
 
-  HostResolverImpl* resolver =
-      new HostResolverImpl(NULL, HostCache::CreateDefaultCache(),
-          max_concurrent_resolves, max_retry_attempts, net_log);
+  // TODO(szym): Add experiments with reserved slots for higher priority
+  // requests.
+
+  HostResolverImpl* resolver = new HostResolverImpl(
+      HostCache::CreateDefaultCache(),
+      PriorityDispatch::Limits::MakeAny(max_concurrent_resolves),
+      HostResolverImpl::ProcJobParams(NULL, max_retry_attempts),
+      net_log);
 
   return resolver;
 }
 
 static int ResolveAddrInfo(HostResolverProc* resolver_proc,
                            const std::string& host,
                            AddressFamily address_family,
                            HostResolverFlags host_resolver_flags,
                            AddressList* out,
                            int* os_error) {
@@ skipping 96 matching lines @@
     return dict;
   }
 
  private:
   const std::string host_;
   const NetLog::Source source_;
 };
 
 //-----------------------------------------------------------------------------
 
 class HostResolverImpl::Request {

[mmenke, 2011/12/22 16:18:49]

Think we should add a comment here that these are owned by the Job, and exist
until the Job is destroyed, even when cancelled.

[szym, 2011/12/28 01:24:10]

Done.

  public:
   Request(const BoundNetLog& source_net_log,
           const BoundNetLog& request_net_log,
           const RequestInfo& info,
           const CompletionCallback& callback,
           AddressList* addresses)
       : source_net_log_(source_net_log),
         request_net_log_(request_net_log),
         info_(info),
         job_(NULL),
         callback_(callback),
         addresses_(addresses) {
   }
 
-  // Mark the request as cancelled.
-  void MarkAsCancelled() {
+  // Mark the request as canceled (so that we don't have to remove it from
+  // a RequestsList).
+  void MarkAsCanceled() {
     job_ = NULL;
     addresses_ = NULL;
     callback_.Reset();
   }
 
-  bool was_cancelled() const {
+  bool was_canceled() const {
     return callback_.is_null();
   }
 
   void set_job(Job* job) {
-    DCHECK(job != NULL);
+    DCHECK(job);
     // Identify which job the request is waiting on.
     job_ = job;
   }
 
+  // Prepare final AddressList and call completion callback.
   void OnComplete(int error, const AddressList& addrlist) {
     if (error == OK)
       *addresses_ = CreateAddressListUsingPort(addrlist, port());
     CompletionCallback callback = callback_;
-    MarkAsCancelled();
+    MarkAsCanceled();
     callback.Run(error);
   }
 
   int port() const {
     return info_.port();
   }
 
   Job* job() const {
     return job_;
   }
 
+  // NetLog for the source, passed in HostResolver::Resolve.
   const BoundNetLog& source_net_log() {
     return source_net_log_;
   }
 
+  // NetLog for this request.
   const BoundNetLog& request_net_log() {
     return request_net_log_;
   }
 
   const RequestInfo& info() const {
     return info_;
   }
 
  private:
   BoundNetLog source_net_log_;
   BoundNetLog request_net_log_;
 
   // The request info that started the request.
   RequestInfo info_;
 
-  // The resolve job (running in worker pool) that this request is dependent on.
+  // The resolve job that this request is dependent on.
   Job* job_;
 
   // The user's callback to invoke when the request completes.
   CompletionCallback callback_;
 
   // The address list to save result into.
   AddressList* addresses_;
 
   DISALLOW_COPY_AND_ASSIGN(Request);
 };
 
 //------------------------------------------------------------------------------
 
 // Provide a common macro to simplify code and readability. We must use a
 // macros as the underlying HISTOGRAM macro creates static varibles.
 #define DNS_HISTOGRAM(name, time) UMA_HISTOGRAM_CUSTOM_TIMES(name, time, \
     base::TimeDelta::FromMicroseconds(1), base::TimeDelta::FromHours(1), 100)
 
-// This class represents a request to the worker pool for a "getaddrinfo()"
-// call.
-class HostResolverImpl::Job
-    : public base::RefCountedThreadSafe<HostResolverImpl::Job> {
+// Calls HostResolverProc on the WorkerPool. Performs retries if necessary.
+// TODO(szym): Move to separate source file for testing and mocking.
+class HostResolverImpl::ProcJob
+    : public base::RefCountedThreadSafe<HostResolverImpl::ProcJob> {

[mmenke, 2011/12/22 16:18:49]

I think using the names "Job" and "ProcJob" are a little confusing.  As it is,
when one sees "job", in this file, it's not clear if it means a Job or a
ProcJob.  I suggest removing the word "Job" entirely from one name or the other.
 ProcTask?  SyncResolverTask?  GetAddrInfoTask?  I'm open to other ideas.

[mmenke, 2011/12/22 16:19:54]

Alternatively, could add a prefix to Job.

[szym, 2011/12/28 01:24:10]

ProcTask it is.

  public:
-  Job(int id,
-      HostResolverImpl* resolver,
-      const Key& key,
-      const BoundNetLog& source_net_log,
-      NetLog* net_log)
-     : id_(id),
-       key_(key),
-       resolver_(resolver),
+  typedef base::Callback<void(int, int, const AddressList&)> Callback;

[mmenke, 2011/12/21 16:22:58]

nit:  Mind adding a linebreak here?

[szym, 2011/12/28 01:24:10]

Done.

+  ProcJob(const Key& key,
+          const ProcJobParams& params,
+          const Callback& callback,
+          const BoundNetLog& job_net_log)
+     : key_(key),

[mmenke, 2011/12/21 16:22:58]

nit:  Know this was like this before, but this should be a 4 space indent. 
Looks like 3 to me.

[szym, 2011/12/28 01:24:10]

Done.

+       params_(params),
+       callback_(callback),
        origin_loop_(base::MessageLoopProxy::current()),
-       resolver_proc_(resolver->effective_resolver_proc()),
-       unresponsive_delay_(resolver->unresponsive_delay()),
        attempt_number_(0),
        completed_attempt_number_(0),
        completed_attempt_error_(ERR_UNEXPECTED),
        had_non_speculative_request_(false),
-       net_log_(BoundNetLog::Make(net_log,
-                                  NetLog::SOURCE_HOST_RESOLVER_IMPL_JOB)) {
-    DCHECK(resolver);
-    net_log_.BeginEvent(
-        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB,
-        make_scoped_refptr(
-            new JobCreationParameters(key.hostname, source_net_log.source())));
-  }
+       net_log_(BoundNetLog::Make(job_net_log.net_log(),
+                                  NetLog::SOURCE_HOST_RESOLVER_IMPL_PROC_JOB)) {
+    if (!params_.resolver_proc_)
+      params_.resolver_proc_ = HostResolverProc::GetDefault();
 
-  // Attaches a request to this job. The job takes ownership of |req| and will
-  // take care to delete it.
-  void AddRequest(Request* req) {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    req->request_net_log().BeginEvent(
-        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_ATTACH,
-        make_scoped_refptr(new NetLogSourceParameter(
-            "source_dependency", net_log_.source())));
+    job_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_JOB,
+                         new NetLogSourceParameter("source_dependency",
+                                                   net_log_.source()));
 
-    req->set_job(this);
-    requests_.push_back(req);
-
-    if (!req->info().is_speculative())
-      had_non_speculative_request_ = true;
+    net_log_.BeginEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_JOB,
+                        new JobCreationParameters(key_.hostname,
+                                                  job_net_log.source()));
   }
 
   void Start() {
     DCHECK(origin_loop_->BelongsToCurrentThread());
     StartLookupAttempt();
   }
 
+  // Cancels this ProcJob. It will be orphaned. Any outstanding resolve
+  // attempts running on worker threads will continue running. Only once all the
+  // attempts complete will the final reference to this ProcJob be released.
+  void Cancel() {
+    DCHECK(origin_loop_->BelongsToCurrentThread());
+
+    if (was_canceled())
+      return;
+
+    net_log_.AddEvent(NetLog::TYPE_CANCELLED, NULL);
+
+    callback_.Reset();
+
+    net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_JOB, NULL);
+  }
+
+  void set_had_non_speculative_request() {
+    DCHECK(origin_loop_->BelongsToCurrentThread());
+    had_non_speculative_request_ = true;
+  }
+
+  bool was_canceled() const {
+    DCHECK(origin_loop_->BelongsToCurrentThread());
+    return callback_.is_null();
+  }
+
+  bool was_completed() const {
+    DCHECK(origin_loop_->BelongsToCurrentThread());
+    return completed_attempt_number_ > 0;
+  }
+
+ private:
   void StartLookupAttempt() {
     DCHECK(origin_loop_->BelongsToCurrentThread());
     base::TimeTicks start_time = base::TimeTicks::Now();
     ++attempt_number_;
     // Dispatch the lookup attempt to a worker thread.
     if (!base::WorkerPool::PostTask(
             FROM_HERE,
-            base::Bind(&Job::DoLookup, this, start_time, attempt_number_),
+            base::Bind(&ProcJob::DoLookup, this, start_time, attempt_number_),
             true)) {
       NOTREACHED();
 
       // Since we could be running within Resolve() right now, we can't just
       // call OnLookupComplete().  Instead we must wait until Resolve() has
       // returned (IO_PENDING).
       origin_loop_->PostTask(
           FROM_HERE,
-          base::Bind(&Job::OnLookupComplete, this, AddressList(),
+          base::Bind(&ProcJob::OnLookupComplete, this, AddressList(),
                      start_time, attempt_number_, ERR_UNEXPECTED, 0));
       return;
     }
 
     net_log_.AddEvent(
         NetLog::TYPE_HOST_RESOLVER_IMPL_ATTEMPT_STARTED,
         make_scoped_refptr(new NetLogIntegerParameter(
             "attempt_number", attempt_number_)));
 
     // Post a task to check if we get the results within a given time.
     // OnCheckForComplete has the potential for starting a new attempt on a
     // different worker thread if none of our outstanding attempts have
     // completed yet.
-    if (attempt_number_ <= resolver_->max_retry_attempts()) {
+    if (attempt_number_ <= params_.max_retry_attempts_) {
       origin_loop_->PostDelayedTask(
           FROM_HERE,
-          base::Bind(&Job::OnCheckForComplete, this),
-          unresponsive_delay_.InMilliseconds());
+          base::Bind(&ProcJob::OnCheckForComplete, this),
+          params_.unresponsive_delay_.InMilliseconds());
     }
   }
 
-  // Cancels the current job. The Job will be orphaned. Any outstanding resolve
-  // attempts running on worker threads will continue running. Only once all the
-  // attempts complete will the final reference to this Job be released.
-  void Cancel() {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    net_log_.AddEvent(NetLog::TYPE_CANCELLED, NULL);
-
-    HostResolver* resolver = resolver_;
-    resolver_ = NULL;
-
-    // End here to prevent issues when a Job outlives the HostResolver that
-    // spawned it.
-    net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB, NULL);
-
-    // We will call HostResolverImpl::CancelRequest(Request*) on each one
-    // in order to notify any observers.
-    for (RequestsList::const_iterator it = requests_.begin();
-         it != requests_.end(); ++it) {
-      HostResolverImpl::Request* req = *it;
-      if (!req->was_cancelled())
-        resolver->CancelRequest(req);
-    }
-  }
-
-  bool was_cancelled() const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    return resolver_ == NULL;
-  }
-
-  bool was_completed() const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    return completed_attempt_number_ > 0;
-  }
-
-  const Key& key() const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    return key_;
-  }
-
-  int id() const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    return id_;
-  }
-
-  const RequestsList& requests() const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    return requests_;
-  }
-
-  // Returns the first request attached to the job.
-  const Request* initial_request() const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    DCHECK(!requests_.empty());
-    return requests_[0];
-  }
-
-  // Returns true if |req_info| can be fulfilled by this job.
-  bool CanServiceRequest(const RequestInfo& req_info) const {
-    DCHECK(origin_loop_->BelongsToCurrentThread());
-    return key_ == resolver_->GetEffectiveKeyForRequest(req_info);
-  }
-
- private:
-  friend class base::RefCountedThreadSafe<HostResolverImpl::Job>;
-
-  ~Job() {
-    // Free the requests attached to this job.
-    STLDeleteElements(&requests_);
-  }
-
   // WARNING: This code runs inside a worker pool. The shutdown code cannot
   // wait for it to finish, so we must be very careful here about using other
   // objects (like MessageLoops, Singletons, etc). During shutdown these objects
   // may no longer exist. Multiple DoLookups() could be running in parallel, so
   // any state inside of |this| must not mutate .
   void DoLookup(const base::TimeTicks& start_time,
                 const uint32 attempt_number) {
     AddressList results;
     int os_error = 0;
     // Running on the worker thread
-    int error = ResolveAddrInfo(resolver_proc_,
+    int error = ResolveAddrInfo(params_.resolver_proc_,
                                 key_.hostname,
                                 key_.address_family,
                                 key_.host_resolver_flags,
                                 &results,
                                 &os_error);
 
     origin_loop_->PostTask(
         FROM_HERE,
-        base::Bind(&Job::OnLookupComplete, this, results, start_time,
+        base::Bind(&ProcJob::OnLookupComplete, this, results, start_time,
                    attempt_number, error, os_error));
   }
 
   // Callback to see if DoLookup() has finished or not (runs on origin thread).
   void OnCheckForComplete() {
     DCHECK(origin_loop_->BelongsToCurrentThread());
 
-    if (was_completed() || was_cancelled())
+    if (was_completed() || was_canceled())
       return;
 
-    DCHECK(resolver_);
-    unresponsive_delay_ *= resolver_->retry_factor();
+    params_.unresponsive_delay_ *= params_.retry_factor_;
     StartLookupAttempt();
   }
 
   // Callback for when DoLookup() completes (runs on origin thread).
   void OnLookupComplete(const AddressList& results,
                         const base::TimeTicks& start_time,
                         const uint32 attempt_number,
                         int error,
                         const int os_error) {
     DCHECK(origin_loop_->BelongsToCurrentThread());
     DCHECK(error || results.head());
 
     bool was_retry_attempt = attempt_number > 1;
 
-    if (!was_cancelled()) {
+    if (!was_canceled()) {
       scoped_refptr<NetLog::EventParameters> params;
       if (error != OK) {
         params = new HostResolveFailedParams(attempt_number, error, os_error);
       } else {
         params = new NetLogIntegerParameter("attempt_number", attempt_number_);
       }
       net_log_.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_ATTEMPT_FINISHED,
                         params);
 
       // If host is already resolved, then record data and return.
@@ skipping 20 matching lines @@
     if (error != OK && NetworkChangeNotifier::IsOffline())
       error = ERR_INTERNET_DISCONNECTED;
 
     // We will record data for the first attempt. Don't contaminate with retry
     // attempt's data.
     if (!was_retry_attempt)
       RecordPerformanceHistograms(start_time, error, os_error);
 
     RecordAttemptHistograms(start_time, attempt_number, error, os_error);
 
-    if (was_cancelled())
+    if (was_canceled())
       return;
 
     if (was_retry_attempt) {
       // If retry attempt finishes before 1st attempt, then get stats on how
       // much time is saved by having spawned an extra attempt.
       retry_attempt_finished_time_ = base::TimeTicks::Now();
     }
 
     scoped_refptr<NetLog::EventParameters> params;
     if (error != OK) {
       params = new HostResolveFailedParams(0, error, os_error);
     } else {
       params = new AddressListNetLogParam(results_);
     }
 
-    // End here to prevent issues when a Job outlives the HostResolver that
-    // spawned it.
-    net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB, params);
+    net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_JOB, params);
 
-    DCHECK(!requests_.empty());
-
-     // Use the port number of the first request.
-    if (error == OK)
-      MutableSetPort(requests_[0]->port(), &results_);
-
-    resolver_->OnJobComplete(this, error, os_error, results_);
+    Callback callback = callback_;
+    callback_.Reset();
+    callback.Run(error, os_error, results_);
   }
 
   void RecordPerformanceHistograms(const base::TimeTicks& start_time,
                                    const int error,
                                    const int os_error) const {
     DCHECK(origin_loop_->BelongsToCurrentThread());
     enum Category {  // Used in HISTOGRAM_ENUMERATION.
       RESOLVE_SUCCESS,
       RESOLVE_FAIL,
       RESOLVE_SPECULATIVE_SUCCESS,
@@ skipping 75 matching lines @@
         DNS_HISTOGRAM(base::FieldTrial::MakeName("DNS.ResolveSuccess",
                                                  "DnsParallelism"), duration);
       }
     }
   }
 
   void RecordAttemptHistograms(const base::TimeTicks& start_time,
                                const uint32 attempt_number,
                                const int error,
                                const int os_error) const {
+    DCHECK(origin_loop_->BelongsToCurrentThread());
     bool first_attempt_to_complete =
         completed_attempt_number_ == attempt_number;
     bool is_first_attempt = (attempt_number == 1);
 
     if (first_attempt_to_complete) {
       // If this was first attempt to complete, then record the resolution
       // status of the attempt.
       if (completed_attempt_error_ == OK) {
         UMA_HISTOGRAM_ENUMERATION(
             "DNS.AttemptFirstSuccess", attempt_number, 100);
       } else {
         UMA_HISTOGRAM_ENUMERATION(
             "DNS.AttemptFirstFailure", attempt_number, 100);
       }
     }
 
     if (error == OK)
       UMA_HISTOGRAM_ENUMERATION("DNS.AttemptSuccess", attempt_number, 100);
     else
       UMA_HISTOGRAM_ENUMERATION("DNS.AttemptFailure", attempt_number, 100);
 
     // If first attempt didn't finish before retry attempt, then calculate stats
     // on how much time is saved by having spawned an extra attempt.
-    if (!first_attempt_to_complete && is_first_attempt && !was_cancelled()) {
+    if (!first_attempt_to_complete && is_first_attempt && !was_canceled()) {
       DNS_HISTOGRAM("DNS.AttemptTimeSavedByRetry",
                     base::TimeTicks::Now() - retry_attempt_finished_time_);
     }
 
-    if (was_cancelled() || !first_attempt_to_complete) {
+    if (was_canceled() || !first_attempt_to_complete) {
       // Count those attempts which completed after the job was already canceled
       // OR after the job was already completed by an earlier attempt (so in
       // effect).
       UMA_HISTOGRAM_ENUMERATION("DNS.AttemptDiscarded", attempt_number, 100);
 
-      // Record if job is cancelled.
-      if (was_cancelled())
+      // Record if job is canceled.
+      if (was_canceled())
         UMA_HISTOGRAM_ENUMERATION("DNS.AttemptCancelled", attempt_number, 100);
     }
 
     base::TimeDelta duration = base::TimeTicks::Now() - start_time;
     if (error == OK)
       DNS_HISTOGRAM("DNS.AttemptSuccessDuration", duration);
     else
       DNS_HISTOGRAM("DNS.AttemptFailDuration", duration);
   }
 
-  // Immutable. Can be read from either thread,
-  const int id_;
-
   // Set on the origin thread, read on the worker thread.
   Key key_;
 
-  // Only used on the origin thread (where Resolve was called).
-  HostResolverImpl* resolver_;
-  RequestsList requests_;  // The requests waiting on this job.
+  // Holds an owning reference to the HostResolverProc that we are going to use.
+  // This may not be the current resolver procedure by the time we call
+  // ResolveAddrInfo, but that's OK... we'll use it anyways, and the owning
+  // reference ensures that it remains valid until we are done.
+  ProcJobParams params_;
+
+  // The listener to the results of this ProcJob.
+  Callback callback_;
 
   // Used to post ourselves onto the origin thread.
   scoped_refptr<base::MessageLoopProxy> origin_loop_;
 
-  // Hold an owning reference to the HostResolverProc that we are going to use.
-  // This may not be the current resolver procedure by the time we call
-  // ResolveAddrInfo, but that's OK... we'll use it anyways, and the owning
-  // reference ensures that it remains valid until we are done.
-  scoped_refptr<HostResolverProc> resolver_proc_;
-
-  // The amount of time after starting a resolution attempt until deciding to
-  // retry.
-  base::TimeDelta unresponsive_delay_;
-
   // Keeps track of the number of attempts we have made so far to resolve the
   // host. Whenever we start an attempt to resolve the host, we increase this
   // number.
   uint32 attempt_number_;
 
   // The index of the attempt which finished first (or 0 if the job is still in
   // progress).
   uint32 completed_attempt_number_;
 
   // The result (a net error code) from the first attempt to complete.
   int completed_attempt_error_;
 
   // The time when retry attempt was finished.
   base::TimeTicks retry_attempt_finished_time_;
 
   // True if a non-speculative request was ever attached to this job
-  // (regardless of whether or not it was later cancelled.
+  // (regardless of whether or not it was later canceled.
   // This boolean is used for histogramming the duration of jobs used to
   // service non-speculative requests.
   bool had_non_speculative_request_;
 
   AddressList results_;
 
   BoundNetLog net_log_;
 
-  DISALLOW_COPY_AND_ASSIGN(Job);
+  DISALLOW_COPY_AND_ASSIGN(ProcJob);
 };
 
 //-----------------------------------------------------------------------------
 
-// This class represents a request to the worker pool for a "probe for IPv6
-// support" call.
+// Represents a request to the worker pool for a "probe for IPv6 support" call.
 class HostResolverImpl::IPv6ProbeJob
     : public base::RefCountedThreadSafe<HostResolverImpl::IPv6ProbeJob> {
  public:
   explicit IPv6ProbeJob(HostResolverImpl* resolver)
       : resolver_(resolver),
         origin_loop_(base::MessageLoopProxy::current()) {
     DCHECK(resolver);
   }
 
   void Start() {
     DCHECK(origin_loop_->BelongsToCurrentThread());
-    if (was_cancelled())
+    if (was_canceled())
       return;
     const bool kIsSlow = true;
     base::WorkerPool::PostTask(
         FROM_HERE, base::Bind(&IPv6ProbeJob::DoProbe, this), kIsSlow);
   }
 
   // Cancels the current job.
   void Cancel() {
     DCHECK(origin_loop_->BelongsToCurrentThread());
-    if (was_cancelled())
+    if (was_canceled())
       return;
     resolver_ = NULL;  // Read/write ONLY on origin thread.
   }
 
  private:
   friend class base::RefCountedThreadSafe<HostResolverImpl::IPv6ProbeJob>;
 
   ~IPv6ProbeJob() {
   }
 
-  bool was_cancelled() const {
+  bool was_canceled() const {
     DCHECK(origin_loop_->BelongsToCurrentThread());
     return !resolver_;
   }
 
   // Run on worker thread.
   void DoProbe() {
     // Do actual testing on this thread, as it takes 40-100ms.
     AddressFamily family = IPv6Supported() ? ADDRESS_FAMILY_UNSPECIFIED
                                            : ADDRESS_FAMILY_IPV4;
 
     origin_loop_->PostTask(
         FROM_HERE,
         base::Bind(&IPv6ProbeJob::OnProbeComplete, this, family));
   }
 
   // Callback for when DoProbe() completes.
   void OnProbeComplete(AddressFamily address_family) {
     DCHECK(origin_loop_->BelongsToCurrentThread());
-    if (was_cancelled())
+    if (was_canceled())
       return;
     resolver_->IPv6ProbeSetDefaultAddressFamily(address_family);
   }
 
   // Used/set only on origin thread.
   HostResolverImpl* resolver_;
 
   // Used to post ourselves onto the origin thread.
   scoped_refptr<base::MessageLoopProxy> origin_loop_;
 
   DISALLOW_COPY_AND_ASSIGN(IPv6ProbeJob);
 };
 
 //-----------------------------------------------------------------------------
 
-// We rely on the priority enum values being sequential having starting at 0,
-// and increasing for lower priorities.
-COMPILE_ASSERT(HIGHEST == 0u &&
-               LOWEST > HIGHEST &&
-               IDLE > LOWEST &&
-               NUM_PRIORITIES > IDLE,
-               priority_indexes_incompatible);
-
-// JobPool contains all the information relating to queued requests, including
-// the limits on how many jobs are allowed to be used for this category of
-// requests.
-class HostResolverImpl::JobPool {
+// Keeps track of the highest priority.
+class PriorityTracker {
  public:
-  JobPool(size_t max_outstanding_jobs, size_t max_pending_requests)
-      : num_outstanding_jobs_(0u) {
-    SetConstraints(max_outstanding_jobs, max_pending_requests);
-  }
-
-  ~JobPool() {
-    // Free the pending requests.
-    for (size_t i = 0; i < arraysize(pending_requests_); ++i)
-      STLDeleteElements(&pending_requests_[i]);
-  }
-
-  // Sets the constraints for this pool. See SetPoolConstraints() for the
-  // specific meaning of these parameters.
-  void SetConstraints(size_t max_outstanding_jobs,
-                      size_t max_pending_requests) {
-    CHECK_NE(max_outstanding_jobs, 0u);
-    max_outstanding_jobs_ = max_outstanding_jobs;
-    max_pending_requests_ = max_pending_requests;
-  }
-
-  // Returns the number of pending requests enqueued to this pool.
-  // A pending request is one waiting to be attached to a job.
-  size_t GetNumPendingRequests() const {
-    size_t total = 0u;
-    for (size_t i = 0u; i < arraysize(pending_requests_); ++i)
-      total += pending_requests_[i].size();
-    return total;
-  }
-
-  bool HasPendingRequests() const {
-    return GetNumPendingRequests() > 0u;
-  }
-
-  // Enqueues a request to this pool. As a result of enqueing this request,
-  // the queue may have reached its maximum size. In this case, a request is
-  // evicted from the queue, and returned. Otherwise returns NULL. The caller
-  // is responsible for freeing the evicted request.
-  Request* InsertPendingRequest(Request* req) {
-    req->request_net_log().BeginEvent(
-        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_POOL_QUEUE,
-        NULL);
-
-    PendingRequestsQueue& q = pending_requests_[req->info().priority()];
-    q.push_back(req);
-
-    // If the queue is too big, kick out the lowest priority oldest request.
-    if (GetNumPendingRequests() > max_pending_requests_) {
-      // Iterate over the queues from lowest priority to highest priority.
-      for (int i = static_cast<int>(arraysize(pending_requests_)) - 1;
-           i >= 0; --i) {
-        PendingRequestsQueue& q = pending_requests_[i];
-        if (!q.empty()) {
-          Request* req = q.front();
-          q.pop_front();
-          req->request_net_log().AddEvent(
-              NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_POOL_QUEUE_EVICTED, NULL);
-          req->request_net_log().EndEvent(
-              NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_POOL_QUEUE, NULL);
-          return req;
+  explicit PriorityTracker(RequestPriority initial)

[mmenke, 2011/12/22 16:18:49]

Don't think there's any reason to take a RequestPriority as an argument, as
we're going to have to be calling Add immediately anyways.  Think we should just
start out at IDLE, save some unnecessary argument passing.

[szym, 2011/12/22 16:56:57]

Not quite immediately.

The Job is added to the dispatcher before the Request is Added to it (in fact,
before the Request is even created). This way if the Job is evicted on Add we
know not to run the callback. 

Alternatively, (1) we could put a flag in the Request as "do not call me back
yet, because I'm in Resolve", or (2) swap in a temporary callback that stores
the result to be later used once we return to Resolve.

Any preference for one of the three: (current), (1), or (2)?

[mmenke, 2011/12/22 17:34:30]

Ah, good point.  Think it's fine as-is, then.

[szym, 2011/12/28 01:24:10]

A new work-around in place using PrioritizedDispatcher::WouldEvict.

+      : highest_priority_(initial), total_count_(0) {
+    memset(counts_, 0, sizeof(counts_));
+  }
+
+  RequestPriority highest_priority() const {
+    return highest_priority_;
+  }
+
+  size_t total_count() const {
+    return total_count_;
+  }
+
+  // Returns true if priority changed.
+  bool Add(RequestPriority req_priority) {
+    ++total_count_;
+    ++counts_[req_priority];
+    if (highest_priority_ < req_priority)
+      return false;
+
+    highest_priority_ = req_priority;
+    return true;
+  }
+
+  // Returns true if priority changed.
+  bool Remove(RequestPriority req_priority) {
+    DCHECK_GT(total_count_, 0u);
+    DCHECK_GT(counts_[req_priority], 0u);
+    --total_count_;
+    --counts_[req_priority];
+    if ((counts_[req_priority] == 0u) && (highest_priority_ == req_priority)) {
+      for (size_t i = highest_priority_; i < NUM_PRIORITIES; ++i) {
+        if (counts_[i] > 0) {
+          highest_priority_ = static_cast<RequestPriority>(i);
+          return true;
         }
       }
-    }
-
-    return NULL;
-  }
-
-  // Erases |req| from this container. Caller is responsible for freeing
-  // |req| afterwards.
-  void RemovePendingRequest(Request* req) {
-    PendingRequestsQueue& q = pending_requests_[req->info().priority()];
-    PendingRequestsQueue::iterator it = std::find(q.begin(), q.end(), req);
-    DCHECK(it != q.end());
-    q.erase(it);
-    req->request_net_log().EndEvent(
-        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_POOL_QUEUE, NULL);
-  }
-
-  // Removes and returns the highest priority pending request.
-  Request* RemoveTopPendingRequest() {
-    DCHECK(HasPendingRequests());
-
-    for (size_t i = 0u; i < arraysize(pending_requests_); ++i) {
-      PendingRequestsQueue& q = pending_requests_[i];
-      if (!q.empty()) {
-        Request* req = q.front();
-        q.pop_front();
-        req->request_net_log().EndEvent(
-            NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_POOL_QUEUE, NULL);
-        return req;
+      DCHECK_EQ(0u, total_count_);
+      // In absence of requests set default.
+      if (highest_priority_ != IDLE) {
+        highest_priority_ = IDLE;
+        return true;
       }
     }
-
-    NOTREACHED();
-    return NULL;
-  }
-
-  // Keeps track of a job that was just added/removed, and belongs to this pool.
-  void AdjustNumOutstandingJobs(int offset) {
-    DCHECK(offset == 1 || (offset == -1 && num_outstanding_jobs_ > 0u));
-    num_outstanding_jobs_ += offset;
-  }
-
-  void ResetNumOutstandingJobs() {
-    num_outstanding_jobs_ = 0;
-  }
-
-  // Returns true if a new job can be created for this pool.
-  bool CanCreateJob() const {
-    return num_outstanding_jobs_ + 1u <= max_outstanding_jobs_;
-  }
-
-  // Removes any pending requests from the queue which are for the
-  // same (hostname / effective address-family) as |job|, and attaches them to
-  // |job|.
-  void MoveRequestsToJob(Job* job) {
-    for (size_t i = 0u; i < arraysize(pending_requests_); ++i) {
-      PendingRequestsQueue& q = pending_requests_[i];
-      PendingRequestsQueue::iterator req_it = q.begin();
-      while (req_it != q.end()) {
-        Request* req = *req_it;
-        if (job->CanServiceRequest(req->info())) {
-          // Job takes ownership of |req|.
-          job->AddRequest(req);
-          req_it = q.erase(req_it);
-        } else {
-          ++req_it;
-        }
+    return false;
+  }
+
+ private:
+  RequestPriority highest_priority_;
+  size_t total_count_;
+  size_t counts_[NUM_PRIORITIES];
+};
+
+//-----------------------------------------------------------------------------
+
+// Aggregates all Requests for the same Key. Dispatched via PriorityDispatch.
+// Spawns ProcJob when started.
+class HostResolverImpl::Job : public PriorityDispatch::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,
+      RequestPriority priority,
+      const BoundNetLog& request_net_log)
+      : 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)
+      Cancel();
+    net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB, NULL);

[mmenke, 2011/12/22 16:18:49]

Think we should log the error here, if any.  Code would be:

net_log_.EndEventWithNetError(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB, net_error_);

If net_error_ is non-negative, no error code will be logged.    While it will be
logged elsewhere, too, logging it here will make the Job have a red background
on about:net-internals, which can be useful.

[szym, 2011/12/28 01:24:10]

Done.

+    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 in |dispatch_|.
+  PriorityDispatch::Handle& handle() {
+    return handle_;
+  }
+
+  // The Job will own |req| and destroy it in ~Job.
+  void AddRequest(Request* req) {
+    DCHECK_EQ(key_.hostname, req->info().hostname());
+
+    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_ATTACH,
+        make_scoped_refptr(new NetLogSourceParameter(
+            "source_dependency", req->request_net_log().source())));

[mmenke, 2011/12/22 16:18:49]

May be a little simpler to just always add the resulting priority to this event.
 Up to you.

[szym, 2011/12/28 01:24:10]

Done.

+
+    req->set_job(this);
+    requests_.push_back(req);
+
+    if (priority_tracker_.Add(req->info().priority())) {
+      net_log_.AddEvent(
+          NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_PRIORITY_UPDATED,
+          make_scoped_refptr(new NetLogIntegerParameter(
+              "priority", priority())));
+    }
+
+    // TODO(szym): Check if this is still needed.
+    if (!req->info().is_speculative()) {
+      had_non_speculative_request_ = true;
+      if (proc_job_)
+        proc_job_->set_had_non_speculative_request();
+    }
+  }
+
+  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());
+    if (priority_tracker_.Remove(req->info().priority())) {
+      net_log_.AddEvent(
+          NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_PRIORITY_UPDATED,
+          make_scoped_refptr(new NetLogIntegerParameter(
+              "priority", priority())));
+    }
+  }
+
+  // Called by resolver in destructor.
+  void Cancel() {
+    if (is_running()) {
+      proc_job_->Cancel();
+      proc_job_ = NULL;

[mmenke, 2011/12/22 17:34:30]

nit:  I think calling release would be a little clearer.

[mmenke, 2011/12/22 17:55:14]

Oops...Nevermind.  Forgot that release with scoped_reftrs doesn't release the
reference.

+    }
+    if (!net_log_.net_log())

[mmenke, 2011/12/22 16:18:49]

Since this generally only happens during testing, don't think it's needed.  All
the logging functions check for this, anyways.

[szym, 2011/12/22 16:56:57]

I don't think cancellations will happen only during testing, but I agree this is
a pre-optimization.

[mmenke, 2011/12/22 17:34:30]

Sorry, I mean net_log_ will probably only be null during testing.

+      return;
+    net_log_.AddEvent(NetLog::TYPE_CANCELLED, NULL);
+    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());
+    }
+  }
+
+  // Called by ProcJob when it completes.
+  void OnProcJobComplete(int net_error, int os_error,
+                         const AddressList& addrlist) {
+    DCHECK(is_running());
+    proc_job_ = NULL;
+    net_error_ = net_error;
+    os_error_ = os_error;
+    CompleteRequests(addrlist);
+  }
+
+  void Abort() {
+    // Job should only be aborted if it's running.
+    DCHECK(is_running());
+    proc_job_->Cancel();
+    proc_job_ = NULL;
+    net_error_ = ERR_ABORTED;
+    os_error_ = 0;
+    CompleteRequests(AddressList());
+  }
+
+  bool was_evicted() const {
+    return net_error_ == ERR_HOST_RESOLVER_QUEUE_TOO_LARGE;
+  }
+
+  bool is_running() const {
+    return proc_job_.get() != NULL;
+  }
+
+  // PriorityDispatch::Job interface.
+  virtual void Start() OVERRIDE {
+    DCHECK(!is_running());
+    handle_ = PriorityDispatch::Handle();
+    if (net_log_.net_log()) {

[mmenke, 2011/12/22 16:18:49]

nit:  Again, don't think this is needed.

+      // Log end of queue.

[mmenke, 2011/12/22 16:18:49]

You don't actually seem to be logging anything here, just doing a DCHECK.

[szym, 2011/12/22 16:56:57]

Oops. Leftovers. Done.

+      for (RequestsList::const_iterator it = requests_.begin();
+           it != requests_.end(); ++it) {
+        Request* req = *it;
+        if (req->was_canceled())
+          continue;
+        DCHECK_EQ(this, req->job());
       }
     }
+    net_log_.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_STARTED, NULL);
+
+    proc_job_ = new ProcJob(
+        key_,
+        resolver_->proc_params_,
+        base::Bind(&Job::OnProcJobComplete, base::Unretained(this)),
+        net_log_);
+
+    if (had_non_speculative_request_)
+      proc_job_->set_had_non_speculative_request();
+    // Start() could be called from within Resolve(), hence it must NOT directly
+    // call OnProcJobComplete.

[mmenke, 2011/12/22 16:18:49]

nit:  To be a little clearer, I think you should add something like "on
synchronous failure" or some such.

[szym, 2011/12/28 01:24:10]

Done.

+    proc_job_->Start();
+  }
+
+  virtual void OnEvicted() OVERRIDE {
+    // Must not be running.
+    DCHECK(!is_running());
+    handle_ = PriorityDispatch::Handle();
+
+    net_log_.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_EVICTED, NULL);
+
+    // NOTE: This signals to OnJobFinished that this job never ran.
+    net_error_ = ERR_HOST_RESOLVER_QUEUE_TOO_LARGE;
+    os_error_ = 0;
+    CompleteRequests(AddressList());
   }
 
  private:
-  typedef std::deque<Request*> PendingRequestsQueue;
-
-  // Maximum number of concurrent jobs allowed to be started for requests
-  // belonging to this pool.
-  size_t max_outstanding_jobs_;
-
-  // The current number of running jobs that were started for requests
-  // belonging to this pool.
-  size_t num_outstanding_jobs_;
-
-  // The maximum number of requests we allow to be waiting on a job,
-  // for this pool.
-  size_t max_pending_requests_;
-
-  // The requests which are waiting to be started for this pool.
-  PendingRequestsQueue pending_requests_[NUM_PRIORITIES];
+  // Completes all Requests. Calls OnJobFinished and deletes self as needed.
+  void CompleteRequests(const AddressList& addrlist) {
+    CHECK(resolver_);
+
+    scoped_ptr<Job> self_deleter;
+    if (net_error_ != ERR_ABORTED) {
+      if (!was_evicted())
+        resolver_->OnJobFinished(this, addrlist);

[mmenke, 2011/12/22 16:18:49]

This is only called in response to OnProcJobComplete, right?  Seems to make more
sense to call it OnJobComplete, and call it from there, rather than checking
against the two other failure cases here.

[szym, 2011/12/22 16:56:57]

It makes sense for me to move the !was_evicted check to OnJobFinished, but we'll
still need to check for ERR_ABORTED.

The trickiness comes in the next two lines. Essentially, CompleteRequests needs
to hold the job in |self_deleter| because otherwise it might be gone when
req->OnComplete returns. (The Job could be Aborted or the resolver could be
destroyed.)

ERR_ABORTED needs to be checked, because AbortAllInProgressJobs iterates through
|jobs_| and calling RemoveJob would invalidate the iterator.

[mmenke, 2011/12/22 17:34:30]

Hmm...Actually, don't we need to call OnJobFinished on ERR_ABORTED when
is_running() is true?

A little more discussion on the deletion stuff below.

[szym, 2011/12/28 01:24:10]

That's correct, although that was already done in the body of
AbortAllInProgressJobs. In the new patch Job::Abort deletes the Job so
OnJobFinished is called except when evicted.

+      if (resolver_->RemoveJob(this))
+        self_deleter.reset(this);
+      // else: We are in AbortAllInProgressJobs() or Resolve() which will take
+      // care of deleting this.
+    }
+
+    // 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);
+
+      // 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.
+  base::WeakPtr<HostResolverImpl> resolver_;
+
+  Key key_;
+
+  // Tracks the highest priority across requests_.

[mmenke, 2011/12/22 16:18:49]

nit:  |requests|

[szym, 2011/12/28 01:24:10]

Done.

+  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 ProcJob created and started when this Job is dispatched..
+  scoped_refptr<ProcJob> proc_job_;
+
+  // All Requests waiting for the result of this Job. Some can be canceled.
+  RequestsList requests_;
+
+  // A handle used by HostResolverImpl in |dispatcher_|.
+  PriorityDispatch::Handle handle_;
 };
 
 //-----------------------------------------------------------------------------
 
+HostResolverImpl::ProcJobParams::ProcJobParams(
+    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::ProcJobParams::~ProcJobParams() {}
+
 HostResolverImpl::HostResolverImpl(
-    HostResolverProc* resolver_proc,
     HostCache* cache,
-    size_t max_jobs,
-    size_t max_retry_attempts,
+    const PriorityDispatch::Limits& job_limits,
+    const ProcJobParams& proc_params,
     NetLog* net_log)
     : cache_(cache),
-      max_jobs_(max_jobs),
-      max_retry_attempts_(max_retry_attempts),
-      unresponsive_delay_(base::TimeDelta::FromMilliseconds(6000)),
-      retry_factor_(2),
-      next_job_id_(0),
-      resolver_proc_(resolver_proc),
+      dispatch_(job_limits, job_limits.Total() * 100u),
+      proc_params_(proc_params),
       default_address_family_(ADDRESS_FAMILY_UNSPECIFIED),
       ipv6_probe_monitoring_(false),
       additional_resolver_flags_(0),
       net_log_(net_log) {
-  DCHECK_GT(max_jobs, 0u);
+
 
   // Maximum of 4 retry attempts for host resolution.
   static const size_t kDefaultMaxRetryAttempts = 4u;
 
-  if (max_retry_attempts_ == HostResolver::kDefaultRetryAttempts)
-    max_retry_attempts_ = kDefaultMaxRetryAttempts;
-
-  // It is cumbersome to expose all of the constraints in the constructor,
-  // so we choose some defaults, which users can override later.
-  job_pools_[POOL_NORMAL] = new JobPool(max_jobs, 100u * max_jobs);
+  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
 }
 
 HostResolverImpl::~HostResolverImpl() {
   // Cancel the outstanding jobs. Those jobs may contain several attached
-  // requests, which will also be cancelled.
+  // requests, which will also be canceled.
   DiscardIPv6ProbeJob();
 
   CancelAllJobs();
 
-  // In case we are being deleted during the processing of a callback.
-  if (cur_completing_job_)
-    cur_completing_job_->Cancel();
-
   NetworkChangeNotifier::RemoveIPAddressObserver(this);
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD)
   NetworkChangeNotifier::RemoveDNSObserver(this);
 #endif
-
-  // Delete the job pools.
-  for (size_t i = 0u; i < arraysize(job_pools_); ++i)
-    delete job_pools_[i];
 }
 
-void HostResolverImpl::SetPoolConstraints(JobPoolIndex pool_index,
-                                          size_t max_outstanding_jobs,
-                                          size_t max_pending_requests) {
-  DCHECK(CalledOnValidThread());
-  CHECK_GE(pool_index, 0);
-  CHECK_LT(pool_index, POOL_COUNT);
-  CHECK(jobs_.empty()) << "Can only set constraints during setup";
-  JobPool* pool = job_pools_[pool_index];
-  pool->SetConstraints(max_outstanding_jobs, max_pending_requests);
+void HostResolverImpl::SetMaxQueuedJobs(size_t max_queued) {
+  dispatch_.SetMaxQueued(max_queued);
 }
 
 int HostResolverImpl::Resolve(const RequestInfo& info,
                               AddressList* addresses,
                               const CompletionCallback& callback,
                               RequestHandle* out_req,
                               const BoundNetLog& source_net_log) {
   DCHECK(addresses);
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(false, callback.is_null());
 
   // Make a log item for the request.
   BoundNetLog request_net_log = BoundNetLog::Make(net_log_,
       NetLog::SOURCE_HOST_RESOLVER_IMPL_REQUEST);
 
-  // Update the net log and notify registered observers.
-  OnStartRequest(source_net_log, request_net_log, info);
+  LogStartRequest(source_net_log, request_net_log, info);
 
   // Build a key that identifies the request in the cache and in the
   // outstanding jobs map.
   Key key = GetEffectiveKeyForRequest(info);
 
   int rv = ResolveHelper(key, info, addresses, request_net_log);
   if (rv != ERR_DNS_CACHE_MISS) {
-    OnFinishRequest(source_net_log, request_net_log, info,
-                    rv,
-                    0  /* os_error (unknown since from cache) */);
+    LogFinishRequest(source_net_log, request_net_log, info, rv,
+                     0  /* os_error (unknown since from cache) */);
     return rv;
   }
 
-  // Create a handle for this request, and pass it back to the user if they
-  // asked for it (out_req != NULL).
-  Request* req = new Request(source_net_log, request_net_log, info,
-                             callback, addresses);
+  // 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, info.priority(), request_net_log);
+    job->handle() = dispatch_.Add(job, job->priority());
+    int net_error = job->net_error();
+    if (net_error != ERR_IO_PENDING) {
+      // Early finish caused by eviction.
+      DCHECK(job->was_evicted());
+      LogFinishRequest(source_net_log, request_net_log, info, net_error, 0);
+      delete job;
+      return net_error;
+    }
+    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->handle() = dispatch_.Update(job->handle(), job->priority());
   if (out_req)
     *out_req = reinterpret_cast<RequestHandle>(req);
 
-  // Next we need to attach our request to a "job". This job is responsible for
-  // calling "getaddrinfo(hostname)" on a worker thread.
-  scoped_refptr<Job> job;
-
-  // If there is already an outstanding job to resolve |key|, use
-  // it. This prevents starting concurrent resolves for the same hostname.
-  job = FindOutstandingJob(key);
-  if (job) {
-    job->AddRequest(req);
-  } else {
-    JobPool* pool = GetPoolForRequest(req);
-    if (CanCreateJobForPool(*pool)) {
-      CreateAndStartJob(req);
-    } else {
-      return EnqueueRequest(pool, req);
-    }
-  }
-
-  // Completion happens during OnJobComplete(Job*).
+  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,
                                     AddressList* addresses,
                                     const BoundNetLog& request_net_log) {
   // The result of |getaddrinfo| for empty hosts is inconsistent across systems.
   // On Windows it gives the default interface's address, whereas on Linux it
   // gives an error. We will make it fail on all platforms for consistency.
@@ skipping 12 matching lines @@
                                        AddressList* addresses,
                                        const BoundNetLog& source_net_log) {
   DCHECK(CalledOnValidThread());
   DCHECK(addresses);
 
   // Make a log item for the request.
   BoundNetLog request_net_log = BoundNetLog::Make(net_log_,
       NetLog::SOURCE_HOST_RESOLVER_IMPL_REQUEST);
 
   // Update the net log and notify registered observers.
-  OnStartRequest(source_net_log, request_net_log, info);
+  LogStartRequest(source_net_log, request_net_log, info);
 
-  // Build a key that identifies the request in the cache and in the
-  // outstanding jobs map.
   Key key = GetEffectiveKeyForRequest(info);
 
   int rv = ResolveHelper(key, info, addresses, request_net_log);
-  OnFinishRequest(source_net_log, request_net_log, info,
-                  rv,
-                  0  /* os_error (unknown since from cache) */);
+  LogFinishRequest(source_net_log, request_net_log, info, rv,
+                   0  /* os_error (unknown since from cache) */);
   return rv;
 }
 
-// See OnJobComplete(Job*) for why it is important not to clean out
-// cancelled requests from Job::requests_.
 void HostResolverImpl::CancelRequest(RequestHandle req_handle) {
   DCHECK(CalledOnValidThread());
   Request* req = reinterpret_cast<Request*>(req_handle);
   DCHECK(req);
 
-  scoped_ptr<Request> request_deleter;  // Frees at end of function.
-
-  if (!req->job()) {
-    // If the request was not attached to a job yet, it must have been
-    // enqueued into a pool. Remove it from that pool's queue.
-    // Otherwise if it was attached to a job, the job is responsible for
-    // deleting it.
-    JobPool* pool = GetPoolForRequest(req);
-    pool->RemovePendingRequest(req);
-    request_deleter.reset(req);
+  Job* job = req->job();
+  DCHECK(job);
+  job->CancelRequest(req);
+  if (job->num_active_requests()) {
+    if (!job->handle().is_null())
+      job->handle() = dispatch_.Update(job->handle(), job->priority());
   } else {
-    req->request_net_log().EndEvent(
-        NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_ATTACH, NULL);
+    // Cancel |job|.
+    PriorityDispatch::Handle handle = job->handle();
+    if (RemoveJob(job))
+      delete job;
+    if (!handle.is_null()) {
+      dispatch_.Cancel(handle);
+    } else {
+      dispatch_.OnJobFinished();
+    }
   }
-
-  // NULL out the fields of req, to mark it as cancelled.
-  req->MarkAsCancelled();
-  OnCancelRequest(req->source_net_log(), req->request_net_log(), req->info());
 }
 
 void HostResolverImpl::SetDefaultAddressFamily(AddressFamily address_family) {
   DCHECK(CalledOnValidThread());
   ipv6_probe_monitoring_ = false;
   DiscardIPv6ProbeJob();
   default_address_family_ = address_family;
 }
 
 AddressFamily HostResolverImpl::GetDefaultAddressFamily() const {
@@ skipping 18 matching lines @@
   DCHECK(addresses);
   DCHECK(net_error);
   IPAddressNumber ip_number;
   if (!ParseIPLiteralToNumber(key.hostname, &ip_number))
     return false;
 
   DCHECK_EQ(key.host_resolver_flags &
       ~(HOST_RESOLVER_CANONNAME | HOST_RESOLVER_LOOPBACK_ONLY |
         HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6),
             0) << " Unhandled flag";
-  bool ipv6_disabled = default_address_family_ == ADDRESS_FAMILY_IPV4 &&
-    !ipv6_probe_monitoring_;
+  bool ipv6_disabled = (default_address_family_ == ADDRESS_FAMILY_IPV4) &&
+      !ipv6_probe_monitoring_;
   *net_error = OK;
-  if (ip_number.size() == 16 && ipv6_disabled) {
+  if ((ip_number.size() == kIPv6AddressSize) && ipv6_disabled) {
     *net_error = ERR_NAME_NOT_RESOLVED;
   } else {
     *addresses = AddressList::CreateFromIPAddressWithCname(
         ip_number, info.port(),
         (key.host_resolver_flags & HOST_RESOLVER_CANONNAME));
   }
   return true;
 }
 
 bool HostResolverImpl::ServeFromCache(const Key& key,
@@ skipping 11 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::AddOutstandingJob(Job* job) {
-  scoped_refptr<Job>& found_job = jobs_[job->key()];
-  DCHECK(!found_job);
-  found_job = job;
-
-  JobPool* pool = GetPoolForRequest(job->initial_request());
-  pool->AdjustNumOutstandingJobs(1);
+void HostResolverImpl::OnJobFinished(Job* job, const AddressList& addrlist) {
+  DCHECK(job);
+  // Signal dispatcher that a slot has opened.
+  dispatch_.OnJobFinished();
+  // Write result to the cache.
+  if (cache_.get())
+    cache_->Set(job->key(), job->net_error(), addrlist,
+                base::TimeTicks::Now());
 }
 
-HostResolverImpl::Job* HostResolverImpl::FindOutstandingJob(const Key& key) {
-  JobMap::iterator it = jobs_.find(key);
-  if (it != jobs_.end())
-    return it->second;
-  return NULL;
+bool HostResolverImpl::RemoveJob(Job* job) {
+  DCHECK(job);
+  return jobs_.erase(job->key()) > 0;
 }
 
-void HostResolverImpl::RemoveOutstandingJob(Job* job) {
-  JobMap::iterator it = jobs_.find(job->key());
-  DCHECK(it != jobs_.end());
-  DCHECK_EQ(it->second.get(), job);
-  jobs_.erase(it);
-
-  JobPool* pool = GetPoolForRequest(job->initial_request());
-  pool->AdjustNumOutstandingJobs(-1);
-}
-
-void HostResolverImpl::OnJobComplete(Job* job,
-                                     int net_error,
-                                     int os_error,
-                                     const AddressList& addrlist) {
-  RemoveOutstandingJob(job);
-
-  // Write result to the cache.
-  if (cache_.get())
-    cache_->Set(job->key(), net_error, addrlist, base::TimeTicks::Now());
-
-  OnJobCompleteInternal(job, net_error, os_error, addrlist);
-}
-
-void HostResolverImpl::AbortJob(Job* job) {
-  OnJobCompleteInternal(job, ERR_ABORTED, 0 /* no os_error */, AddressList());
-}
-
-void HostResolverImpl::OnJobCompleteInternal(
-    Job* job,
-    int net_error,
-    int os_error,
-    const AddressList& addrlist) {
-  // Make a note that we are executing within OnJobComplete() in case the
-  // HostResolver is deleted by a callback invocation.
-  DCHECK(!cur_completing_job_);
-  cur_completing_job_ = job;
-
-  // Try to start any queued requests now that a job-slot has freed up.
-  ProcessQueuedRequests();
-
-  // Complete all of the requests that were attached to the job.
-  for (RequestsList::const_iterator it = job->requests().begin();
-       it != job->requests().end(); ++it) {
-    Request* req = *it;
-    if (!req->was_cancelled()) {
-      DCHECK_EQ(job, req->job());
-      req->request_net_log().EndEvent(
-          NetLog::TYPE_HOST_RESOLVER_IMPL_JOB_ATTACH, NULL);
-
-      // Update the net log and notify registered observers.
-      OnFinishRequest(req->source_net_log(), req->request_net_log(),
-                      req->info(), net_error, os_error);
-
-      req->OnComplete(net_error, addrlist);
-
-      // Check if the job was cancelled as a result of running the callback.
-      // (Meaning that |this| was deleted).
-      if (job->was_cancelled())
-        return;
-    }
-  }
-
-  cur_completing_job_ = NULL;
-}
-
-void HostResolverImpl::OnStartRequest(const BoundNetLog& source_net_log,
-                                      const BoundNetLog& request_net_log,
-                                      const RequestInfo& info) {
+// static
+void HostResolverImpl::LogStartRequest(const BoundNetLog& source_net_log,
+                                       const BoundNetLog& request_net_log,
+                                       const RequestInfo& info) {
   source_net_log.BeginEvent(
       NetLog::TYPE_HOST_RESOLVER_IMPL,
       make_scoped_refptr(new NetLogSourceParameter(
           "source_dependency", request_net_log.source())));
 
   request_net_log.BeginEvent(
       NetLog::TYPE_HOST_RESOLVER_IMPL_REQUEST,
       make_scoped_refptr(new RequestInfoParameters(
           info, source_net_log.source())));
 }
 
-void HostResolverImpl::OnFinishRequest(const BoundNetLog& source_net_log,
-                                       const BoundNetLog& request_net_log,
-                                       const RequestInfo& info,
-                                       int net_error,
-                                       int os_error) {
-  bool was_resolved = net_error == OK;
-
-  // Log some extra parameters on failure for synchronous requests.
+// static
+void HostResolverImpl::LogFinishRequest(const BoundNetLog& source_net_log,
+                                        const BoundNetLog& request_net_log,
+                                        const RequestInfo& info,
+                                        int net_error,
+                                        int os_error) {
   scoped_refptr<NetLog::EventParameters> params;
-  if (!was_resolved) {
+  if (net_error != OK) {
     params = new HostResolveFailedParams(0, net_error, os_error);
   }
 
   request_net_log.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_REQUEST, params);
   source_net_log.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL, NULL);
 }
 
-void HostResolverImpl::OnCancelRequest(const BoundNetLog& source_net_log,
-                                       const BoundNetLog& request_net_log,
-                                       const RequestInfo& info) {
+// static
+void HostResolverImpl::LogCancelRequest(const BoundNetLog& source_net_log,
+                                        const BoundNetLog& request_net_log,
+                                        const RequestInfo& info) {
   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);
 }
 
 void HostResolverImpl::DiscardIPv6ProbeJob() {
   if (ipv6_probe_job_.get()) {
     ipv6_probe_job_->Cancel();
     ipv6_probe_job_ = NULL;
   }
 }
 
 void HostResolverImpl::IPv6ProbeSetDefaultAddressFamily(
     AddressFamily address_family) {
   DCHECK(address_family == ADDRESS_FAMILY_UNSPECIFIED ||
          address_family == ADDRESS_FAMILY_IPV4);
   if (default_address_family_ != address_family) {
     VLOG(1) << "IPv6Probe forced AddressFamily setting to "
             << ((address_family == ADDRESS_FAMILY_UNSPECIFIED) ?
                 "ADDRESS_FAMILY_UNSPECIFIED" : "ADDRESS_FAMILY_IPV4");
   }
   default_address_family_ = address_family;
   // Drop reference since the job has called us back.
   DiscardIPv6ProbeJob();
 }
 
-bool HostResolverImpl::CanCreateJobForPool(const JobPool& pool) const {
-  DCHECK_LE(jobs_.size(), max_jobs_);
-
-  // We can't create another job if it would exceed the global total.
-  if (jobs_.size() + 1 > max_jobs_)
-    return false;
-
-  // Check whether the pool's constraints are met.
-  return pool.CanCreateJob();
-}
-
-// static
-HostResolverImpl::JobPoolIndex HostResolverImpl::GetJobPoolIndexForRequest(
-    const Request* req) {
-  return POOL_NORMAL;
-}
-
-void HostResolverImpl::ProcessQueuedRequests() {
-  // Find the highest priority request that can be scheduled.
-  Request* top_req = NULL;
-  for (size_t i = 0; i < arraysize(job_pools_); ++i) {
-    JobPool* pool = job_pools_[i];
-    if (pool->HasPendingRequests() && CanCreateJobForPool(*pool)) {
-      top_req = pool->RemoveTopPendingRequest();
-      break;
-    }
-  }
-
-  if (!top_req)
-    return;
-
-  scoped_refptr<Job> job(CreateAndStartJob(top_req));
-
-  // Search for any other pending request which can piggy-back off this job.
-  for (size_t pool_i = 0; pool_i < POOL_COUNT; ++pool_i) {
-    JobPool* pool = job_pools_[pool_i];
-    pool->MoveRequestsToJob(job);
-  }
-}
-
 HostResolverImpl::Key HostResolverImpl::GetEffectiveKeyForRequest(
     const RequestInfo& info) const {
   HostResolverFlags effective_flags =
       info.host_resolver_flags() | additional_resolver_flags_;
   AddressFamily effective_address_family = info.address_family();
   if (effective_address_family == ADDRESS_FAMILY_UNSPECIFIED &&
       default_address_family_ != ADDRESS_FAMILY_UNSPECIFIED) {
     effective_address_family = default_address_family_;
     if (ipv6_probe_monitoring_)
       effective_flags |= HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6;
   }
   return Key(info.hostname(), effective_address_family, effective_flags);
 }
 
-HostResolverImpl::Job* HostResolverImpl::CreateAndStartJob(Request* req) {
-  DCHECK(CanCreateJobForPool(*GetPoolForRequest(req)));
-  Key key = GetEffectiveKeyForRequest(req->info());
-
-  req->request_net_log().AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_CREATE_JOB,
-                                  NULL);
-
-  scoped_refptr<Job> job(new Job(next_job_id_++, this, key,
-                                 req->request_net_log(), net_log_));
-  job->AddRequest(req);
-  AddOutstandingJob(job);
-  job->Start();
-
-  return job.get();
-}
-
-int HostResolverImpl::EnqueueRequest(JobPool* pool, Request* req) {
-  scoped_ptr<Request> req_evicted_from_queue(
-      pool->InsertPendingRequest(req));
-
-  // If the queue has become too large, we need to kick something out.
-  if (req_evicted_from_queue.get()) {
-    Request* r = req_evicted_from_queue.get();
-    int error = ERR_HOST_RESOLVER_QUEUE_TOO_LARGE;
-
-    OnFinishRequest(r->source_net_log(), r->request_net_log(), r->info(), error,
-                    0  /* os_error (not applicable) */);
-
-    if (r == req)
-      return error;
-
-    r->OnComplete(error, AddressList());
-  }
-
-  return ERR_IO_PENDING;
-}
-
 void HostResolverImpl::CancelAllJobs() {
-  JobMap jobs;
-  jobs.swap(jobs_);
-  for (JobMap::iterator it = jobs.begin(); it != jobs.end(); ++it)
-    it->second->Cancel();
+  STLDeleteValues(&jobs_);
 }
 
 void HostResolverImpl::AbortAllInProgressJobs() {
-  for (size_t i = 0; i < arraysize(job_pools_); ++i)
-    job_pools_[i]->ResetNumOutstandingJobs();
-  JobMap jobs;
-  jobs.swap(jobs_);
-  for (JobMap::iterator it = jobs.begin(); it != jobs.end(); ++it) {
-    AbortJob(it->second);
-    it->second->Cancel();
+  base::WeakPtr<HostResolverImpl> self = AsWeakPtr();
+  // Scan |jobs_| for running jobs and abort them.
+  for (JobMap::iterator it = jobs_.begin(); it != jobs_.end(); ) {

[mmenke, 2011/12/22 16:18:49]

Looks to me like some of this complexity could be avoided if you just deleted
jobs that haven't started yet here, too.  Then you could also make RemoveJob
delete the job being removed too, and reduce the complexity of CompleteRequests
as well.

[szym, 2011/12/22 16:56:57]

True, although this would change the current behavior which only aborts running
jobs and the pending requests stick around. Would that change be ok?

[mmenke, 2011/12/22 17:34:30]

Hmm...  Good point.  I'm really not fond of the current dependencies on how
cleanup in here and in CompleteRequests works.  I don't think changing behavior
would be a huge deal, since we're aborting 100 requests anyways.

However, we could keep the current behavior by incrementing |it| first, right? 
Then job->Abort could delete the job, too.

I'm open to other workarounds, just think it's best for code not to delete stuff
or not based on what's 2 functions up on the callstack, if we can avoid it.

[szym, 2011/12/28 01:24:10]

Done. Job::Abort deletes the job.

+    Job* job = it->second;
+    if (!job->is_running()) {
+      // Keep it in |dispatch_|.
+      DCHECK(!job->handle().is_null());
+      ++it;
+      continue;
+    }
+    job->Abort();
+    // Check if resolver was deleted in a request callback.
+    if (!self)
+      return;
+    delete job;
+    jobs_.erase(it++);
+    dispatch_.OnJobFinished();
   }
 }
 
 void HostResolverImpl::OnIPAddressChanged() {
   if (cache_.get())
     cache_->clear();
   if (ipv6_probe_monitoring_) {
     DiscardIPv6ProbeJob();
     ipv6_probe_job_ = new IPv6ProbeJob(this);
     ipv6_probe_job_->Start();
@@ skipping 16 matching lines @@
   // 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().
 }
 
 }  // namespace net