[net/dns] Perform A/AAAA queries for AF_UNSPEC resolutions in parallel.

net/dns/host_resolver_impl_unittest.cc

Index: net/dns/host_resolver_impl_unittest.cc
===================================================================
--- net/dns/host_resolver_impl_unittest.cc	(revision 212872)
+++ net/dns/host_resolver_impl_unittest.cc	(working copy)
@@ -12,6 +12,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/condition_variable.h"
@@ -413,6 +414,20 @@
 
   HostResolverImplTest() : proc_(new MockHostResolverProc()) {}
 
+  void CreateResolver() {
+    CreateResolverWithLimitsAndParams(DefaultLimits(),
+                                      DefaultParams(proc_.get()));
+  }
+
+  // This HostResolverImpl will only allow 1 outstanding resolve at a time and
+  // perform no retries.
+  void CreateSerialResolver() {
+    HostResolverImpl::ProcTaskParams params = DefaultParams(proc_.get());
+    params.max_retry_attempts = 0u;
+    PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, 1);
+    CreateResolverWithLimitsAndParams(limits, params);
+  }
+
  protected:
   // A Request::Handler which is a proxy to the HostResolverImplTest fixture.
   struct Handler : public Request::Handler {
@@ -435,26 +450,24 @@
     HostResolverImplTest* test;
   };
 
