Refactoring of job dispatch in HostResolverImpl in preparation for DnsClient.

net/base/host_resolver_impl.h

Index: net/base/host_resolver_impl.h
diff --git a/net/base/host_resolver_impl.h b/net/base/host_resolver_impl.h
index da694519fc1eaec22ffed3e3f349045de5ff7469..25e204465bde9df8f5b5c0bee27c3b4383585c0f 100644
--- a/net/base/host_resolver_impl.h
+++ b/net/base/host_resolver_impl.h
@@ -6,6 +6,7 @@
 #define NET_BASE_HOST_RESOLVER_IMPL_H_
 #pragma once
 
+#include <map>
 #include <vector>
 
 #include "base/basictypes.h"
@@ -20,15 +21,16 @@
 #include "net/base/net_export.h"
 #include "net/base/net_log.h"
 #include "net/base/network_change_notifier.h"
+#include "net/base/priority_dispatch.h"
 
 namespace net {
 
 // For each hostname that is requested, HostResolver creates a
-// HostResolverImpl::Job. This job gets dispatched to a thread in the global
-// WorkerPool, where it runs SystemHostResolverProc(). If requests for that same
-// host are made while the job is already outstanding, then they are attached
-// to the existing job rather than creating a new one. This avoids doing
-// parallel resolves for the same host.
+// HostResolverImpl::Job. When this job gets dispatched it creates a ProcJob
+// which runs the given HostResolverProc on a WorkerPool thread. If requests for
+// that same host are made during the jobs lifetime, they are attached to the

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

nit:  job's lifetime

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

Done.

+// existing job rather than creating a new one. This avoids doing parallel
+// resolves for the same host.
 //
 // The way these classes fit together is illustrated by:
 //
@@ -63,34 +65,15 @@ class NET_EXPORT HostResolverImpl
     : public HostResolver,
       NON_EXPORTED_BASE(public base::NonThreadSafe),
       public NetworkChangeNotifier::IPAddressObserver,
-      public NetworkChangeNotifier::DNSObserver {
+      public NetworkChangeNotifier::DNSObserver,
+      public base::SupportsWeakPtr<HostResolverImpl> {
  public:
-  // The index into |job_pools_| for the various job pools. Pools with a higher
-  // index have lower priority.
-  //
-  // Note: This is currently unused, since there is a single pool
-  //       for all requests.
-  enum JobPoolIndex {
-    POOL_NORMAL = 0,
-    POOL_COUNT,
-  };
-
-  // Creates a HostResolver that first uses the local cache |cache|, and then
-  // falls back to |resolver_proc|.
-  //
-  // If |cache| is NULL, then no caching is used. Otherwise we take
-  // ownership of the |cache| pointer, and will free it during destructor.
+  // Parameters for ProcJob which resolves hostnames using HostResolveProc.
   //
   // |resolver_proc| is used to perform the actual resolves; it must be
   // thread-safe since it is run from multiple worker threads. If
   // |resolver_proc| is NULL then the default host resolver procedure is
   // used (which is SystemHostResolverProc except if overridden).
-  // |max_jobs| specifies the maximum number of threads that the host resolver
-  // will use (not counting potential duplicate attempts). Use
-  // SetPoolConstraints() to specify finer-grain settings.
-  // |max_retry_attempts| is the maximum number of times we will retry for host
-  // resolution. Pass HostResolver::kDefaultRetryAttempts to choose a default
-  // value.
   //
   // For each attempt, we could start another attempt if host is not resolved
   // within unresponsive_delay_ time. We keep attempting to resolve the host

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

While you're here, could you surround the variable names in this paragraph with
"|"s?

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

Done.

@@ -99,11 +82,42 @@ class NET_EXPORT HostResolverImpl
   // multiplied by the retry factor each time). Once we have retried
   // max_retry_attempts, we give up on additional attempts.
   //
+  struct NET_EXPORT_PRIVATE ProcJobParams {
+    // Sets up defaults.
+    ProcJobParams(HostResolverProc* resolver_proc, size_t max_retry_attempts);
+
+    ~ProcJobParams();
+
+    // The procedure to use for resolving host names. This will be NULL, except
+    // in the case of unit-tests which inject custom host resolving behaviors.
+    scoped_refptr<HostResolverProc> resolver_proc_;

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

The style guide doesn't allow the terminating underscore in struct member names
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names)

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

Done.

+
+    // Maximum number retry attempts to resolve the hostname.
+    // Pass HostResolver::kDefaultRetryAttempts to choose a default
+    // value.
+    size_t max_retry_attempts_;
+
+    // This is the limit after which we make another attempt to resolve the host
+    // if the worker thread has not responded yet.
+    base::TimeDelta unresponsive_delay_;
+
+    // Factor to grow unresponsive_delay_ when we re-re-try.
+    uint32 retry_factor_;
+  };
+
+  // Creates a HostResolver that first uses the local cache |cache|, and then
+  // falls back to |proc_params.resolver_proc_|.
+  //
+  // If |cache| is NULL, then no caching is used. Otherwise we take
+  // ownership of the |cache| pointer, and will free it during destructor.

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

nit:  "during destruction" or "in the destructor"

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

Done.

+  //
+  // |job_limits| specifies the maximum number of threads that the host resolver
+  // will use (not counting potential duplicate attempts).

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

Should clarify it's the number of active jobs, rather than the number of queued
jobs, now that we immediately create jobs.

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

Done.

+  //
   // |net_log| must remain valid for the life of the HostResolverImpl.
-  HostResolverImpl(HostResolverProc* resolver_proc,
-                   HostCache* cache,
-                   size_t max_jobs,
-                   size_t max_retry_attempts,
+  HostResolverImpl(HostCache* cache,
+                   const PriorityDispatch::Limits& job_limits,
+                   const ProcJobParams& proc_params,
                    NetLog* net_log);
 
   // If any completion callbacks are pending when the resolver is destroyed,
@@ -111,20 +125,9 @@ class NET_EXPORT HostResolverImpl
   // be called.
   virtual ~HostResolverImpl();
 
-  // Applies a set of constraints for requests that belong to the specified
-  // pool. NOTE: Don't call this after requests have been already been started.
-  //
-  //  |pool_index| -- Specifies which pool these constraints should be applied
-  //                  to.
-  //  |max_outstanding_jobs| -- How many concurrent jobs are allowed for this
-  //                            pool.
-  //  |max_pending_requests| -- How many requests can be enqueued for this pool
-  //                            before we start dropping requests. Dropped
-  //                            requests fail with
-  //                            ERR_HOST_RESOLVER_QUEUE_TOO_LARGE.
-  void SetPoolConstraints(JobPoolIndex pool_index,
-                          size_t max_outstanding_jobs,
-                          size_t max_pending_requests);
+  // Configures maximum number of Jobs in the queue. Exposed for testing.
+  // Only allowed when the queue is empty.
+  void SetMaxQueuedJobs(size_t max_queued);

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

Since this is only used in one test, could just go ahead and make it private and
friend the test, unless you think we may eventually need it in tests outside of
net/

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

nit:  Also, think either |max| or |max_queued_jobs| would be better - variable
names should usually be nouns.

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

Done.

 
   // HostResolver methods:
   virtual int Resolve(const RequestInfo& info,
@@ -143,18 +146,15 @@ class NET_EXPORT HostResolverImpl
 
  private:
   // Allow tests to access our innards for testing purposes.
-  friend class LookupAttemptHostResolverProc;
-
-  // Allow tests to access our innards for testing purposes.
   FRIEND_TEST_ALL_PREFIXES(HostResolverImplTest, MultipleAttempts);
 
   class Job;
-  class JobPool;
+  class ProcJob;
   class IPv6ProbeJob;
   class Request;
-  typedef std::vector<Request*> RequestsList;
   typedef HostCache::Key Key;
-  typedef std::map<Key, scoped_refptr<Job> > JobMap;
+  typedef std::map<Key, Job*> JobMap;
+  typedef std::vector<Request*> RequestsList;
 
   // Helper used by |Resolve()| and |ResolveFromCache()|.  Performs IP
   // literal and cache lookup, returns OK if successful,
@@ -181,153 +181,69 @@ class NET_EXPORT HostResolverImpl
                       int* net_error,
                       AddressList* addresses);
 
-  // Returns the HostResolverProc to use for this instance.
-  HostResolverProc* effective_resolver_proc() const {
-    return resolver_proc_ ?
-        resolver_proc_.get() : HostResolverProc::GetDefault();
-  }
-
-  // Adds a job to outstanding jobs list.
-  void AddOutstandingJob(Job* job);
-
-  // Returns the outstanding job for |key|, or NULL if there is none.
-  Job* FindOutstandingJob(const Key& key);
-
-  // Removes |job| from the outstanding jobs list.
-  void RemoveOutstandingJob(Job* job);
+  // The logging routines are defined here because some requests are resolved
+  // without a Request object.
 
-  // Callback for when |job| has completed with |net_error| and |addrlist|.
-  void OnJobComplete(Job* job, int net_error, int os_error,
-                     const AddressList& addrlist);
+  // Logs when a request has just been started.
+  static void LogStartRequest(const BoundNetLog& source_net_log,
+                              const BoundNetLog& request_net_log,
+                              const RequestInfo& info);
 
-  // Aborts |job|.  Same as OnJobComplete() except does not remove |job|
-  // from |jobs_| and does not cache the result (ERR_ABORTED).
-  void AbortJob(Job* job);
+  // Logs when a request has just completed (before its callback is run).
+  static void LogFinishRequest(const BoundNetLog& source_net_log,
+                               const BoundNetLog& request_net_log,
+                               const RequestInfo& info,
+                               int net_error,
+                               int os_error);
 
-  // Used by both OnJobComplete() and AbortJob();
-  void OnJobCompleteInternal(Job* job, int net_error, int os_error,
-                             const AddressList& addrlist);
+  // Logs when a request has been cancelled.
+  static void LogCancelRequest(const BoundNetLog& source_net_log,
+                               const BoundNetLog& request_net_log,
+                               const RequestInfo& info);
 
-  // Called when a request has just been started.
-  void OnStartRequest(const BoundNetLog& source_net_log,
-                      const BoundNetLog& request_net_log,
-                      const RequestInfo& info);
-
-  // Called when a request has just completed (before its callback is run).
-  void OnFinishRequest(const BoundNetLog& source_net_log,
-                       const BoundNetLog& request_net_log,
-                       const RequestInfo& info,
-                       int net_error,
-                       int os_error);
-
-  // Called when a request has been cancelled.
-  void OnCancelRequest(const BoundNetLog& source_net_log,
-                       const BoundNetLog& request_net_log,
-                       const RequestInfo& info);
-
-  // Notify IPv6ProbeJob not to call back, and discard reference to the job.
+  // Notifies IPv6ProbeJob not to call back, and discard reference to the job.
   void DiscardIPv6ProbeJob();
 
   // Callback from IPv6 probe activity.
   void IPv6ProbeSetDefaultAddressFamily(AddressFamily address_family);
 
-  // Returns true if the constraints for |pool| are met, and a new job can be
-  // created for this pool.
-  bool CanCreateJobForPool(const JobPool& pool) const;
-
-  // Returns the index of the pool that request |req| maps to.
-  static JobPoolIndex GetJobPoolIndexForRequest(const Request* req);
-
-  JobPool* GetPoolForRequest(const Request* req) {
-    return job_pools_[GetJobPoolIndexForRequest(req)];
-  }
-
-  // Starts up to 1 job given the current pool constraints. This job
-  // may have multiple requests attached to it.
-  void ProcessQueuedRequests();
-
   // Returns the (hostname, address_family) key to use for |info|, choosing an
   // "effective" address family by inheriting the resolver's default address
   // family when the request leaves it unspecified.
   Key GetEffectiveKeyForRequest(const RequestInfo& info) const;
 
-  // Attaches |req| to a new job, and starts it. Returns that job.
-  Job* CreateAndStartJob(Request* req);
+  // Called by |job| when it has finished running with result.
+  void OnJobFinished(Job* job, const AddressList& addrlist);
 
-  // Adds a pending request |req| to |pool|.
-  int EnqueueRequest(JobPool* pool, Request* req);
+  // Called by |job| when it is ready to be destroyed. Returns true if |job| was
+  // removed from |jobs_|, which means the caller should delete it.
+  // Otherwise, the previous caller will delete it.

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

Previous caller?

[szym, 2011/12/21 22:19:34]

Whoever was the first to call RemoveJob and receive true.

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

This part of the logic removed.

+  bool RemoveJob(Job* job);
 
-  // Cancels all jobs.
+  // Cancels all jobs. Drops all requests.
   void CancelAllJobs();

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

Mind renaming this to Shutdown() or ShutdownAllJobs()?  Think that makes the
behavior a little clearer than the current name.

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

Removed.

 
-  // Aborts all in progress jobs (but might start new ones).
+  // Aborts all in progress jobs and notifies their requests.
+  // Might start new jobs.
   void AbortAllInProgressJobs();
 
   // NetworkChangeNotifier::IPAddressObserver methods:
   virtual void OnIPAddressChanged() OVERRIDE;
 
-  // Helper methods to get and set max_retry_attempts_.
-  size_t max_retry_attempts() const {
-    return max_retry_attempts_;
-  }
-  void set_max_retry_attempts(const size_t max_retry_attempts) {
-    max_retry_attempts_ = max_retry_attempts;
-  }
-
-  // Helper methods for unit tests to get and set unresponsive_delay_.
-  base::TimeDelta unresponsive_delay() const { return unresponsive_delay_; }
-  void set_unresponsive_delay(const base::TimeDelta& unresponsive_delay) {
-    unresponsive_delay_ = unresponsive_delay;
-  }
-
-  // Helper methods to get and set retry_factor_.
-  uint32 retry_factor() const {
-    return retry_factor_;
-  }
-  void set_retry_factor(const uint32 retry_factor) {
-    retry_factor_ = retry_factor;
-  }
-
   // NetworkChangeNotifier::OnDNSChanged methods:
   virtual void OnDNSChanged() OVERRIDE;
 
   // Cache of host resolution results.
   scoped_ptr<HostCache> cache_;
 
-  // Map from hostname to outstanding job.
+  // Map from cache key to outstanding job.
   JobMap jobs_;
 
-  // Maximum number of concurrent jobs allowed, across all pools. Each job may
-  // create multiple concurrent resolve attempts for the hostname.
-  size_t max_jobs_;
-
-  // Maximum number retry attempts to resolve the hostname.
-  size_t max_retry_attempts_;
-
-  // This is the limit after which we make another attempt to resolve the host
-  // if the worker thread has not responded yet. Allow unit tests to change the
-  // value.
-  base::TimeDelta unresponsive_delay_;
-
-  // Factor to grow unresponsive_delay_ when we re-re-try. Allow unit tests to
-  // change the value.
-  uint32 retry_factor_;
-
-  // The information to track pending requests for a JobPool, as well as
-  // how many outstanding jobs the pool already has, and its constraints.
-  JobPool* job_pools_[POOL_COUNT];
-
-  // The job that OnJobComplete() is currently processing (needed in case
-  // HostResolver gets deleted from within the callback).
-  scoped_refptr<Job> cur_completing_job_;
-
-  // Monotonically increasing ID number to assign to the next job.
-  // The only consumer of this ID is the requests tracing code.
-  int next_job_id_;
+  // Starts Jobs according to their priority and the configured limits.
+  PriorityDispatch dispatch_;
 
-  // The procedure to use for resolving host names. This will be NULL, except
-  // in the case of unit-tests which inject custom host resolving behaviors.
-  scoped_refptr<HostResolverProc> resolver_proc_;
+  // Parameters for ProcJob.
+  ProcJobParams proc_params_;
 
   // Address family to use when the request doesn't specify one.
   AddressFamily default_address_family_;