Refactoring of job dispatch in HostResolverImpl in preparation for DnsClient.

net/base/host_resolver_impl.cc

Index: net/base/host_resolver_impl.cc
diff --git a/net/base/host_resolver_impl.cc b/net/base/host_resolver_impl.cc
index 217f1e1329e678e0500a337fbc45cf9a33e165e1..d986b79309064da249a3134a97b2bc9699a78702 100644
--- a/net/base/host_resolver_impl.cc
+++ b/net/base/host_resolver_impl.cc
@@ -10,16 +10,18 @@
 #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"
@@ -141,9 +143,14 @@ HostResolver* CreateSystemHostResolver(size_t max_concurrent_resolves,
   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;
 }
@@ -275,28 +282,30 @@ class HostResolverImpl::Request {
         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);
   }
 
@@ -308,10 +317,12 @@ class HostResolverImpl::Request {
     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_;
   }
@@ -327,7 +338,7 @@ class HostResolverImpl::Request {
   // 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.
@@ -346,56 +357,75 @@ class HostResolverImpl::Request {
 #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();
+
+    job_net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_JOB,
+                         new NetLogSourceParameter("source_dependency",
+                                                   net_log_.source()));
+
+    net_log_.BeginEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_JOB,
+                        new JobCreationParameters(key_.hostname,
+                                                  job_net_log.source()));
   }
 
-  // Attaches a request to this job. The job takes ownership of |req| and will
-  // take care to delete it.
-  void AddRequest(Request* req) {
+  void Start() {
     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())));
+    StartLookupAttempt();
+  }
 
-    req->set_job(this);
-    requests_.push_back(req);
+  // 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 (!req->info().is_speculative())
-      had_non_speculative_request_ = true;
+    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 Start() {
+  void set_had_non_speculative_request() {
     DCHECK(origin_loop_->BelongsToCurrentThread());
-    StartLookupAttempt();
+    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();
@@ -403,7 +433,7 @@ class HostResolverImpl::Job
     // 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();
 
@@ -412,7 +442,7 @@ class HostResolverImpl::Job
       // 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;
     }
@@ -426,84 +456,14 @@ class HostResolverImpl::Job
     // 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());
-    }
-  }
-
-  // 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);
+          base::Bind(&ProcJob::OnCheckForComplete, this),
+          params_.unresponsive_delay_.InMilliseconds());
     }
   }
 
-  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
@@ -514,7 +474,7 @@ class HostResolverImpl::Job
     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,
@@ -523,7 +483,7 @@ class HostResolverImpl::Job
 
     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));
   }
 
@@ -531,11 +491,10 @@ class HostResolverImpl::Job
   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();
   }
 
@@ -550,7 +509,7 @@ class HostResolverImpl::Job
 
     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);
@@ -591,7 +550,7 @@ class HostResolverImpl::Job
 
     RecordAttemptHistograms(start_time, attempt_number, error, os_error);
 
-    if (was_cancelled())
+    if (was_canceled())
       return;
 
     if (was_retry_attempt) {
@@ -607,17 +566,11 @@ class HostResolverImpl::Job
       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);
-
-    DCHECK(!requests_.empty());
+    net_log_.EndEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_PROC_JOB, params);
 
-     // 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,
@@ -713,6 +666,7 @@ class HostResolverImpl::Job
                                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);
@@ -736,19 +690,19 @@ class HostResolverImpl::Job
 
     // 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);
     }
 
@@ -759,28 +713,20 @@ class HostResolverImpl::Job
       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.
-
-  // 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.
+  // 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.
-  scoped_refptr<HostResolverProc> resolver_proc_;
+  ProcJobParams params_;
 
-  // The amount of time after starting a resolution attempt until deciding to
-  // retry.
-  base::TimeDelta unresponsive_delay_;
+  // The listener to the results of this ProcJob.
+  Callback callback_;
+
+  // Used to post ourselves onto the origin thread.
+  scoped_refptr<base::MessageLoopProxy> origin_loop_;
 
   // 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
@@ -798,7 +744,7 @@ class HostResolverImpl::Job
   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_;
@@ -807,13 +753,12 @@ class HostResolverImpl::Job
 
   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:
@@ -825,7 +770,7 @@ class HostResolverImpl::IPv6ProbeJob
 
   void Start() {
     DCHECK(origin_loop_->BelongsToCurrentThread());
-    if (was_cancelled())
+    if (was_canceled())
       return;
     const bool kIsSlow = true;
     base::WorkerPool::PostTask(
@@ -835,7 +780,7 @@ class HostResolverImpl::IPv6ProbeJob
   // 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.
   }
@@ -846,7 +791,7 @@ class HostResolverImpl::IPv6ProbeJob
   ~IPv6ProbeJob() {
   }
 
-  bool was_cancelled() const {
+  bool was_canceled() const {
     DCHECK(origin_loop_->BelongsToCurrentThread());
     return !resolver_;
   }
@@ -865,7 +810,7 @@ class HostResolverImpl::IPv6ProbeJob
   // 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);
   }
@@ -881,199 +826,355 @@ class HostResolverImpl::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;
         }
       }