-  void CreateResolver() {
-    resolver_.reset(new HostResolverImpl(HostCache::CreateDefaultCache(),
-                                         DefaultLimits(),
-                                         DefaultParams(proc_.get()),
-                                         NULL));
+  // testing::Test implementation:
+  virtual void SetUp() OVERRIDE {
+    CreateResolver();
   }
 
-  // This HostResolverImpl will only allow 1 outstanding resolve at a time and
-  // perform no retries.
-  void CreateSerialResolver() {
-    HostResolverImpl::ProcTaskParams params = DefaultParams(proc_.get());
-    params.max_retry_attempts = 0u;
-    PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, 1);
-    resolver_.reset(new HostResolverImpl(
-        HostCache::CreateDefaultCache(),
-        limits,
-        params,
-        NULL));
+  virtual void TearDown() OVERRIDE {
+    if (resolver_.get())
+      EXPECT_EQ(0u, resolver_->num_running_dispatcher_jobs_for_tests());
+    EXPECT_FALSE(proc_->HasBlockedRequests());
   }
 
+  virtual void CreateResolverWithLimitsAndParams(
+      const PrioritizedDispatcher::Limits& limits,
+      const HostResolverImpl::ProcTaskParams& params) {
+    resolver_.reset(new HostResolverImpl(HostCache::CreateDefaultCache(),
+                                         limits, params, NULL));
+  }
+
   // The Request will not be made until a call to |Resolve()|, and the Job will
   // not start until released by |proc_->SignalXXX|.
   Request* CreateRequest(const HostResolver::RequestInfo& info) {
@@ -488,25 +501,15 @@
     return CreateRequest(hostname, kDefaultPort);
   }
 
-  virtual void SetUp() OVERRIDE {
-    CreateResolver();
-  }
-
-  virtual void TearDown() OVERRIDE {
-    if (resolver_.get())
-      EXPECT_EQ(0u, resolver_->num_running_jobs_for_tests());
-    EXPECT_FALSE(proc_->HasBlockedRequests());
-  }
-
   void set_handler(Handler* handler) {
     handler_.reset(handler);
     handler_->test = this;
   }
 
   // Friendship is not inherited, so use proxies to access those.
-  size_t num_running_jobs() const {
+  size_t num_running_dispatcher_jobs() const {
     DCHECK(resolver_.get());
-    return resolver_->num_running_jobs_for_tests();
+    return resolver_->num_running_dispatcher_jobs_for_tests();
   }
 
   void set_fallback_to_proctask(bool fallback_to_proctask) {
@@ -889,7 +892,7 @@
 
   EXPECT_EQ(ERR_NETWORK_CHANGED, requests_[0]->WaitForResult());
 
-  EXPECT_EQ(1u, num_running_jobs());
+  EXPECT_EQ(1u, num_running_dispatcher_jobs());
 
   EXPECT_FALSE(requests_[1]->completed());
   EXPECT_FALSE(requests_[2]->completed());
@@ -1255,36 +1258,63 @@
 
 // Specialized fixture for tests of DnsTask.
 class HostResolverImplDnsTest : public HostResolverImplTest {
+ public:
+  HostResolverImplDnsTest() : dns_client_(NULL) {}
+
  protected:
+  // testing::Test implementation:
   virtual void SetUp() OVERRIDE {
-    AddDnsRule("nx", dns_protocol::kTypeA, MockDnsClientRule::FAIL);
-    AddDnsRule("nx", dns_protocol::kTypeAAAA, MockDnsClientRule::FAIL);
-    AddDnsRule("ok", dns_protocol::kTypeA, MockDnsClientRule::OK);
-    AddDnsRule("ok", dns_protocol::kTypeAAAA, MockDnsClientRule::OK);
-    AddDnsRule("4ok", dns_protocol::kTypeA, MockDnsClientRule::OK);
-    AddDnsRule("4ok", dns_protocol::kTypeAAAA, MockDnsClientRule::EMPTY);
-    AddDnsRule("6ok", dns_protocol::kTypeA, MockDnsClientRule::EMPTY);
-    AddDnsRule("6ok", dns_protocol::kTypeAAAA, MockDnsClientRule::OK);
-    AddDnsRule("4nx", dns_protocol::kTypeA, MockDnsClientRule::OK);
-    AddDnsRule("4nx", dns_protocol::kTypeAAAA, MockDnsClientRule::FAIL);
+    AddDnsRule("nx", dns_protocol::kTypeA, MockDnsClientRule::FAIL, false);
+    AddDnsRule("nx", dns_protocol::kTypeAAAA, MockDnsClientRule::FAIL, false);
+    AddDnsRule("ok", dns_protocol::kTypeA, MockDnsClientRule::OK, false);
+    AddDnsRule("ok", dns_protocol::kTypeAAAA, MockDnsClientRule::OK, false);
+    AddDnsRule("4ok", dns_protocol::kTypeA, MockDnsClientRule::OK, false);
+    AddDnsRule("4ok", dns_protocol::kTypeAAAA, MockDnsClientRule::EMPTY, false);
+    AddDnsRule("6ok", dns_protocol::kTypeA, MockDnsClientRule::EMPTY, false);
+    AddDnsRule("6ok", dns_protocol::kTypeAAAA, MockDnsClientRule::OK, false);
+    AddDnsRule("4nx", dns_protocol::kTypeA, MockDnsClientRule::OK, false);
+    AddDnsRule("4nx", dns_protocol::kTypeAAAA, MockDnsClientRule::FAIL, false);
+
+    AddDnsRule("4slow_ok", dns_protocol::kTypeA, MockDnsClientRule::OK, true);
+    AddDnsRule("4slow_ok", dns_protocol::kTypeAAAA, MockDnsClientRule::OK,
+               false);
+    AddDnsRule("6slow_ok", dns_protocol::kTypeA, MockDnsClientRule::OK, false);
+    AddDnsRule("6slow_ok", dns_protocol::kTypeAAAA, MockDnsClientRule::OK,
+               true);
+    AddDnsRule("4slow_4ok", dns_protocol::kTypeA, MockDnsClientRule::OK, true);
+    AddDnsRule("4slow_4ok", dns_protocol::kTypeAAAA, MockDnsClientRule::EMPTY,
+               false);
+    AddDnsRule("4slow_4timeout", dns_protocol::kTypeA,
+               MockDnsClientRule::TIMEOUT, true);
+    AddDnsRule("4slow_4timeout", dns_protocol::kTypeAAAA, MockDnsClientRule::OK,
+               false);
+    AddDnsRule("4slow_6timeout", dns_protocol::kTypeA,
+               MockDnsClientRule::OK, true);
+    AddDnsRule("4slow_6timeout", dns_protocol::kTypeAAAA,
+               MockDnsClientRule::TIMEOUT, false);
     CreateResolver();
   }
 
-  void CreateResolver() {
+  // HostResolverImplTest implementation:
+  virtual void CreateResolverWithLimitsAndParams(
+      const PrioritizedDispatcher::Limits& limits,
+      const HostResolverImpl::ProcTaskParams& params) OVERRIDE {
     resolver_.reset(new HostResolverImpl(HostCache::CreateDefaultCache(),
-                                         DefaultLimits(),
-                                         DefaultParams(proc_.get()),
+                                         limits,
+                                         params,
                                          NULL));
     // Disable IPv6 support probing.
     resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
-    resolver_->SetDnsClient(CreateMockDnsClient(DnsConfig(), dns_rules_));
+    dns_client_ = new MockDnsClient(DnsConfig(), dns_rules_);
+    resolver_->SetDnsClient(scoped_ptr<DnsClient>(dns_client_));
   }
 
   // Adds a rule to |dns_rules_|. Must be followed by |CreateResolver| to apply.
   void AddDnsRule(const std::string& prefix,
                   uint16 qtype,
-                  MockDnsClientRule::Result result) {
-    dns_rules_.push_back(MockDnsClientRule(prefix, qtype, result));
+                  MockDnsClientRule::Result result,
+                  bool delay) {
+    dns_rules_.push_back(MockDnsClientRule(prefix, qtype, result, delay));
   }
 
   void ChangeDnsConfig(const DnsConfig& config) {
@@ -1294,6 +1324,7 @@
   }
 
   MockDnsClientRuleList dns_rules_;
+  MockDnsClient* dns_client_;

[szym, 2013/08/06 20:35:31]

  // Owned by |resolver_|.

[mmenke, 2013/08/19 17:31:11]

Done.

 };
 
 // TODO(szym): Test AbortAllInProgressJobs due to DnsConfig change.
@@ -1361,7 +1392,8 @@
 
   // Simulate the case when the preference or policy has disabled the DNS client
   // causing AbortDnsTasks.
-  resolver_->SetDnsClient(CreateMockDnsClient(DnsConfig(), dns_rules_));
+  resolver_->SetDnsClient(
+      scoped_ptr<DnsClient>(new MockDnsClient(DnsConfig(), dns_rules_)));
   ChangeDnsConfig(CreateValidDnsConfig());
 
   // First request is resolved by MockDnsClient, others should fail due to
@@ -1585,7 +1617,8 @@
                                        DefaultLimits(),
                                        DefaultParams(proc.get()),
                                        NULL));
-  resolver_->SetDnsClient(CreateMockDnsClient(DnsConfig(), dns_rules_));
+  resolver_->SetDnsClient(
+      scoped_ptr<DnsClient>(new MockDnsClient(DnsConfig(), dns_rules_)));
   resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
 
   // Get the expected output.
@@ -1638,4 +1671,157 @@
   EXPECT_EQ(saw_ipv6, req->HasAddress("::1", 80));
 }
 
+// Cancel a request with a single DNS transaction active.
+TEST_F(HostResolverImplDnsTest, AsyncResolverCancelWithOneTransactionActive) {

[szym, 2013/08/06 20:35:31]

No need for AsyncResolver prefix to these tests. All HostResolverImplDnsTest
exercise DnsTask.

[mmenke, 2013/08/19 17:31:11]

Done (Though I think it's very easy to miss the "DNS" in
"HostResolverImplDnsTest").

+  set_fallback_to_proctask(false);

[szym, 2013/08/06 20:35:31]

I don't think these tests need set_fallback_to_proctask(false);

[mmenke, 2013/08/19 17:31:11]

Done.

+  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
+  ChangeDnsConfig(CreateValidDnsConfig());
+
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok", 80)->Resolve());
+  EXPECT_EQ(1u, num_running_dispatcher_jobs());
+  requests_[0]->Cancel();

[szym, 2013/08/06 20:35:31]

When the test depends on TearDown, suggest comment at end of test:
// Check result in TearDown.

[mmenke, 2013/08/19 17:31:11]

Done.

+}
+
+// Cancel a request with a single DNS transaction active and another pending.
+TEST_F(HostResolverImplDnsTest,
+       AsyncResolverCancelWithOneTransactionActiveOnePending) {
+  CreateSerialResolver();
+  set_fallback_to_proctask(false);
+  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
+  ChangeDnsConfig(CreateValidDnsConfig());
+
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok", 80)->Resolve());
+  EXPECT_EQ(1u, num_running_dispatcher_jobs());
+  requests_[0]->Cancel();
+}
+
+// Cancel a request with two DNS transactions active.
+TEST_F(HostResolverImplDnsTest, AsyncResolverCancelWithTwoTransactionsActive) {
+  set_fallback_to_proctask(false);
+  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
+  ChangeDnsConfig(CreateValidDnsConfig());
+
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok", 80)->Resolve());
+  EXPECT_EQ(2u, num_running_dispatcher_jobs());
+  requests_[0]->Cancel();
+}
+
+// Cancel a request with only the IPv6 transaction active.
+TEST_F(HostResolverImplDnsTest, AsyncResolverCancelWithIPv6TransactionActive) {
+  set_fallback_to_proctask(false);
+  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
+  ChangeDnsConfig(CreateValidDnsConfig());
+
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("6slow_ok", 80)->Resolve());
+  EXPECT_EQ(2u, num_running_dispatcher_jobs());
+
+  // The IPv4 request should complete, the IPv6 request is still pending.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1u, num_running_dispatcher_jobs());
+
+  requests_[0]->Cancel();
+}
+
+// Cancel a request with only the IPv4 transaction pending.
+TEST_F(HostResolverImplDnsTest, AsyncResolverCancelWithIPv4TransactionPending) {
+  set_fallback_to_proctask(false);
+  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
+  ChangeDnsConfig(CreateValidDnsConfig());
+
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("4slow_ok", 80)->Resolve());
+  EXPECT_EQ(2u, num_running_dispatcher_jobs());
+
+  // The IPv6 request should complete, the IPv4 request is still pending.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1u, num_running_dispatcher_jobs());
+
+  requests_[0]->Cancel();
+}
+

