Allow LogDnsClient queries to be rate-limited

components/certificate_transparency/log_dns_client_unittest.cc

 // Copyright 2016 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.
 
 #include "components/certificate_transparency/log_dns_client.h"
 
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
+#include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "components/certificate_transparency/mock_log_dns_traffic.h"
 #include "crypto/sha2.h"
 #include "net/base/net_errors.h"
 #include "net/cert/merkle_audit_proof.h"
 #include "net/cert/signed_certificate_timestamp.h"
 #include "net/dns/dns_client.h"
 #include "net/dns/dns_config_service.h"
 #include "net/dns/dns_protocol.h"
 #include "net/log/net_log.h"
 #include "net/test/gtest_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace certificate_transparency {
 namespace {
 
+using ::testing::Eq;
 using ::testing::IsEmpty;
 using ::testing::IsNull;
 using ::testing::Not;
 using ::testing::NotNull;
 using net::test::IsError;
 using net::test::IsOk;
 
 constexpr char kLeafHash[] =
     "\x1f\x25\xe1\xca\xba\x4f\xf9\xb8\x27\x24\x83\x0f\xca\x60\xe4\xc2\xbe\xa8"
     "\xc3\xa9\x44\x1c\x27\xb0\xb4\x3e\x6a\x96\x94\xc7\xb8\x04";
@@ skipping 73 matching lines @@
 };
 
 class LogDnsClientTest : public ::testing::TestWithParam<net::IoMode> {
  protected:
   LogDnsClientTest()
       : network_change_notifier_(net::NetworkChangeNotifier::CreateMock()) {
     mock_dns_.SetSocketReadMode(GetParam());
     mock_dns_.InitializeDnsConfig();
   }
 
+  std::unique_ptr<LogDnsClient> CreateLogDnsClient(
+      size_t max_concurrent_queries) {
+    return base::MakeUnique<LogDnsClient>(mock_dns_.CreateDnsClient(),
+                                          net::BoundNetLog(),
+                                          max_concurrent_queries);
+  }
+
+  void QueryLeafIndexAsync(LogDnsClient* log_client,
+                           base::StringPiece log_domain,
+                           base::StringPiece leaf_hash,
+                           const LogDnsClient::LeafIndexCallback& callback) {
+    log_client->QueryLeafIndex(log_domain, leaf_hash, callback);
+  }
+
+  // Convenience function for calling QueryLeafIndexAsync synchronously.
   void QueryLeafIndex(base::StringPiece log_domain,
                       base::StringPiece leaf_hash,
                       MockLeafIndexCallback* callback) {
-    LogDnsClient log_client(mock_dns_.CreateDnsClient(), net::BoundNetLog());
-    log_client.QueryLeafIndex(log_domain, leaf_hash, callback->AsCallback());
+    std::unique_ptr<LogDnsClient> log_client = CreateLogDnsClient(0);
+    QueryLeafIndexAsync(log_client.get(), log_domain, leaf_hash,
+                        callback->AsCallback());
     callback->WaitUntilRun();
   }
 
+  void QueryAuditProofAsync(LogDnsClient* log_client,
+                            base::StringPiece log_domain,
+                            uint64_t leaf_index,
+                            uint64_t tree_size,
+                            const LogDnsClient::AuditProofCallback& callback) {
+    log_client->QueryAuditProof(log_domain, leaf_index, tree_size, callback);
+  }
+
+  // Convenience function for calling QueryAuditProofAsync synchronously.
   void QueryAuditProof(base::StringPiece log_domain,
                        uint64_t leaf_index,
                        uint64_t tree_size,
                        MockAuditProofCallback* callback) {
-    LogDnsClient log_client(mock_dns_.CreateDnsClient(), net::BoundNetLog());
-    log_client.QueryAuditProof(log_domain, leaf_index, tree_size,
-                               callback->AsCallback());
+    std::unique_ptr<LogDnsClient> log_client = CreateLogDnsClient(0);
+    QueryAuditProofAsync(log_client.get(), log_domain, leaf_index, tree_size,
+                         callback->AsCallback());
     callback->WaitUntilRun();
   }
 
   // This will be the NetworkChangeNotifier singleton for the duration of the
   // test. It is accessed statically by LogDnsClient.
   std::unique_ptr<net::NetworkChangeNotifier> network_change_notifier_;
   // Queues and handles asynchronous DNS tasks. Indirectly used by LogDnsClient,
   // the underlying net::DnsClient, and NetworkChangeNotifier.
   base::MessageLoopForIO message_loop_;
   // Allows mock DNS sockets to be setup.
   MockLogDnsTraffic mock_dns_;
 };
 
 TEST_P(LogDnsClientTest, QueryLeafIndex) {
   mock_dns_.ExpectLeafIndexRequestAndResponse(
       "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
       "123456");
 
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", kLeafHash, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsOk());
-  EXPECT_THAT(callback.leaf_index(), 123456);
+  EXPECT_THAT(callback.leaf_index(), Eq(123456u));
 }
 
 TEST_P(LogDnsClientTest, QueryLeafIndexReportsThatLogDomainDoesNotExist) {
   mock_dns_.ExpectRequestAndErrorResponse(
       "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
       net::dns_protocol::kRcodeNXDOMAIN);
 
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", kLeafHash, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_NAME_NOT_RESOLVED));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest, QueryLeafIndexReportsServerFailure) {
   mock_dns_.ExpectRequestAndErrorResponse(
       "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
       net::dns_protocol::kRcodeSERVFAIL);
 
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", kLeafHash, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_DNS_SERVER_FAILED));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest, QueryLeafIndexReportsServerRefusal) {
   mock_dns_.ExpectRequestAndErrorResponse(
       "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
       net::dns_protocol::kRcodeREFUSED);
 
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", kLeafHash, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_DNS_SERVER_FAILED));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest,
        QueryLeafIndexReportsMalformedResponseIfLeafIndexIsNotNumeric) {
   mock_dns_.ExpectLeafIndexRequestAndResponse(
       "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
       "foo");
 
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", kLeafHash, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_DNS_MALFORMED_RESPONSE));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest,
        QueryLeafIndexReportsMalformedResponseIfLeafIndexIsFloatingPoint) {
   mock_dns_.ExpectLeafIndexRequestAndResponse(
       "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
       "123456.0");
 
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", kLeafHash, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_DNS_MALFORMED_RESPONSE));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest,
        QueryLeafIndexReportsMalformedResponseIfLeafIndexIsEmpty) {
   mock_dns_.ExpectLeafIndexRequestAndResponse(
       "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.", "");
 
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", kLeafHash, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_DNS_MALFORMED_RESPONSE));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest,
        QueryLeafIndexReportsMalformedResponseIfLeafIndexHasNonNumericPrefix) {
   mock_dns_.ExpectLeafIndexRequestAndResponse(
       "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
       "foo123456");
 
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", kLeafHash, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_DNS_MALFORMED_RESPONSE));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest,
        QueryLeafIndexReportsMalformedResponseIfLeafIndexHasNonNumericSuffix) {
   mock_dns_.ExpectLeafIndexRequestAndResponse(
       "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
       "123456foo");
 
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", kLeafHash, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_DNS_MALFORMED_RESPONSE));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest, QueryLeafIndexReportsInvalidArgIfLogDomainIsEmpty) {
   MockLeafIndexCallback callback;
   QueryLeafIndex("", kLeafHash, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_INVALID_ARGUMENT));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest, QueryLeafIndexReportsInvalidArgIfLogDomainIsNull) {
   MockLeafIndexCallback callback;
   QueryLeafIndex(nullptr, kLeafHash, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_INVALID_ARGUMENT));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest, QueryLeafIndexReportsInvalidArgIfLeafHashIsInvalid) {
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", "foo", &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_INVALID_ARGUMENT));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest, QueryLeafIndexReportsInvalidArgIfLeafHashIsEmpty) {
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", "", &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_INVALID_ARGUMENT));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest, QueryLeafIndexReportsInvalidArgIfLeafHashIsNull) {
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", nullptr, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_INVALID_ARGUMENT));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest, QueryLeafIndexReportsSocketError) {
   mock_dns_.ExpectRequestAndSocketError(
       "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
       net::ERR_CONNECTION_REFUSED);
 
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", kLeafHash, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_CONNECTION_REFUSED));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest, QueryLeafIndexReportsTimeout) {
   mock_dns_.ExpectRequestAndTimeout(
       "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.");
 
   MockLeafIndexCallback callback;
   QueryLeafIndex("ct.test", kLeafHash, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_DNS_TIMED_OUT));
