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

net/dns/dns_client.cc

Index: net/dns/dns_client.cc
diff --git a/net/dns/dns_client.cc b/net/dns/dns_client.cc
index de60cc33fee98c30b3c7844035b1bbb3443bacf0..501d5dc1fd3524b1ccc12f39c176456fe0b5a71b 100644
--- a/net/dns/dns_client.cc
+++ b/net/dns/dns_client.cc
@@ -2,89 +2,512 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "net/dns/dns_client.h"
+#include <deque>
+#include <string>
+#include <vector>
 
 #include "base/bind.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop.h"
+#include "base/rand_util.h"
+#include "base/stl_util.h"
 #include "base/string_piece.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/timer.h"
+#include "net/base/completion_callback.h"
+#include "net/base/dns_util.h"
+#include "net/base/io_buffer.h"
+#include "net/base/ip_endpoint.h"
 #include "net/base/net_errors.h"
+#include "net/base/net_log.h"
+#include "net/dns/dns_client.h"

[mmenke, 2012/01/13 16:44:37]

This should go first, above the other includes, per style guide.

+#include "net/dns/dns_protocol.h"
+#include "net/dns/dns_query.h"
 #include "net/dns/dns_response.h"
 #include "net/dns/dns_session.h"
-#include "net/dns/dns_transaction.h"
 #include "net/socket/client_socket_factory.h"