[szym, 2013/08/06 20:35:31]

Add a test which ends in resolver_.Reset() without canceling the requests.

[mmenke, 2013/08/19 17:31:11]

Done (Added two, actually).

+// Test cases where AAAA completes first.
+TEST_F(HostResolverImplDnsTest, AAAACompletesFirst) {
+  set_fallback_to_proctask(false);

[szym, 2013/08/06 20:35:31]

OK. These three last tests do need fallback disabled.

[mmenke, 2013/08/19 17:31:11]

Yea, I wrote them first, and then just copied + pasted for the others, without
thinking about it.

+  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
+  ChangeDnsConfig(CreateValidDnsConfig());
+
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("4slow_ok", 80)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("4slow_4ok", 80)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("4slow_4timeout", 80)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("4slow_6timeout", 80)->Resolve());
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(requests_[0]->completed());
+  EXPECT_FALSE(requests_[1]->completed());
+  EXPECT_FALSE(requests_[2]->completed());
+  // The IPv6 of the third request should have failed and resulted in cancelling
+  // the IPv4 request.
+  EXPECT_TRUE(requests_[3]->completed());
+  EXPECT_EQ(ERR_DNS_TIMED_OUT, requests_[3]->result());
+  EXPECT_EQ(3u, num_running_dispatcher_jobs());
+
+  dns_client_->CompleteDelayedTransactions();
+  EXPECT_TRUE(requests_[0]->completed());
+  EXPECT_EQ(OK, requests_[0]->result());
+  EXPECT_EQ(2u, requests_[0]->NumberOfAddresses());
+  EXPECT_TRUE(requests_[0]->HasAddress("127.0.0.1", 80));
+  EXPECT_TRUE(requests_[0]->HasAddress("::1", 80));
+
+  EXPECT_TRUE(requests_[1]->completed());
+  EXPECT_EQ(OK, requests_[1]->result());
+  EXPECT_EQ(1u, requests_[1]->NumberOfAddresses());
+  EXPECT_TRUE(requests_[1]->HasAddress("127.0.0.1", 80));
+
+  EXPECT_TRUE(requests_[2]->completed());
+  EXPECT_EQ(ERR_DNS_TIMED_OUT, requests_[2]->result());
+}
+
+// Test the case where only a single transaction slot is available.
+TEST_F(HostResolverImplDnsTest, SerialResolver) {
+  CreateSerialResolver();
+  set_fallback_to_proctask(false);
+  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
+  ChangeDnsConfig(CreateValidDnsConfig());
+
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok", 80)->Resolve());
+  EXPECT_EQ(1u, num_running_dispatcher_jobs());
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(requests_[0]->completed());
+  EXPECT_EQ(OK, requests_[0]->result());
+  EXPECT_EQ(2u, requests_[0]->NumberOfAddresses());
+  EXPECT_TRUE(requests_[0]->HasAddress("127.0.0.1", 80));
+  EXPECT_TRUE(requests_[0]->HasAddress("::1", 80));
+}
+
+// Test the case where the AAAA query is started when another transaction
+// completes.
+TEST_F(HostResolverImplDnsTest, AAAAStartsAfterOtherJobFinishes) {
+  CreateResolverWithLimitsAndParams(
+      PrioritizedDispatcher::Limits(NUM_PRIORITIES, 2),
+      DefaultParams(proc_.get()));
+  set_fallback_to_proctask(false);
+  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
+  ChangeDnsConfig(CreateValidDnsConfig());
+
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok", 80, MEDIUM,

[szym, 2013/08/06 20:35:31]

MEDIUM is the default priority in CreateRequest.

[mmenke, 2013/08/19 17:31:11]

There's no overload that takes an address family, but not a priority.  I use an
IPv4 request so it only occupies one slot.

[szym, 2013/08/19 18:47:21]

I totally missed that this request is IPV4 only. Sorry.

+                                          ADDRESS_FAMILY_IPV4)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("4slow_ok", 80)->Resolve());

