Allow LogDnsClient queries to be rate-limited

components/certificate_transparency/log_dns_client.cc

Index: components/certificate_transparency/log_dns_client.cc
diff --git a/components/certificate_transparency/log_dns_client.cc b/components/certificate_transparency/log_dns_client.cc
index 5fb28b973eb7b431532fa677777810973ccfc2d1..1607d4637d3555faf305ee14c85e27763154afe0 100644
--- a/components/certificate_transparency/log_dns_client.cc
+++ b/components/certificate_transparency/log_dns_client.cc
@@ -82,9 +82,11 @@ bool ParseAuditPath(const net::DnsResponse& response,
 }  // namespace
 
 LogDnsClient::LogDnsClient(std::unique_ptr<net::DnsClient> dns_client,
-                           const net::BoundNetLog& net_log)
+                           const net::BoundNetLog& net_log,
+                           size_t max_concurrent_queries)
     : dns_client_(std::move(dns_client)),
       net_log_(net_log),
+      max_concurrent_queries_(max_concurrent_queries),
       weak_ptr_factory_(this) {
   CHECK(dns_client_);
   net::NetworkChangeNotifier::AddDNSObserver(this);
@@ -112,6 +114,13 @@ void LogDnsClient::QueryLeafIndex(base::StringPiece domain_for_log,
     return;
   }
 
+  if (HasMaxConcurrentQueriesInProgress()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, net::Error::ERR_TEMPORARILY_THROTTLED, 0));

[Ryan Sleevi, 2016/09/12 18:12:43]

The downside to this approach is that it forces a thread sleep/wakeup,
potentially, for what could be a synchronous error handling.

Most of the //net APIs try to follow a sync-or-async pattern that lends itself
naturally into forcing a state machine progression style loop. In that model,
the overhead of sync-or-async should be eliminated, with the benefit that it
reduces overheads.

To that end, I suppose it equally applies to 111-115 (and other error returns).

That seems like a much larger change, so I can certainly buy that we should be
treating that as separate; if this was the first instance of a sync return, I
would say in this CL, but since we have pre-existing code, we should try to
update it.

[Rob Percival, 2016/09/13 14:06:31]

I originally began implementing this class using that pattern, but found it to
be less clear given there are two distinct state machines in this class.
Breaking the state machines out into LeafIndexQuery and AuditProofQuery classes,
using that pattern for both, and gutting this down to just tracking the DNS
config and constructing instances of those classes might have been better, but
seemed overly complex. I could do something like that in a separate CL though if
you think it's worth it (or just adapt this class to have a single, mixed state
machine).

On a related note, should I update the comment for ERR_TEMPORARILY_THROTTLED at
https://cs.chromium.org/chromium/src/net/base/net_error_list.h?l=250 to make it
less specific (currently talks about the "request throttler module")? There's
another error code, ERR_HOST_RESOLVER_QUEUE_TOO_LARGE, which could alternatively
be used, but that too suffers from being a bit too specialised.

+    return;
+  }
+
   std::string encoded_leaf_hash =
       base32::Base32Encode(leaf_hash, base32::Base32EncodePolicy::OMIT_PADDING);
   DCHECK_EQ(encoded_leaf_hash.size(), 52u);
@@ -146,7 +155,8 @@ void LogDnsClient::QueryLeafIndex(base::StringPiece domain_for_log,
 // of the code would increase though, as it would need to detect gaps in the
 // audit proof caused by the server not responding with the anticipated number
 // of nodes. Ownership of the proof would need to change, as it would be shared
-// between simultaneous DNS transactions.
+// between simultaneous DNS transactions. Throttling of queries would also need
+// to take into account this increase in parallelism.
 void LogDnsClient::QueryAuditProof(base::StringPiece domain_for_log,
                                    uint64_t leaf_index,
                                    uint64_t tree_size,
@@ -158,6 +168,13 @@ void LogDnsClient::QueryAuditProof(base::StringPiece domain_for_log,
     return;
   }
 
+  if (HasMaxConcurrentQueriesInProgress()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, net::Error::ERR_TEMPORARILY_THROTTLED, nullptr));
+    return;
+  }
+
   std::unique_ptr<net::ct::MerkleAuditProof> proof(
       new net::ct::MerkleAuditProof);
   proof->leaf_index = leaf_index;
@@ -304,6 +321,14 @@ void LogDnsClient::QueryAuditProofNodesComplete(
       base::Bind(query.callback, net::OK, base::Passed(std::move(proof))));
 }
 
+bool LogDnsClient::HasMaxConcurrentQueriesInProgress() const {
+  const size_t queries_in_progress =
+      leaf_index_queries_.size() + audit_proof_queries_.size();
+
+  return max_concurrent_queries_ != 0 &&
+         queries_in_progress >= max_concurrent_queries_;
+}
+
 void LogDnsClient::UpdateDnsConfig() {
   net::DnsConfig config;
   net::NetworkChangeNotifier::GetDnsConfig(&config);