+#include "net/udp/datagram_client_socket.h"
 
 namespace net {
 
-DnsClient::Request::Request(const base::StringPiece& qname,
-                            uint16 qtype,
-                            const RequestCallback& callback)
-    : qname_(qname.data(), qname.size()),
-      qtype_(qtype),
-      callback_(callback) {
+namespace {
+
+// Count labels in the fully-qualified name in DNS format.
+int CountLabels(const std::string& name) {
+  size_t count = 0;
+  for (size_t i = 0; i < name.size() && name[i]; i+= name[i] + 1)

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

Nit: space between i and +=

+    ++count;
+  return count;
 }
 
-DnsClient::Request::~Request() {}
+class StartParameters : public NetLog::EventParameters {
+ public:
+  StartParameters(const std::string& hostname,
+                  uint16 qtype,
+                  const NetLog::Source& source)
+      : hostname_(hostname), qtype_(qtype), source_(source) {}
 
-// Implementation of DnsClient that uses DnsTransaction to serve requests.
-class DnsClientImpl : public DnsClient {
+  virtual Value* ToValue() const {

[mmenke, 2012/01/13 16:44:37]

nit:  OVERRIDE

+    DictionaryValue* dict = new DictionaryValue();
+    dict->SetString("hostname", hostname_);
+    dict->SetInteger("query_type", qtype_);
+    dict->Set("source_dependency", source_.ToValue());
+    return dict;
+  }
+
+ private:
+  std::string hostname_;
+  uint16 qtype_;

[mmenke, 2012/01/13 16:44:37]

nit:  Both of these can be const

+  const NetLog::Source source_;
+};
+
+class ResponseParameters : public NetLog::EventParameters {
+ public:
+  ResponseParameters(int rcode, int answer_count, const NetLog::Source& source)
+      : rcode_(rcode), answer_count_(answer_count), source_(source) {}
+
+  virtual Value* ToValue() const {

[mmenke, 2012/01/13 16:44:37]

nit:  OVERRIDE

+    DictionaryValue* dict = new DictionaryValue();
+    dict->SetInteger("rcode", rcode_);
+    dict->SetInteger("answer_count", answer_count_);
+    dict->Set("source_dependency", source_.ToValue());
+    return dict;
+  }
+
+ private:
+  int rcode_;
+  int answer_count_;

[mmenke, 2012/01/13 16:44:37]

nit:  Both of these can be const

+  const NetLog::Source source_;
+};
+
+// ----------------------------------------------------------------------------
+
+// A single asynchronous DNS exchange over UDP, which consists of sending out a
+// DNS query, waiting for a response, and returning the response that it
+// matches. Logging is done in the socket and in the outer DnsTransaction.
+class DnsUDPAttempt {
+ public:
+  DnsUDPAttempt(scoped_ptr<DatagramClientSocket> socket,
+                const IPEndPoint& server,
+                scoped_ptr<DnsQuery> query,
+                const CompletionCallback& callback)
+      : next_state_(STATE_NONE),
+        socket_(socket.Pass()),
+        server_(server),
+        query_(query.Pass()),
+        callback_(callback) {
+  }
+
+  // Starts the attempt. Returns ERR_IO_PENDING if cannot complete synchronously
+  // and calls |callback| upon completion.
+  int Start() {
+    DCHECK_EQ(STATE_NONE, next_state_);
+    next_state_ = STATE_CONNECT;
+    return DoLoop(OK);
+  }
+
+  const DnsQuery* query() {

[mmenke, 2012/01/13 16:44:37]

nit:  const

+    return query_.get();
+  }
+
+  const DatagramClientSocket* socket() const {
+    return socket_.get();
+  }
+
+  // Returns the response or NULL if has not received a matching response from
+  // the server.
+  const DnsResponse* response() const {
+    return (response_.get() != NULL && response_->Parser().IsValid()) ?
+        response_.get() : NULL;
+  }
+
+ private:
+  enum State {
+    STATE_CONNECT,
+    STATE_SEND_QUERY,
+    STATE_SEND_QUERY_COMPLETE,
+    STATE_READ_RESPONSE,
+    STATE_READ_RESPONSE_COMPLETE,
+    STATE_NONE,
+  };
+
+  int DoLoop(int result) {
+    DCHECK_NE(STATE_NONE, next_state_);
+    int rv = result;
+    do {
+      State state = next_state_;
+      next_state_ = STATE_NONE;
+      switch (state) {
+        case STATE_CONNECT:
+          rv = DoConnect();
+          break;
+        case STATE_SEND_QUERY:
+          rv = DoSendQuery();
+          break;
+        case STATE_SEND_QUERY_COMPLETE:
+          rv = DoSendQueryComplete(rv);
+          break;
+        case STATE_READ_RESPONSE:
+          rv = DoReadResponse();
+          break;
+        case STATE_READ_RESPONSE_COMPLETE:
+          rv = DoReadResponseComplete(rv);
+          break;
+        default:
+          NOTREACHED();
+          break;
+      }
+    } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
+
+    return rv;
+  }
+
+  int DoConnect() {
+    next_state_ = STATE_SEND_QUERY;
+    return socket_->Connect(server_);
+  }
+
+  int DoSendQuery() {
+    next_state_ = STATE_SEND_QUERY_COMPLETE;
+    return socket_->Write(query_->io_buffer(),
+                          query_->io_buffer()->size(),
+                          base::Bind(&DnsUDPAttempt::OnIOComplete,
+                                     base::Unretained(this)));
+  }
+
+  int DoSendQueryComplete(int rv) {
+    if (rv < 0)
+      return rv;
+
+    // Writing to UDP should not result in a partial datagram.
+    if (rv != query_->io_buffer()->size())
+      return ERR_MSG_TOO_BIG;
+
+    next_state_ = STATE_READ_RESPONSE;
+    return OK;
+  }
+
+  int DoReadResponse() {
+    next_state_ = STATE_READ_RESPONSE_COMPLETE;
+    response_.reset(new DnsResponse());
+    return socket_->Read(response_->io_buffer(),
+                         response_->io_buffer()->size(),
+                         base::Bind(&DnsUDPAttempt::OnIOComplete,
+                                    base::Unretained(this)));
+  }
+
+  int DoReadResponseComplete(int rv) {
+    DCHECK_NE(ERR_IO_PENDING, rv);
+    if (rv < 0)
+      return rv;
+
+    DCHECK(rv);
+    if (!response_->InitParse(rv, *query_))
+      return ERR_DNS_MALFORMED_RESPONSE;
+    if (response_->flags() & dns_protocol::kFlagTC)
+      return ERR_DNS_SERVER_REQUIRES_TCP;
+    if (response_->rcode() != dns_protocol::kRcodeNOERROR &&
+        response_->rcode() != dns_protocol::kRcodeNXDOMAIN) {
+      return ERR_DNS_SERVER_FAILED;
+    }
+    if (response_->answer_count() == 0)
+      return ERR_NAME_NOT_RESOLVED;
+
+    return OK;
+  }
+
+  void OnIOComplete(int rv) {
+    rv = DoLoop(rv);
+    if (rv != ERR_IO_PENDING)
+      callback_.Run(rv);
+  }
+
+  State next_state_;
+
+  scoped_ptr<DatagramClientSocket> socket_;
+  IPEndPoint server_;
+  scoped_ptr<DnsQuery> query_;
+
+  scoped_ptr<DnsResponse> response_;
+
+  CompletionCallback callback_;

[mmenke, 2012/01/13 16:44:37]

nit:  DISALLOW_COPY_AND_ASSIGN

+};
+
+// ----------------------------------------------------------------------------
+
+// Implements DnsTransaction. Configuration is supplied by DnsSession.
+// The suffix list is built according to the DnsConfig from the session.
+// The timeout for each DnsUDPAttempt is given by DnsSession::NextTimeout.
+// The first server to attempt on each query is given by
+// DnsSession::NextFirstServerIndex, and the order is round-robin afterwards.
+// Each server is attempted DnsConfig::attempts times.
+class DnsTransactionImpl : public DnsTransaction,
+                           public base::NonThreadSafe,
+                           public base::SupportsWeakPtr<DnsTransactionImpl> {
  public:
-  class RequestImpl : public Request {
-   public:
-    RequestImpl(const base::StringPiece& qname,
-                uint16 qtype,
-                const RequestCallback& callback,
-                DnsSession* session,
-                const BoundNetLog& net_log)
-        : Request(qname, qtype, callback),
-          session_(session),
-          net_log_(net_log) {
+  DnsTransactionImpl(DnsSession* session,
+                     const std::string& hostname,
+                     uint16 qtype,
+                     const DnsClient::CallbackType& callback,
+                     const BoundNetLog& source_net_log)
+    : session_(session),
+      hostname_(hostname),
+      qtype_(qtype),
+      callback_(callback),
+      net_log_(BoundNetLog::Make(session->net_log(),
+                                 NetLog::SOURCE_DNS_TRANSACTION)),
+      successful_attempt_(NULL) {

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

first_server_index_ should be initialized - yeah, it's done in Start, but I like
to be safe. 

+    DCHECK(session_);
+    DCHECK(!hostname_.empty());
+    DCHECK(!callback_.is_null());
+

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

Should this check if qtype is one of the known types for now? 

+    net_log_.BeginEvent(NetLog::TYPE_DNS_TRANSACTION, make_scoped_refptr(
+        new StartParameters(hostname_, qtype_, source_net_log.source())));
+
+    int rv = PrepareSearch();

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

Not really sure if I like having all this in the constructor because of the
synchronous completion case. Could you add a 

int Start();

method to the DnsTransaction interface? 

You'd also want to move the PrepareSearch logic to DnsClient::CreateTransaction
and return a NULL DnsTransaction in that case.

This would also remove the need for weak pointer support. 

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

Do you mean move PrepareSearch to Start? I'd rather not return NULL, because
then the caller receives no neterror.

I've been going back and forth with "int Start()". I modeled the current
behavior after the old HostResolverImpl::Job (now ProcTask) which posts itself
to the MessageLoop on unexpected synchronous failure. But I agree that Start()
is simpler and more clear.

[cbentzel, 2012/01/13 18:12:59]

I did mean do it in DnsClient::CreateTransaction, but am completely happy with
having it done in DnsTransaction::Start

+    if (rv == OK)
+      rv = StartQuery();
+    if (rv != ERR_IO_PENDING) {
+      DCHECK_NE(OK, rv);
+      DCHECK_NE(ERR_NAME_NOT_RESOLVED, rv);
+      // Unexpected synchronous completion. Use WeakPtr in case the user
+      // destroys the transaction before the task is executed.
+      MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&DnsTransactionImpl::DoCallback, AsWeakPtr(), rv));
     }
+  }
 
-    virtual int Start() OVERRIDE {
-      transaction_.reset(new DnsTransaction(
-          session_,
-          qname(),
-          qtype(),
-          base::Bind(&RequestImpl::OnComplete, base::Unretained(this)),
-          net_log_));
-      return transaction_->Start();
+  virtual ~DnsTransactionImpl() {
+    STLDeleteElements(&attempts_);
+    if (!callback_.is_null()) {
+      net_log_.AddEvent(NetLog::TYPE_CANCELLED, NULL);
+      net_log_.EndEventWithNetErrorCode(NetLog::TYPE_DNS_TRANSACTION,
+                                        ERR_ABORTED);
     }
+  }
+
+  virtual const std::string& GetHostname() const OVERRIDE {
+    DCHECK(CalledOnValidThread());
+    return hostname_;
+  }
 
-    void OnComplete(DnsTransaction* transaction, int rv) {
-      DCHECK_EQ(transaction_.get(), transaction);
-      // TODO(szym):
-      // - handle retransmissions here instead of DnsTransaction
-      // - handle rcode and flags here instead of DnsTransaction
-      // - update RTT in DnsSession
-      // - perform suffix search
-      // - handle DNS over TCP
-      DoCallback(rv, (rv == OK) ? transaction->response() : NULL);
+  virtual uint16 GetType() const OVERRIDE {
+    DCHECK(CalledOnValidThread());
+    return qtype_;
+  }
+
+ private:
+  // Prepares |qnames_| according to the DnsConfig.
+  int PrepareSearch() {
+    const DnsConfig& config = session_->config();
+
+    std::string hostname;

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

It would be nice to use a better variable name than hostname, since it's
confusing with hostname_. Unfortunately, I don't think the RFCs differentiate
between dotted/colon separated textual representation and a sequence-of-labels
for this.

Perhaps labeled_name?  

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

I could use dotted_hostname for the other case.

+    if (!DNSDomainFromDot(hostname_, &hostname))
+      return ERR_INVALID_ARGUMENT;
+
+    if (hostname_[hostname_.size() - 1] == '.') {
+      // It's a fully-qualified name, no suffix search.
+      qnames_.push_back(hostname);
+      return OK;
     }
 
-   private:
-    scoped_refptr<DnsSession> session_;
-    BoundNetLog net_log_;
-    scoped_ptr<DnsTransaction> transaction_;
-  };
+    // Set true when |hostname| is put on the list.
+    bool had_hostname = false;
+
+    int ndots = CountLabels(hostname) - 1;
+    if (ndots >= config.ndots) {
+      qnames_.push_back(hostname);
+      had_hostname = true;
+    }
+
+    std::string qname;
+    for (size_t i = 0; i < config.search.size(); ++i) {
+      // Ignore invalid (too long) combinations.
+      if (!DNSDomainFromDot(hostname_ + "." + config.search[i], &qname))
+        continue;
+      if (qname.size() == hostname.size()) {

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

Interesting - is an empty suffix allowed? Or is this some other case I'm not
thinking about.

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

DNSDomainFromDot always appends the root domain at the end. This effectively
checks that config.search[i] == '.'.

I'm a bit concerned that DNSDomainFromDot simply skips empty labels
"www..chromium..org" is converted without question.

+        if (had_hostname)

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

Do you want to make this more generic in the loop - don't add another name to
qnames_ if it already exists? This would prevent double lookups if search
suffixes had the same name twice.

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

I think it'd make sense to do this filtering at the DnsConfigService level,
since it will be re-done for every transaction.

I was actually thinking about converting DnsConfig::search with
DNSDomainFromDot, but then we'd still need a check for length here to ignore
prefix+suffix combinations that are too long.

+          continue;
+        had_hostname = true;
+      }
+      qnames_.push_back(qname);
+    }
+
+    if (!had_hostname)
+      qnames_.push_back(hostname);
+
+    return OK;
+  }
+
+  void DoCallback(int rv) {
+    if (callback_.is_null())
+      return;
+    DCHECK_NE(ERR_IO_PENDING, rv);
+    DCHECK(rv != OK || successful_attempt_ != NULL);
+
+    DnsClient::CallbackType callback = callback_;
+    callback_.Reset();
+    net_log_.EndEventWithNetErrorCode(NetLog::TYPE_DNS_TRANSACTION, rv);
+    callback.Run(this,
+                 rv,
+                 successful_attempt_ ? successful_attempt_->response() : NULL);
+  }
+
+  // Makes another attempt at the current name, |qnames_.front()|, using the
+  // next nameserver.
+  int MakeAttempt() {
+    int attempt_number = attempts_.size();
+
+    scoped_ptr<DatagramClientSocket> socket(
+        session_->socket_factory()->CreateDatagramClientSocket(
+            DatagramSocket::RANDOM_BIND,
+            base::Bind(&base::RandInt),
+            net_log_.net_log(),
+            net_log_.source()));
+
+    uint16 id = session_->NextQueryId();
+    scoped_ptr<DnsQuery> query;
+    if (attempts_.empty()) {
+      query.reset(new DnsQuery(id, qnames_.front(), qtype_));
+    } else {
+      query.reset(attempts_[0]->query()->CloneWithNewId(id));

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

Note to self: See if this any easier.

+    }
+
+    net_log_.AddEvent(NetLog::TYPE_DNS_TRANSACTION_ATTEMPT, make_scoped_refptr(
+        new NetLogSourceParameter("socket", socket->NetLog().source())));

[mmenke, 2012/01/13 16:44:37]

Think it would be a little clearer if you also logged attempt_number, and used
attempt_number in the corresponding response event instead of "socket".  It's
easier to match single digit numbers again each other than source ids,
particularly when the browser has been running a while.

[szym, 2012/01/13 17:17:42]

The reason why I don't include attempt_number is because I assumed it would be
apparent from the net-internals view. The third attempt logged within
DNS_TRANSACTION_QUERY will have attempt_number = 2. The reason I'm including a
reference to the socket source is so that we can see the server address (which
is logged in Connect) and the socket data.

I now realized that I never EndEvent(DNS_TRANSACTION_QUERY). I'll fix that.

[mmenke, 2012/01/13 17:22:58]

I was just thinking that it would be clearer for the
TYPE_DNS_TRANSACTION_RESPONSE to have a number rather than a source, so it's
clearer which attempt actually succeeded, rather than having to match up 4-6
digit source ids.  If you prefer the current approach, that's fine with me.

+
+    const DnsConfig& config = session_->config();
+
+    int server_index = first_server_index_ +

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

You should check for 0 length config.nameservers, since modulo operation with
that is undefined.

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

If |config.nameservers| is empty, the config is invalid, see
DnsConfig::IsValid(), so we would never even create the DnsClient. I'll add a
DCHECK to DnsClient::CreateClient.

[cbentzel, 2012/01/13 18:12:59]

What if the config changed while the DnsTransaction is active to be invalid? I
suppose we won't swap it in - but I want to make sure we don't do an undefined
C++ operation here. 

[szym, 2012/01/13 20:06:17]

DnsConfig is const in DnsSession, so DnsTransaction will not be affected by
config change unless the transaction is destroyed.

+        (attempt_number % config.nameservers.size());
 
+    DnsUDPAttempt* attempt = new DnsUDPAttempt(
+        socket.Pass(),
+        config.nameservers[server_index],
+        query.Pass(),
+        base::Bind(&DnsTransactionImpl::OnAttemptComplete,
+                   base::Unretained(this),
+                   attempt_number));
+
+    base::TimeDelta timeout = session_->NextTimeout(attempt_number);
+    timer_.Start(FROM_HERE, timeout, this, &DnsTransactionImpl::OnTimeout);
+    attempts_.push_back(attempt);
+    return attempt->Start();
+  }
+
+  // Begins query for the current name. Makes the first attempt.
+  int StartQuery() {
+    std::string dotted_qname = DNSDomainToString(qnames_.front());
+    net_log_.BeginEvent(
+        NetLog::TYPE_DNS_TRANSACTION_QUERY,

[mmenke, 2012/01/13 17:16:24]

Where do you log the corresponding end event?

+        make_scoped_refptr(new NetLogStringParameter("qname", dotted_qname)));
+
+    first_server_index_ = session_->NextFirstServerIndex();
+
+    STLDeleteElements(&attempts_);
+    return MakeAttempt();
+  }
+
+  void OnAttemptComplete(int attempt_number, int rv) {
+    timer_.Stop();
+
+    const DnsUDPAttempt* attempt = attempts_[attempt_number];
+
+    net_log_.AddEvent(
+        NetLog::TYPE_DNS_TRANSACTION_RESPONSE,
+        make_scoped_refptr(
+            new NetLogSourceParameter("socket",
+                                      attempt->socket()->NetLog().source())));
+
+    switch (rv) {
+      case ERR_NAME_NOT_RESOLVED:
+        // Try next suffix.
+        qnames_.pop_front();
+        if (qnames_.empty())
+          rv = ERR_NAME_NOT_RESOLVED;
+        else
+          rv = StartQuery();
+        break;
+      case OK:
+        successful_attempt_ = attempt;
+        break;
+      default:
+        // TODO(szym): Some nameservers could fail and we should just ignore
+        // them.
+        break;
+    }
+    if (rv != ERR_IO_PENDING)
+      DoCallback(rv);
+  }
+
+  void OnTimeout() {
+    const DnsConfig& config = session_->config();
+    if (attempts_.size() == config.attempts * config.nameservers.size()) {
+      DoCallback(ERR_DNS_TIMED_OUT);
+      return;
+    }
+    int rv = MakeAttempt();
+    if (rv != ERR_IO_PENDING)
+      DoCallback(rv);
+  }
+
+  scoped_refptr<DnsSession> session_;
+  std::string hostname_;
+  uint16 qtype_;
+  // Set to NULL once the transaction completes.

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

Nit: perhaps null instead of NULL - NULL implies pointer, null is used in
callback_internal.h as well. 

+  DnsClient::CallbackType callback_;
+
+  BoundNetLog net_log_;
+
+  // Search list of fully-qualified DNS names to query next (in DNS format).
+  std::deque<std::string> qnames_;

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

deque not really needed here, since just doing push'es on the back. I'm not too
strongly against it though. 

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

Need qnames_.pop_front().

+
+  // List of attempts for the current name.
+  std::vector<DnsUDPAttempt*> attempts_;
+  // The one of the |attempts_| that succeeded first.

[mmenke, 2012/01/13 16:44:37]

Nit:  This is the member of |attempts_| that succeeded first, if any.

+  const DnsUDPAttempt* successful_attempt_;
+
+  // Index of the first server to try on each search query.
+  int first_server_index_;
+
+  base::OneShotTimer<DnsTransactionImpl> timer_;
+
+  DISALLOW_COPY_AND_ASSIGN(DnsTransactionImpl);
+};
+
+// ----------------------------------------------------------------------------
+
+// Implementation of DnsClient that returns instances of DnsTransactionImpl.
+class DnsClientImpl : public DnsClient {

[mmenke, 2012/01/13 16:44:37]

Is there a compelling reason to keep DnsClient and DnsClientImpl separate?  The
only things you seem to get out of it are the ability to forward declare
DnsSession in the header, and not include the scoped_refptr header.

[szym, 2012/01/13 17:17:42]

The reason I'd keep them separate is so that we can provide MockDnsClient for
tests of HostResolverImpl (see async_host_resolver_unittest.cc). I'm also
thinking about creating a similar factory object for HostResolverImpl::ProcTask,
although there would be less benefit in that.

The reasoning behind MockDnsClient is that setting up DnsClient for tests by
providing a MockClientSocketFactory is cumbersome and often leads to effectively
testing a different component than intended.

[mmenke, 2012/01/13 17:22:58]

Ah, right.  Sounds good to me.

+ public:
   explicit DnsClientImpl(DnsSession* session) {
     session_ = session;
   }
 
-  virtual Request* CreateRequest(
-      const base::StringPiece& qname,
+  virtual scoped_ptr<DnsTransaction> CreateTransaction(
+      const std::string& hostname,
       uint16 qtype,
-      const RequestCallback& callback,
+      const CallbackType& callback,
       const BoundNetLog& source_net_log) OVERRIDE {
-    return new RequestImpl(qname, qtype, callback, session_, source_net_log);
+    return scoped_ptr<DnsTransaction>(new DnsTransactionImpl(session_,
+                                                             hostname,
+                                                             qtype,
+                                                             callback,
+                                                             source_net_log));
   }
 
  private:
   scoped_refptr<DnsSession> session_;
 };
 
+}  // namespace
+
 // static
-DnsClient* DnsClient::CreateClient(DnsSession* session) {
-  return new DnsClientImpl(session);
+scoped_ptr<DnsClient> DnsClient::CreateClient(DnsSession* session) {
+  return scoped_ptr<DnsClient>(new DnsClientImpl(session));
 }
 
 }  // namespace net