-  EXPECT_THAT(callback.leaf_index(), 0);
+  EXPECT_THAT(callback.leaf_index(), Eq(0u));
 }
 
 TEST_P(LogDnsClientTest, QueryAuditProof) {
   const std::vector<std::string> audit_proof = GetSampleAuditProof(20);
 
-  // It should require 3 queries to collect the entire audit proof, as there is
-  // only space for 7 nodes per UDP packet.
+  // It should require 3 requests to collect the entire audit proof, as there is
+  // only space for 7 nodes per TXT record. One node is 32 bytes long and the
+  // TXT RDATA can have a maximum length of 255 bytes (255 / 32).
   mock_dns_.ExpectAuditProofRequestAndResponse("0.123456.999999.tree.ct.test.",
                                                audit_proof.begin(),
                                                audit_proof.begin() + 7);
   mock_dns_.ExpectAuditProofRequestAndResponse("7.123456.999999.tree.ct.test.",
                                                audit_proof.begin() + 7,
                                                audit_proof.begin() + 14);
   mock_dns_.ExpectAuditProofRequestAndResponse("14.123456.999999.tree.ct.test.",
                                                audit_proof.begin() + 14,
                                                audit_proof.end());
 
   MockAuditProofCallback callback;
   QueryAuditProof("ct.test", 123456, 999999, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsOk());
   ASSERT_THAT(callback.proof(), NotNull());
-  EXPECT_THAT(callback.proof()->leaf_index, 123456);
-  // EXPECT_THAT(callback.proof()->tree_size, 999999);
-  EXPECT_THAT(callback.proof()->nodes, audit_proof);
+  EXPECT_THAT(callback.proof()->leaf_index, Eq(123456u));
+  // TODO(robpercival): Enable this once MerkleAuditProof has tree_size.
+  // EXPECT_THAT(callback.proof()->tree_size, Eq(999999));
+  EXPECT_THAT(callback.proof()->nodes, Eq(audit_proof));
 }
 
 TEST_P(LogDnsClientTest, QueryAuditProofHandlesResponsesWithShortAuditPaths) {
   const std::vector<std::string> audit_proof = GetSampleAuditProof(20);
 
   // Make some of the responses contain fewer proof nodes than they can hold.
   mock_dns_.ExpectAuditProofRequestAndResponse("0.123456.999999.tree.ct.test.",
                                                audit_proof.begin(),
                                                audit_proof.begin() + 1);
   mock_dns_.ExpectAuditProofRequestAndResponse("1.123456.999999.tree.ct.test.",
@@ skipping 10 matching lines @@
                                                audit_proof.begin() + 13);
   mock_dns_.ExpectAuditProofRequestAndResponse("13.123456.999999.tree.ct.test.",
                                                audit_proof.begin() + 13,
                                                audit_proof.end());
 
   MockAuditProofCallback callback;
   QueryAuditProof("ct.test", 123456, 999999, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsOk());
   ASSERT_THAT(callback.proof(), NotNull());
-  EXPECT_THAT(callback.proof()->leaf_index, 123456);
-  // EXPECT_THAT(callback.proof()->tree_size, 999999);
-  EXPECT_THAT(callback.proof()->nodes, audit_proof);
+  EXPECT_THAT(callback.proof()->leaf_index, Eq(123456u));
+  // TODO(robpercival): Enable this once MerkleAuditProof has tree_size.
+  // EXPECT_THAT(callback.proof()->tree_size, Eq(999999));
+  EXPECT_THAT(callback.proof()->nodes, Eq(audit_proof));
 }
 
 TEST_P(LogDnsClientTest, QueryAuditProofReportsThatLogDomainDoesNotExist) {
   mock_dns_.ExpectRequestAndErrorResponse("0.123456.999999.tree.ct.test.",
                                           net::dns_protocol::kRcodeNXDOMAIN);
 
   MockAuditProofCallback callback;
   QueryAuditProof("ct.test", 123456, 999999, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_NAME_NOT_RESOLVED));
