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..6bc61b5665e2f114df46c5895c4c7e49b3f11c4e 100644
--- a/net/dns/dns_client.h
+++ b/net/dns/dns_client.h
@@ -2,89 +2,80 @@
 // 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_
+
+#ifndef NET_DNS_DNS_TRANSACTION_H_

[cbentzel, 2012/01/13 22:17:25]

This needs to move file names for these guards to change. 

+#define NET_DNS_DNS_TRANSACTION_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.
-// It does NOT perform caching, aggregation or prioritization of requests.
+// DnsTransaction implements a stub DNS resolver as defined in RFC 1034.
+// The DnsTransaction takes care of retransmissions, name server fallback (or
+// round-robin), suffix search, and simple response validation ("does it match
+// the query") to fight poisoning.

[cbentzel, 2012/01/13 22:17:25]

I'd move the "It does not do caching" comment up here, instead of from the
factory.

 //
-// Destroying DnsClient does NOT affect any already created Requests.
+// 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|.
+  virtual uint16 GetType() const = 0;
+
+  // Starts the transaction. Returns the net error on synchronous failure or
+  // ERR_IO_PENDING in which case the result will be passed via the callback.

[cbentzel, 2012/01/13 22:17:25]

"The callback" isn't really defined as part of the interface. 

Should the callback be passed into start instead of CreateTransaction?

[szym, 2012/01/13 22:32:44]

I'm fine either way. The benefit of having it in the constructor is that we
don't need to worry about initialization.

A potential benefit of having it in Start() is that then CallbackType could be
Callback<void(int, const DnsResponse*)>. If any user really needed the
DnsTransaction* they could just curry it, albeit this costs a little bit of
memory for storage. I don't actually expect to need the pointer in
HostResolverImpl, since the callback will already be bound to a specific Job or
a DnsTask.

+  virtual int Start() = 0;
+};
+
+// Creates DnsTransaction which performs asynchronous DNS search.
+// It does NOT perform caching, aggregation or prioritization of transactions.
 //
-// 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 the factory does NOT affect any already created DnsTransactions.
+class NET_EXPORT_PRIVATE 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);
-  };
-
-  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,
+  // Called with the response or NULL if no matching response was received.
+  // Note that the |GetDottedName()| of the response may be different than the
+  // original |hostname| as a result of suffix search.
+  typedef base::Callback<void(DnsTransaction* transaction,
+                              int neterror,
+                              const DnsResponse* response)> CallbackType;
+
+  virtual ~DnsTransactionFactory() {}
+
+  // Creates DnsTransaction for the given |hostname| and |qtype| (assuming
+  // QCLASS is IN). |hostname| should be in the dotted form. A dot at the end
+  // implies the domain name is fully-qualified and will be exempt from suffix
+  // search. |hostname| should not be an IP literal.
+  //
+  // The transaction will run |callback| upon asynchronous completion.
+  // The source of |source_net_log| is used as source dependency in log.
+  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;
+  // Creates a DnsTransactionFactory which creates DnsTransactionImpl using the
+  // |session|.
+  static scoped_ptr<DnsTransactionFactory> CreateFactory(
+      DnsSession* session) WARN_UNUSED_RESULT;
 };
 
 }  // namespace net
 
-#endif  // NET_DNS_DNS_CLIENT_H_
+#endif  // NET_DNS_DNS_TRANSACTION_H_