Allow LogDnsClient queries to be rate-limited

components/certificate_transparency/log_dns_client.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 <sstream>
 
 #include "base/bind.h"
 #include "base/location.h"
@@ skipping 64 matching lines @@
   for (size_t i = 0; i < audit_path.size(); i += crypto::kSHA256Length) {
     proof->nodes.push_back(audit_path.substr(i, crypto::kSHA256Length));
   }
 
   return true;
 }
 
 }  // 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);
   UpdateDnsConfig();
 }
 
 LogDnsClient::~LogDnsClient() {
   net::NetworkChangeNotifier::RemoveDNSObserver(this);
 }
 
 void LogDnsClient::OnDNSChanged() {
   UpdateDnsConfig();
 }
 
 void LogDnsClient::OnInitialDNSConfigRead() {
   UpdateDnsConfig();
 }
 
 void LogDnsClient::QueryLeafIndex(base::StringPiece domain_for_log,
                                   base::StringPiece leaf_hash,
                                   const LeafIndexCallback& callback) {
   if (domain_for_log.empty() || leaf_hash.size() != crypto::kSHA256Length) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(callback, net::Error::ERR_INVALID_ARGUMENT, 0));
     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);
 
   net::DnsTransactionFactory* factory = dns_client_->GetTransactionFactory();
   if (factory == nullptr) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::Bind(callback, net::Error::ERR_NAME_RESOLUTION_FAILED, 0));
     return;
@@ skipping 14 matching lines @@
 }
 
 // The performance of this could be improved by sending all of the expected
 // queries up front. Each response can contain a maximum of 7 audit path nodes,
 // so for an audit proof of size 20, it could send 3 queries (for nodes 0-6,
 // 7-13 and 14-19) immediately. Currently, it sends only the first and then,
 // based on the number of nodes received, sends the next query. The complexity
 // 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,
                                    const AuditProofCallback& callback) {
   if (domain_for_log.empty() || leaf_index >= tree_size) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::Bind(callback, net::Error::ERR_INVALID_ARGUMENT, nullptr));
     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;
   // TODO(robpercival): Once a "tree_size" field is added to MerkleAuditProof,
   // pass |tree_size| to QueryAuditProofNodes using that.
 
   // Query for the first batch of audit proof nodes (i.e. starting from 0).
   QueryAuditProofNodes(std::move(proof), domain_for_log, tree_size, 0,
                        callback);
 }
@@ skipping 126 matching lines @@
     QueryAuditProofNodes(std::move(proof), domain_for_log, tree_size,
                          audit_path_nodes_received, query.callback);
     return;
   }
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       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);
   if (config.IsValid())
     dns_client_->SetConfig(config);
 }
 
 }  // namespace certificate_transparency