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

net/dns/dns_client.h

 // 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.
 
 #ifndef NET_DNS_DNS_CLIENT_H_
 #define NET_DNS_DNS_CLIENT_H_
 #pragma once
 
 #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.
+// 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.
+//
+// Destroying DnsTransaction cancels the underlying network effort.
+class NET_EXPORT_PRIVATE DnsTransaction {
+ public:
+  virtual ~DnsTransaction() {}
+
+  // 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;
+};
+
+// 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 Requests.
-//
-// 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)
+// 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:
-  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;
-
-    void DoCallback(int result, const DnsResponse* response) {
-      callback_.Run(this, result, response);
-    }
-
-   private:
-    std::string qname_;
-    uint16 qtype_;
-    RequestCallback callback_;
-
-    DISALLOW_COPY_AND_ASSIGN(Request);
-  };
+  // 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
 
 #endif  // NET_DNS_DNS_CLIENT_H_