@@ skipping 115 matching lines @@
   MockAuditProofCallback callback;
   QueryAuditProof("ct.test", 123456, 999999, &callback);
   ASSERT_TRUE(callback.called());
   EXPECT_THAT(callback.net_error(), IsError(net::ERR_DNS_TIMED_OUT));
   EXPECT_THAT(callback.proof(), IsNull());
 }
 
 TEST_P(LogDnsClientTest, AdoptsLatestDnsConfigIfValid) {
   std::unique_ptr<net::DnsClient> tmp = mock_dns_.CreateDnsClient();
   net::DnsClient* dns_client = tmp.get();
-  LogDnsClient log_client(std::move(tmp), net::BoundNetLog());
+  LogDnsClient log_client(std::move(tmp), net::BoundNetLog(), 0);
 
   // Get the current DNS config, modify it and broadcast the update.
   net::DnsConfig config(*dns_client->GetConfig());
   ASSERT_NE(123, config.attempts);
   config.attempts = 123;
   mock_dns_.SetDnsConfig(config);
 
   // Let the DNS config change propogate.
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(123, dns_client->GetConfig()->attempts);
 }
 
 TEST_P(LogDnsClientTest, IgnoresLatestDnsConfigIfInvalid) {
   std::unique_ptr<net::DnsClient> tmp = mock_dns_.CreateDnsClient();
   net::DnsClient* dns_client = tmp.get();
-  LogDnsClient log_client(std::move(tmp), net::BoundNetLog());
+  LogDnsClient log_client(std::move(tmp), net::BoundNetLog(), 0);
 
   // Get the current DNS config, modify it and broadcast the update.
   net::DnsConfig config(*dns_client->GetConfig());
   ASSERT_THAT(config.nameservers, Not(IsEmpty()));
   config.nameservers.clear();  // Makes config invalid
   mock_dns_.SetDnsConfig(config);
 
   // Let the DNS config change propogate.
   base::RunLoop().RunUntilIdle();
   EXPECT_THAT(dns_client->GetConfig()->nameservers, Not(IsEmpty()));
 }
 
