DnsClient refactoring + features (timeout, suffix search, server rotation).

net/dns/dns_client.h

Index: net/dns/dns_client.h
diff --git a/net/dns/dns_client.h b/net/dns/dns_client.h
index b2e3c0ac74be0068bb3f8d4758f7473d2d6cdd0e..21086f452e8cddaf3cd29d48c025176313b41773 100644
--- a/net/dns/dns_client.h
+++ b/net/dns/dns_client.h
@@ -9,79 +9,58 @@
 #include <string>
 
 #include "base/basictypes.h"
-#include "base/callback.h"
-#include "base/memory/weak_ptr.h"
-#include "base/string_piece.h"
+#include "base/callback_forward.h"
+#include "base/memory/scoped_ptr.h"
 #include "net/base/net_export.h"
 
 namespace net {
 
 class BoundNetLog;
-class ClientSocketFactory;
 class DnsResponse;
 class DnsSession;
 
-// DnsClient performs asynchronous DNS queries. DnsClient takes care of
-// retransmissions, DNS server fallback (or round-robin), suffix search, and
-// simple response validation ("does it match the query") to fight poisoning.
-// It does NOT perform caching, aggregation or prioritization of requests.
-//
-// Destroying DnsClient does NOT affect any already created Requests.
+// The DnsTransaction takes care of retransmissions, DNS server fallback (or
+// round-robin), suffix search, and simple response validation ("does it match
+// the query") to fight poisoning.
 //
-// TODO(szym): consider adding flags to MakeRequest to indicate options:
-// -- don't perform suffix search
-// -- query both A and AAAA at once
-// -- accept truncated response (and/or forbid TCP)
-class NET_EXPORT_PRIVATE DnsClient {
+// Destroying DnsTransaction cancels the underlying network effort.
+class NET_EXPORT_PRIVATE DnsTransaction {
  public:
-  class Request;
-  // Callback for complete requests. Note that DnsResponse might be NULL if
-  // the DNS server(s) could not be reached.
-  typedef base::Callback<void(Request* req,
-                              int result,
-                              const DnsResponse* resp)> RequestCallback;
-
-  // A data-holder for a request made to the DnsClient.
-  // Destroying the request cancels the underlying network effort.
-  class NET_EXPORT_PRIVATE Request {
-   public:
-    Request(const base::StringPiece& qname,
-            uint16 qtype,
-            const RequestCallback& callback);
-    virtual ~Request();
-
-    const std::string& qname() const { return qname_; }
-
-    uint16 qtype() const { return qtype_; }
-
-    virtual int Start() = 0;
+  virtual ~DnsTransaction() {}
 
-    void DoCallback(int result, const DnsResponse* response) {
-      callback_.Run(this, result, response);
-    }
-
-   private:
-    std::string qname_;
-    uint16 qtype_;
-    RequestCallback callback_;
+  // Returns the original |hostname|.
+  virtual const std::string& GetHostname() const = 0;
+  // Returns the |qtype|.

[cbentzel, 2012/01/13 13:39:54]

Nit: extra newline before this comment.

+  virtual uint16 GetType() const = 0;
+};
 
-    DISALLOW_COPY_AND_ASSIGN(Request);
-  };
+// Creates DnsTransaction which performs asynchronous DNS search.
+// It does NOT perform caching, aggregation or prioritization of requests.
+//
+// Destroying DnsClient does NOT affect any already created DnsTransactions.
+class NET_EXPORT_PRIVATE DnsClient {

[cbentzel, 2012/01/13 13:39:54]

I wonder if this should just be called DnsTransactionFactory

+ public:
+  // Called with the response or NULL if no matching response was received.
+  // Note that the |qname| of the response may be different than |hostname|
+  // as a result of suffix search.
+  typedef base::Callback<void(DnsTransaction* transaction,
+                              int neterror,
+                              const DnsResponse* response)> CallbackType;
 
   virtual ~DnsClient() {}
 
-  // Makes asynchronous DNS query for the given |qname| and |qtype| (assuming
-  // QCLASS == IN). The caller is responsible for destroying the returned
-  // request whether to cancel it or after its completion.
-  // (Destroying DnsClient does not abort the requests.)
-  virtual Request* CreateRequest(
-      const base::StringPiece& qname,
+  // Creates DnsTransaction for the given |hostname| and |qtype| (assuming
+  // QCLASS is IN). The transaction will run |callback| upon asynchronous
+  // completion. Uses the source of |source_net_log| as source dependency.

[cbentzel, 2012/01/13 13:39:54]

It would be worth pointing out that |hostname| must not be an IP literal - or
fixing DnsTransactionImpl to work correctly with IP literals.

For example, on an IPv6 literal it will fail early since DNSDomainFromDot will
fail. For IPv4, it may run into issues since DNSDomainFromDot will work, and
then suffixes may be appended to it particularly if ndots are set to a very high
value.

If you don't want DNSTransactionImpl (and this) to work with IP literals, then
please add some DCHECKs. 

[szym, 2012/01/13 15:43:40]

I'll add DCHECKs. For requests that can complete synchronously (IP literals,
HOSTS, cache), there is no need to create a HostResolverImpl::Job.

+  virtual scoped_ptr<DnsTransaction> CreateTransaction(
+      const std::string& hostname,
       uint16 qtype,
-      const RequestCallback& callback,
+      const CallbackType& callback,
       const BoundNetLog& source_net_log) WARN_UNUSED_RESULT = 0;
 
   // Creates a socket-based DnsClient using the |session|.
-  static DnsClient* CreateClient(DnsSession* session) WARN_UNUSED_RESULT;
+  static scoped_ptr<DnsClient> CreateClient(
+      DnsSession* session) WARN_UNUSED_RESULT;
 };
 
 }  // namespace net