+      DCHECK_EQ(0u, total_count_);
+      // In absence of requests set default.
+      if (highest_priority_ != IDLE) {
+        highest_priority_ = IDLE;
+        return true;
+      }
     }
+    return false;
+  }
 
-    return NULL;
+ 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())));
   }
 
-  // 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);
+  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_);
   }
 
-  // Removes and returns the highest priority pending request.
-  Request* RemoveTopPendingRequest() {
-    DCHECK(HasPendingRequests());
+  HostResolverImpl* resolver() const {
+    return resolver_;
+  }
 
-    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;
-      }
+  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())));
     }
 
-    NOTREACHED();
-    return NULL;
+    // 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())));
+    }
   }
 
-  // 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;
+  // 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());
+    }
   }
 
-  void ResetNumOutstandingJobs() {
-    num_outstanding_jobs_ = 0;
+  // 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);
   }
 
-  // Returns true if a new job can be created for this pool.
-  bool CanCreateJob() const {
-    return num_outstanding_jobs_ + 1u <= max_outstanding_jobs_;
+  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());
   }
 
-  // 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;
-        }
+  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;
+  // 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.
+    }
 
-  // Maximum number of concurrent jobs allowed to be started for requests
-  // belonging to this pool.
-  size_t max_outstanding_jobs_;
+    // 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;
 
-  // The current number of running jobs that were started for requests
-  // belonging to this pool.
-  size_t num_outstanding_jobs_;
+      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_);
 
-  // The maximum number of requests we allow to be waiting on a job,
-  // for this pool.
-  size_t max_pending_requests_;
+      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;
+    }
+  }
 
-  // The requests which are waiting to be started for this pool.
-  PendingRequestsQueue pending_requests_[NUM_PRIORITIES];
+  // 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::HostResolverImpl(
+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(
     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();
@@ -1093,34 +1194,19 @@ HostResolverImpl::HostResolverImpl(
 
 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,
@@ -1136,8 +1222,7 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
   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.
@@ -1145,38 +1230,44 @@ int HostResolverImpl::Resolve(const RequestInfo& 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);
-  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);
+  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;
   }
 
-  // Completion happens during OnJobComplete(Job*).
+  // 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);
+
+  DCHECK_EQ(ERR_IO_PENDING, job->net_error());
+  // Completion happens during Job::CompleteRequests().
   return ERR_IO_PENDING;
 }
 
@@ -1209,44 +1300,38 @@ int HostResolverImpl::ResolveFromCache(const RequestInfo& info,
       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) {
@@ -1285,10 +1370,10 @@ bool HostResolverImpl::ResolveAsIP(const Key& key,
       ~(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(
@@ -1320,90 +1405,25 @@ bool HostResolverImpl::ServeFromCache(const Key& key,
   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);
-}
-
-HostResolverImpl::Job* HostResolverImpl::FindOutstandingJob(const Key& key) {
-  JobMap::iterator it = jobs_.find(key);
-  if (it != jobs_.end())
-    return it->second;
-  return NULL;
-}
-
-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);
-
+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(), net_error, addrlist, base::TimeTicks::Now());
-
-  OnJobCompleteInternal(job, net_error, os_error, addrlist);
+    cache_->Set(job->key(), job->net_error(), addrlist,
+                base::TimeTicks::Now());
 }
 
-void HostResolverImpl::AbortJob(Job* job) {
-  OnJobCompleteInternal(job, ERR_ABORTED, 0 /* no os_error */, AddressList());
+bool HostResolverImpl::RemoveJob(Job* job) {
+  DCHECK(job);
+  return jobs_.erase(job->key()) > 0;
 }
 
-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(
@@ -1415,16 +1435,14 @@ void HostResolverImpl::OnStartRequest(const BoundNetLog& source_net_log,
           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);
   }
 
@@ -1432,9 +1450,10 @@ void HostResolverImpl::OnFinishRequest(const BoundNetLog& source_net_log,
   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);
@@ -1461,46 +1480,6 @@ void HostResolverImpl::IPv6ProbeSetDefaultAddressFamily(
   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 =
@@ -1515,58 +1494,28 @@ HostResolverImpl::Key HostResolverImpl::GetEffectiveKeyForRequest(
   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();
   }
 }