+TEST_P(LogDnsClientTest, CanPerformLeafIndexQueriesInParallel) {
+  // Test that 3 leaf index queries can be performed in parallel.
+  constexpr size_t kNumOfParallelQueries = 3;
+  std::unique_ptr<LogDnsClient> log_client =
+      CreateLogDnsClient(kNumOfParallelQueries);
+  MockLeafIndexCallback callbacks[kNumOfParallelQueries];
+
+  // Expect 3 requests.
+  for (size_t i = 0; i < kNumOfParallelQueries; ++i) {
+    mock_dns_.ExpectLeafIndexRequestAndResponse(
+        "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
+        "123456");

[Ryan Sleevi, 2016/09/16 02:11:03]

Is this value necessary as part of the test? The same applies to all of the
tests that you add here - are the actual values meaningful? You're testing that
queries can be made in parallel, which seems to be more about state transitions;
it's unclear to me whether the values themselves are significant and relevant to
the tests.

[Rob Percival, 2016/09/16 16:28:19]

They're required to test that nothing goes quietly wrong when running queries in
parallel, i.e. the queries don't fail, but just return the wrong data. These
tests would be better if they used different inputs for each of the queries, and
perhaps that would demonstrate that the values are meaningful. As it happens, I
had already started on doing that. Along with extra documentation, would that
address this issue?

[Ryan Sleevi, 2016/09/17 01:51:52]

Right, with the current structure (using the same inputs) it doesn't seem like
you're getting what you wanted to be tested. Using different inputs would make
it more obvious that the inputs are themselves meaningful.

[Rob Percival, 2016/09/20 18:54:42]

Done.

+  }
+
+  // Start the queries.
+  for (size_t i = 0; i < kNumOfParallelQueries; ++i) {
+    QueryLeafIndexAsync(log_client.get(), "ct.test", kLeafHash,
+                        callbacks[i].AsCallback());
+  }
+
+  // Wait for each query to complete and check its results.
+  for (size_t i = 0; i < kNumOfParallelQueries; ++i) {
+    MockLeafIndexCallback& callback = callbacks[i];
+    callback.WaitUntilRun();
+
+    SCOPED_TRACE(i);
+    EXPECT_TRUE(callback.called());
+    if (callback.called()) {
+      EXPECT_THAT(callback.net_error(), IsOk());
+      EXPECT_THAT(callback.leaf_index(), Eq(123456u));
+    }
+  }

[Ryan Sleevi, 2016/09/16 02:11:03]

For what it's worth, I spent 20 minutes digging through code to try and convince
myself that MockLeafIndexCallback was going to be safe if these parallel
queries' callbacks are invoked out of order (e.g. started as 1-2-3 but finish as
3-2-1).

The answer is that it's only safe because base::RunLoop is 'safe' to have Quit()
called before Run() is called, and it terminates immediately.

