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..2bbc17b432f05feb59109d6009f94d4d8f701b93 100644
--- a/components/certificate_transparency/log_dns_client.cc
+++ b/components/certificate_transparency/log_dns_client.cc
@@ -85,6 +85,7 @@ LogDnsClient::LogDnsClient(std::unique_ptr<net::DnsClient> dns_client,
                            const net::BoundNetLog& net_log)
     : dns_client_(std::move(dns_client)),
       net_log_(net_log),
+      max_concurrent_queries_(0),
       weak_ptr_factory_(this) {
   CHECK(dns_client_);
   net::NetworkChangeNotifier::AddDNSObserver(this);
@@ -112,6 +113,13 @@ void LogDnsClient::QueryLeafIndex(base::StringPiece domain_for_log,
     return;
   }
 
+  if (HasMaxConcurrentQueriesInProgress()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(

[Eran Messeri, 2016/09/12 13:26:48]

Rather than invoke the callback, I suggest returning a value here to indicate
the request has not been queued. This would simplify handling of this case in
the client - i.e. the caller could immediately make a decision on what to do
with the request given it's throttled, rather than having to wait with the
decision until the callback is involved. So easier reasoning and more
straightforward handling of this case (also less code).

[Rob Percival, 2016/09/12 17:26:24]

Would you suggest the other errors that can be returned synchronously
(ERR_INVALID_ARGUMENT, ERR_NAME_RESOLUTION_FAILED) also be returned in this way?

I thought it was simpler having a single way to report errors (the callback),
especially since the likely response to being throttled (retry later) is the
same response you might want to take to other errors. However, if you aren't
worrying about retrying in those other scenarios, I can imagine that knowing
you're being throttled immediately could simplify calling code.

[Eran Messeri, 2016/09/14 23:25:42]

Yes, I would suggest other errors are also returned this way.
It does simplify calling code, since as you pointed out, the caller would simply
stop calling this method.
Other errors should definitely not be re-tried - if the caller is not positive
that the return value indicates throttling, it may be a usage error (it could
DCHECK or simply drop the request).

+        FROM_HERE,
+        base::Bind(callback, net::Error::ERR_TEMPORARILY_THROTTLED, 0));
+    return;
+  }
+
   std::string encoded_leaf_hash =
       base32::Base32Encode(leaf_hash, base32::Base32EncodePolicy::OMIT_PADDING);
   DCHECK_EQ(encoded_leaf_hash.size(), 52u);
@@ -146,7 +154,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 +167,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 +320,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);