[szym, 2013/08/06 20:35:31]

Maybe make the priority explicit here.

[mmenke, 2013/08/19 17:31:11]

Done.

+  // An IPv4 request should have been started pending for each job.

[szym, 2013/08/06 20:35:31]

Actually, the first Resolve() call will cause the first Job to launch both of
its transactions. The two job slots for the second Resolve() will be queued
despite its higher priority.

To exercise the intended scenario use CreateSerialResolver() here and just
interleave them as follows:

req0->Resolve at MEDIUM, it takes the job slot, queues another at MEDIUM
req1->Resolve at HIGH, it queues two at HIGH

RunUntilIdle
req0's first job completes, req1's first job is started
req1's first job completes, req1's second job is started
req1's first job completes, req0's second job is started
req0's second job completes

Up to you if you want to sprinkle in some delayed transactions.

[mmenke, 2013/08/19 17:31:11]

The first resolve only has one transaction (It's IPv4 for the purpose).  The
jobs are supposed to have the same priority.  The purpose of the test is to have
a Job's second transaction start when the transaction of some other job
completes while the first transaction is still running.  A serial resolver can't
do that.
Your suggestion doesn't actually work - when req0's first job completes, req0's
second job will start, if it hasn't already, since I bypass the dispatcher's
queue in that case.

[mmenke, 2013/08/19 18:12:08]

To give a better explanation of how this works:

Request 0 starts its only transaction on creation.
Request 1 starts its A transaction on creation.
Request 0 completes, request 1 starts its AAAA transaction (In the RunUntilIdle
call).
Request 1's AAAA transaction completes (Also in the RunUntilIdle call)
Request 1's A transaction completes (In the CompleteDelayedTransactions call).

[szym, 2013/08/19 18:47:21]

I understand now. It all makes sense once I noticed that req0 is single family.
Right.

+  EXPECT_EQ(2u, num_running_dispatcher_jobs());
+
+  // Request 0's IPv4 request should complete, starting Request 1's IPv6
+  // request, which should also complete.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1u, num_running_dispatcher_jobs());
+  EXPECT_TRUE(requests_[0]->completed());
+  EXPECT_FALSE(requests_[1]->completed());
+
+  dns_client_->CompleteDelayedTransactions();
+  EXPECT_TRUE(requests_[1]->completed());
+  EXPECT_EQ(OK, requests_[1]->result());
+  EXPECT_EQ(2u, requests_[1]->NumberOfAddresses());
+  EXPECT_TRUE(requests_[1]->HasAddress("127.0.0.1", 80));
+  EXPECT_TRUE(requests_[1]->HasAddress("::1", 80));
+}
+
 }  // namespace net