That said, I think the heart of my unease is that it feels very... weird... to
be smuggling base::RunLoops on callbacks, considering that a base::RunLoop's
intended purpose was with respect to nesting message loops, and we're totally
not trying to do that here (they're independent state machines)

[Rob Percival, 2016/09/16 16:28:19]

This is another thing that was only added as a result of reviewer feedback when
I originally wrote it. What would you recommend instead?

[Ryan Sleevi, 2016/09/17 01:51:52]

Link to previous CL?

[Rob Percival, 2016/09/20 18:54:42]

https://codereview.chromium.org/2066553002/#ps180001

[Ryan Sleevi, 2016/09/22 04:37:07]

Thanks. I think there was a breakdown in communication there.

I agree with Matt that RunUntilIdle() is not a safe pattern to endorse here, for
all the reasons he mentioned. The core of the 'surprise' here, which required
digging into, is that you create a nested RunLoop context each time you create a
MockLeafIndexCallback, which thus implies an ordering dependency that's not
actually related.

Compare with net::TestCompletionCallback, which creates a RunLoop on-demand to
execute until task completion, unless the task has already completed. You can
create as many net::TestCompletionCallback's as necessary (which do not create
any RunLoops), and it's not until you attempt to WaitForResult that a RunLoop is
created (and only if the result is not already available).

Further, this code is also confusing, in that it's non-obvious that
WaitUntilRun() will ever return if a callback is not called (c.f. line 603).
There's nothing that will ever quit the specific callback runloop until it's Run
- that is, you could NEVER hit 602/603 if 599 succeeded. And if it didn't
succeed, it would run until eternity.

In terms of concrete suggestions for how to change this, I think we should
revisit this after we've adapted LogDnsClient to a more natural form of async
programming, and then see if there are concrete suggestions to be made here.

[Rob Percival, 2016/09/22 11:33:23]

Ah I see, TestCompletionCallback has a unique_ptr<RunLoop> and defers creating
the RunLoop. Using RunUntilIdle, rather than Run, might actually be an
improvement WRT ensuring WaitUntilRun terminates (though it's still not
guaranteed). However, I won't change anything until the async-sync pattern CL as
you suggest.

+}
+
+TEST_P(LogDnsClientTest, CanPerformAuditProofQueriesInParallel) {
+  // Check that 3 audit proof queries can be performed in parallel.
+  constexpr size_t kNumOfParallelQueries = 3;
+  std::unique_ptr<LogDnsClient> log_client =
+      CreateLogDnsClient(kNumOfParallelQueries);
+  MockAuditProofCallback callbacks[kNumOfParallelQueries];
+  const std::vector<std::string> audit_proof = GetSampleAuditProof(20);
+
+  // It should require 3 requests to collect the entire audit proof, as there is
+  // only space for 7 nodes per TXT record. One node is 32 bytes long and the
+  // TXT RDATA can have a maximum length of 255 bytes (255 / 32). Expect each of
+  // these queries to occur once for each call to QueryAuditProof.
+
+  for (size_t i = 0; i < kNumOfParallelQueries; ++i) {
+    mock_dns_.ExpectAuditProofRequestAndResponse(
+        "0.123456.999999.tree.ct.test.", audit_proof.begin(),
+        audit_proof.begin() + 7);
+  }
+
+  for (size_t i = 0; i < kNumOfParallelQueries; ++i) {
+    mock_dns_.ExpectAuditProofRequestAndResponse(
+        "7.123456.999999.tree.ct.test.", audit_proof.begin() + 7,
+        audit_proof.begin() + 14);
+  }
+
+  for (size_t i = 0; i < kNumOfParallelQueries; ++i) {
+    mock_dns_.ExpectAuditProofRequestAndResponse(
+        "14.123456.999999.tree.ct.test.", audit_proof.begin() + 14,
+        audit_proof.end());
+  }
+
+  // Start the queries.
+  for (size_t i = 0; i < kNumOfParallelQueries; ++i) {
+    QueryAuditProofAsync(log_client.get(), "ct.test", 123456, 999999,
+                         callbacks[i].AsCallback());
+  }
+
+  // Wait for each query to complete and check its results.
+  for (size_t i = 0; i < kNumOfParallelQueries; ++i) {
+    MockAuditProofCallback& callback = callbacks[i];
+    callbacks[i].WaitUntilRun();
+
+    SCOPED_TRACE(i);
+    EXPECT_TRUE(callback.called());
+    if (callback.called()) {
+      EXPECT_THAT(callback.net_error(), IsOk());
+      EXPECT_THAT(callback.proof(), NotNull());
+      if (callback.proof() != nullptr) {
+        EXPECT_THAT(callback.proof()->leaf_index, Eq(123456u));
+        // TODO(robpercival): Enable this once MerkleAuditProof has tree_size.
+        // EXPECT_THAT(callback.proof()->tree_size, Eq(999999));
+        EXPECT_THAT(callback.proof()->nodes, Eq(audit_proof));
+      }
+    }
+  }
+}
+
+TEST_P(LogDnsClientTest, CanPerformLeafIndexAndAuditProofQueriesInParallel) {
+  // Check that a leaf index and audit proof query can be performed in parallel.
+  constexpr size_t kNumOfParallelQueries = 2;
+  std::unique_ptr<LogDnsClient> log_client =
+      CreateLogDnsClient(kNumOfParallelQueries);
+  MockLeafIndexCallback leaf_index_callback;
+  MockAuditProofCallback audit_proof_callback;
+  const std::vector<std::string> audit_proof = GetSampleAuditProof(20);
+
+  mock_dns_.ExpectLeafIndexRequestAndResponse(
+      "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
+      "123456");
+
+  // It should require 3 requests to collect the entire audit proof, as there is
+  // only space for 7 nodes per TXT record. One node is 32 bytes long and the
+  // TXT RDATA can have a maximum length of 255 bytes (255 / 32).
+  mock_dns_.ExpectAuditProofRequestAndResponse("0.123456.999999.tree.ct.test.",
+                                               audit_proof.begin(),
+                                               audit_proof.begin() + 7);
+  mock_dns_.ExpectAuditProofRequestAndResponse("7.123456.999999.tree.ct.test.",
+                                               audit_proof.begin() + 7,
+                                               audit_proof.begin() + 14);
+  mock_dns_.ExpectAuditProofRequestAndResponse("14.123456.999999.tree.ct.test.",
+                                               audit_proof.begin() + 14,
+                                               audit_proof.end());
+
+  // Start the queries.
+  QueryLeafIndexAsync(log_client.get(), "ct.test", kLeafHash,
+                      leaf_index_callback.AsCallback());
+  QueryAuditProofAsync(log_client.get(), "ct.test", 123456, 999999,
+                       audit_proof_callback.AsCallback());
+
+  // Wait for the queries to complete, then check their results.
+  leaf_index_callback.WaitUntilRun();
+  audit_proof_callback.WaitUntilRun();
+
+  EXPECT_TRUE(leaf_index_callback.called());
+  if (leaf_index_callback.called()) {
+    EXPECT_THAT(leaf_index_callback.net_error(), IsOk());
+    EXPECT_THAT(leaf_index_callback.leaf_index(), Eq(123456u));
+  }
+
+  EXPECT_TRUE(audit_proof_callback.called());
+  if (audit_proof_callback.called()) {
+    EXPECT_THAT(audit_proof_callback.net_error(), IsOk());
+    EXPECT_THAT(audit_proof_callback.proof(), NotNull());
+    if (audit_proof_callback.proof() != nullptr) {
+      EXPECT_THAT(audit_proof_callback.proof()->leaf_index, Eq(123456u));
+      // TODO(robpercival): Enable this once MerkleAuditProof has tree_size.
+      // EXPECT_THAT(audit_proof_callback.proof()->tree_size, Eq(999999));
+      EXPECT_THAT(audit_proof_callback.proof()->nodes, Eq(audit_proof));
+    }
+  }
+}
+
+TEST_P(LogDnsClientTest, CanBeThrottledToOneLeafIndexQueryAtATime) {
+  // Check that leaf index queries can be rate-limited to one at a time.
+  // The second query, initiated while the first is in progress, should fail.
+  mock_dns_.ExpectLeafIndexRequestAndResponse(
+      "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
+      "123456");
+
+  const size_t max_concurrent_queries = 1;
+  std::unique_ptr<LogDnsClient> log_client =
+      CreateLogDnsClient(max_concurrent_queries);
+
+  // Start the queries.
+  MockLeafIndexCallback callback1;
+  QueryLeafIndexAsync(log_client.get(), "ct.test", kLeafHash,
+                      callback1.AsCallback());
+  MockLeafIndexCallback callback2;
+  QueryLeafIndexAsync(log_client.get(), "ct.test", kLeafHash,
+                      callback2.AsCallback());
+
+  callback1.WaitUntilRun();
+  callback2.WaitUntilRun();
+
+  // Check that the first query succeeded.
+  EXPECT_TRUE(callback1.called());
+  if (callback1.called()) {

[Ryan Sleevi, 2016/09/16 02:11:03]

This is non-obvious to me why you have an EXPECT_TRUE but then subsequently
EXPECT_THAT; why isn't this just ASSERT_TRUE ?

That is, put differently, "Expect that this code will be called, but then check
if we weren't called" is... unclear to me the testing value. Failing faster in
the face of failure seems more beneficial.

[Rob Percival, 2016/09/16 16:28:19]

It's actually "Expect that this code will be called, *and* then check
if we *were* called ...". It's because I'd like a failure of this test to tell
me which of the two callbacks, if any, were called, rather than just telling me
that the first one wasn't. This requires that I use EXPECT_*. Only if a callback
has been called, is it then valid to check what it was called with.

[Ryan Sleevi, 2016/09/17 01:51:52]

Why?

In the face of failure, what new and additional information do you gain, versus
the complexity in trying to figure out whether this is intentional or if it's an
anti-pattern like

DCHECK(thing)
if (thing)

[The above is prohibited]

[Rob Percival, 2016/09/20 18:54:42]

As discussed in
https://testing.googleblog.com/2008/07/tott-expect-vs-assert.html, EXPECT gives
you more information about why a test is failing. In this case, it's potentially
useful to know that callback2 was called but callback1 wasn't, as that could
hint at what the bug is. DCHECK and EXPECT should not be confused, as the former
expresses a condition that should result in immediate termination, whereas
EXPECT does not. I could expand the comment to hopefully avoid such confusion if
you'd like?

[Ryan Sleevi, 2016/09/22 04:37:07]

Apologies that I worded the question poorly, and perhaps did not give a clearer
understanding of why I have considerable unease with this, to the point I'm not
comfortable approving it as written. Concretely, I would prefer you write this
as:

ASSERT_TRUE(callback1.called());
EXPECT_THAT(callback1.net_error(), IsOk());
EXPECT_THAT(callback1.leaf_index(), Eq(123456u));

ASSERT_TRUE(callback2.called());
EXPECT_THAT(callback2.net_error(), IsError(net::ERR_TEMPORARILY_THROTTLED));
EXPECT_THAT(callback2.leaf_index(), Eq(0u));


My reasoning is simple:
1) I do not believe that the distinction (knowing that callback1 is not called,
but callback2 is) is relevant or useful information to help diagnose the test.
2) Using that TOTT as an example, I do not believe it makes any sense to
continue if callback1 has failed. The test will fail, and knowing information
about callback2 is irrelevant (and likely to result in more noise than use).
3) Conditionals such as this are like 'debugging breadcrumbs'. Expect strong
push back from me on any use of debugging breadcrumbs in Chrome code, as I do
not believe in their value, from working on Chrome and other codebases. They add
complexity (to reading, to writing, and to maintaining), their cost is not
visible to the author (because they don't have to support it), and it's a bit
like 'forward engineering' - you never know what debugging breadcrumbs are
necessary until you're debugging, and you can't be sure what was necessary this
time will be useful in the future.

The above rewrite makes it clear that both callback1 and callback2 are expected
to be called, but that there's no point in continuing the test if they're not.

The only possible information that can be gleaned from this is that callback2
was called when callback1 was not called, but that's readily visible to anyone
with a debugger, or can easily be investigated if they fail.

To be clear, this logic applies to just about every tuple of
EXPECT_THAT(conditional) + if(conditional) - I am not comfortable with any one
of them in this CL, for the reasons above.

[Rob Percival, 2016/09/22 11:33:23]

Very well. I'm not entirely convinced, but I'll defer to your experience.

+    EXPECT_THAT(callback1.net_error(), IsOk());
+    EXPECT_THAT(callback1.leaf_index(), Eq(123456u));
+  }
+
+  // Check that the second query failed.
+  EXPECT_TRUE(callback2.called());
+  if (callback2.called()) {
+    EXPECT_THAT(callback2.net_error(), IsError(net::ERR_TEMPORARILY_THROTTLED));
+    EXPECT_THAT(callback2.leaf_index(), Eq(0u));
+  }
+}
+
+TEST_P(LogDnsClientTest, CanBeThrottledToOneAuditProofQueryAtATime) {
+  // Check that audit proof queries can be rate-limited to one at a time.
+  // The second query, initiated while the first is in progress, should fail.
+  const std::vector<std::string> audit_proof = GetSampleAuditProof(20);
+
+  // It should require 3 requests to collect the entire audit proof, as there is
+  // only space for 7 nodes per TXT record. One node is 32 bytes long and the
+  // TXT RDATA can have a maximum length of 255 bytes (255 / 32).
+  // Rate limiting should not interfere with these requests.
+  mock_dns_.ExpectAuditProofRequestAndResponse("0.123456.999999.tree.ct.test.",
+                                               audit_proof.begin(),
+                                               audit_proof.begin() + 7);
+  mock_dns_.ExpectAuditProofRequestAndResponse("7.123456.999999.tree.ct.test.",
+                                               audit_proof.begin() + 7,
+                                               audit_proof.begin() + 14);
+  mock_dns_.ExpectAuditProofRequestAndResponse("14.123456.999999.tree.ct.test.",
+                                               audit_proof.begin() + 14,
+                                               audit_proof.end());
+
+  const size_t max_concurrent_queries = 1;
+  std::unique_ptr<LogDnsClient> log_client =
+      CreateLogDnsClient(max_concurrent_queries);
+
+  // Start the queries.
+  MockAuditProofCallback callback1;
+  QueryAuditProofAsync(log_client.get(), "ct.test", 123456, 999999,
+                       callback1.AsCallback());
+  MockAuditProofCallback callback2;
+  QueryAuditProofAsync(log_client.get(), "ct.test", 123456, 999999,
+                       callback2.AsCallback());
+
+  callback1.WaitUntilRun();
+  callback2.WaitUntilRun();
+
+  // Check that the first query succeeded.
+  EXPECT_TRUE(callback1.called());
+  if (callback1.called()) {
+    EXPECT_THAT(callback1.net_error(), IsOk());
+    EXPECT_THAT(callback1.proof(), NotNull());
+    if (callback1.proof() != nullptr) {
+      EXPECT_THAT(callback1.proof()->leaf_index, Eq(123456u));
+      // TODO(robpercival): Enable this once MerkleAuditProof has tree_size.
+      // EXPECT_THAT(callback1.proof()->tree_size, Eq(999999));
+      EXPECT_THAT(callback1.proof()->nodes, Eq(audit_proof));
+    }
+  }
+
+  // Check that the second query failed.
+  EXPECT_TRUE(callback2.called());
+  if (callback2.called()) {
+    EXPECT_THAT(callback2.net_error(), IsError(net::ERR_TEMPORARILY_THROTTLED));
+    EXPECT_THAT(callback2.proof(), IsNull());
+  }
+}
+
+TEST_P(LogDnsClientTest, ThrottlingAppliesAcrossQueryTypes) {
+  // Check that queries can be rate-limited to one at a time, regardless of the
+  // type of query. The second query, initiated while the first is in progress,
+  // should fail.
+  mock_dns_.ExpectLeafIndexRequestAndResponse(
+      "D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
+      "123456");
+
+  const size_t max_concurrent_queries = 1;
+  std::unique_ptr<LogDnsClient> log_client =
+      CreateLogDnsClient(max_concurrent_queries);
+
+  // Start the queries.
+  MockLeafIndexCallback leaf_index_callback;
+  QueryLeafIndexAsync(log_client.get(), "ct.test", kLeafHash,
+                      leaf_index_callback.AsCallback());
+  MockAuditProofCallback audit_proof_callback;
+  QueryAuditProofAsync(log_client.get(), "ct.test", 123456, 999999,
+                       audit_proof_callback.AsCallback());
+
+  leaf_index_callback.WaitUntilRun();
+  audit_proof_callback.WaitUntilRun();
+
+  // Check that the first query succeeded.
+  EXPECT_TRUE(leaf_index_callback.called());
+  if (leaf_index_callback.called()) {
+    EXPECT_THAT(leaf_index_callback.net_error(), IsOk());
+    EXPECT_THAT(leaf_index_callback.leaf_index(), Eq(123456u));
+  }
+
+  // Check that the second query failed.
+  EXPECT_TRUE(audit_proof_callback.called());
+  if (audit_proof_callback.called()) {
+    EXPECT_THAT(audit_proof_callback.net_error(),
+                IsError(net::ERR_TEMPORARILY_THROTTLED));
+    EXPECT_THAT(audit_proof_callback.proof(), IsNull());
+  }
+}
+
 INSTANTIATE_TEST_CASE_P(ReadMode,
                         LogDnsClientTest,
                         ::testing::Values(net::IoMode::ASYNC,
                                           net::IoMode::SYNCHRONOUS));
 
 }  // namespace
 }  // namespace certificate_transparency