Add the ability for MDnsListener to actively refresh records

net/dns/mdns_client_impl.cc

 // Copyright 2013 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 "net/dns/mdns_client_impl.h"
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/stl_util.h"
 #include "base/time/default_clock.h"
 #include "net/base/dns_util.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/base/rand_callback.h"
 #include "net/dns/dns_protocol.h"
 #include "net/dns/record_rdata.h"
 #include "net/udp/datagram_socket.h"
 
 // TODO(gene): Remove this temporary method of disabling NSEC support once it
 // becomes clear whether this feature should be
 // supported. http://crbug.com/255232
 #define ENABLE_NSEC
 
 namespace net {
 
 namespace {
 
 const unsigned MDnsTransactionTimeoutSeconds = 3;
+const double kListenerRefreshPercentage1 = 0.85;

[szym, 2014/01/23 11:24:03]

Percentage suggests that this is divided by 100, i.e., percentage=0.85 means
0.85% or 0.0085. Suggest ListenerRefreshRatio.

Even then the name is unclear so this needs a comment.

// Refresh ratio is the time before the record expiration at which
// the listener should refresh it expressed as a fraction of
// the record TTL. For example, with refresh ratio of 0.85, a record
// with announced TTL of 100s should be refreshed 15s before expiration.

What's the reasoning for using time relative to TTL instead of absolute? I'm
concerned that the TTL received in DNS response is always the remaining TTL, so
it will be less if it's sent from a cache.

[Noam Samuel, 2014/01/23 17:50:42]

The time is relative to the record's original TTL, as per the mDNS spec. 
RecordParsed has separate fields for ttl and time_created so that the full
information on a packet's expiration time is maintained (which is another work
item for this CL, I don't actually use time_created, but will fix that).

+const double kListenerRefreshPercentage2 = 0.95;
+const unsigned kMillisecondsPerSecond = 1000;
 
 }  // namespace
 
 void MDnsSocketFactoryImpl::CreateSockets(
     ScopedVector<DatagramServerSocket>* sockets) {
   InterfaceIndexFamilyList interfaces(GetMDnsInterfacesToBind());
   for (size_t i = 0; i < interfaces.size(); ++i) {
     DCHECK(interfaces[i].second == net::ADDRESS_FAMILY_IPV4 ||
            interfaces[i].second == net::ADDRESS_FAMILY_IPV6);
     scoped_ptr<DatagramServerSocket> socket(
@@ skipping 146 matching lines @@
 
   return connection_->Send(query.io_buffer(), query.io_buffer()->size());
 }
 
 void MDnsClientImpl::Core::HandlePacket(DnsResponse* response,
                                         int bytes_read) {
   unsigned offset;
   // Note: We store cache keys rather than record pointers to avoid
   // erroneous behavior in case a packet contains multiple exclusive
   // records with the same type and name.
-  std::map<MDnsCache::Key, MDnsListener::UpdateType> update_keys;
+  std::map<MDnsCache::Key, MDnsCache::UpdateType> update_keys;
 
   if (!response->InitParseWithoutQuery(bytes_read)) {
     LOG(WARNING) << "Could not understand an mDNS packet.";
     return;  // Message is unreadable.
   }
 
   // TODO(noamsml): duplicate query suppression.
   if (!(response->flags() & dns_protocol::kFlagResponse))
     return;  // Message is a query. ignore it.
 
@@ skipping 22 matching lines @@
       LOG(WARNING) << "Received an mDNS record with non-IN class. Ignoring.";
       continue;  // Ignore all records not in the IN class.
     }
 
     MDnsCache::Key update_key = MDnsCache::Key::CreateFor(record.get());
     MDnsCache::UpdateType update = cache_.UpdateDnsRecord(record.Pass());
 
     // Cleanup time may have changed.
     ScheduleCleanup(cache_.next_expiration());
 
-    if (update != MDnsCache::NoChange) {
-      MDnsListener::UpdateType update_external;
-
-      switch (update) {
-        case MDnsCache::RecordAdded:
-          update_external = MDnsListener::RECORD_ADDED;
-          break;
-        case MDnsCache::RecordChanged:
-          update_external = MDnsListener::RECORD_CHANGED;
-          break;
-        case MDnsCache::NoChange:
-        default:
-          NOTREACHED();
-          // Dummy assignment to suppress compiler warning.
-          update_external = MDnsListener::RECORD_CHANGED;
-          break;
-      }
-
-      update_keys.insert(std::make_pair(update_key, update_external));
-    }
+    update_keys.insert(std::make_pair(update_key, update));
   }
 
-  for (std::map<MDnsCache::Key, MDnsListener::UpdateType>::iterator i =
+  for (std::map<MDnsCache::Key, MDnsCache::UpdateType>::iterator i =
            update_keys.begin(); i != update_keys.end(); i++) {
     const RecordParsed* record = cache_.LookupKey(i->first);
     if (!record)
       continue;
 
     if (record->type() == dns_protocol::kTypeNSEC) {
 #if defined(ENABLE_NSEC)
       NotifyNsecRecord(record);
 #endif
     } else {
@@ skipping 33 matching lines @@
       FOR_EACH_OBSERVER(MDnsListenerImpl, *i->second, AlertNsecRecord());
     }
   }
 }
 
 void MDnsClientImpl::Core::OnConnectionError(int error) {
   // TODO(noamsml): On connection error, recreate connection and flush cache.
 }
 
 void MDnsClientImpl::Core::AlertListeners(
-    MDnsListener::UpdateType update_type,
+    MDnsCache::UpdateType update_type,
     const ListenerKey& key,
     const RecordParsed* record) {
   ListenerMap::iterator listener_map_iterator = listeners_.find(key);
   if (listener_map_iterator == listeners_.end()) return;
 
   FOR_EACH_OBSERVER(MDnsListenerImpl, *listener_map_iterator->second,
-                    AlertDelegate(update_type, record));
+                    HandleRecordUpdate(update_type, record));
 }
 
 void MDnsClientImpl::Core::AddListener(
     MDnsListenerImpl* listener) {
   ListenerKey key(listener->GetName(), listener->GetType());
   std::pair<ListenerMap::iterator, bool> observer_insert_result =
       listeners_.insert(
           make_pair(key, static_cast<ObserverList<MDnsListenerImpl>*>(NULL)));
 
   // If an equivalent key does not exist, actually create the observer list.
@@ skipping 53 matching lines @@
 
 void MDnsClientImpl::Core::DoCleanup() {
   cache_.CleanupRecords(base::Time::Now(), base::Bind(
       &MDnsClientImpl::Core::OnRecordRemoved, base::Unretained(this)));
 
   ScheduleCleanup(cache_.next_expiration());
 }
 
 void MDnsClientImpl::Core::OnRecordRemoved(
     const RecordParsed* record) {
-  AlertListeners(MDnsListener::RECORD_REMOVED,
+  AlertListeners(MDnsCache::RecordRemoved,
                  ListenerKey(record->name(), record->type()), record);
 }
 
 void MDnsClientImpl::Core::QueryCache(
     uint16 rrtype, const std::string& name,
     std::vector<const RecordParsed*>* records) const {
   cache_.FindDnsRecords(rrtype, name, records, base::Time::Now());
 }
 
 MDnsClientImpl::MDnsClientImpl() {
@@ skipping 36 matching lines @@
   return scoped_ptr<MDnsTransaction>(
       new MDnsTransactionImpl(rrtype, name, flags, callback, this));
 }
 
 MDnsListenerImpl::MDnsListenerImpl(
     uint16 rrtype,
     const std::string& name,
     MDnsListener::Delegate* delegate,
     MDnsClientImpl* client)
     : rrtype_(rrtype), name_(name), client_(client), delegate_(delegate),
-      started_(false) {
+      started_(false), active_refresh_(false) {
+}
+
+MDnsListenerImpl::~MDnsListenerImpl() {
+  if (started_) {
+    DCHECK(client_->core());
+    client_->core()->RemoveListener(this);
+  }
 }
 
 bool MDnsListenerImpl::Start() {
   DCHECK(!started_);
 
   started_ = true;
 
   DCHECK(client_->core());
   client_->core()->AddListener(this);
 
   return true;
 }
 
-MDnsListenerImpl::~MDnsListenerImpl() {
+void MDnsListenerImpl::SetActiveRefresh(bool active_refresh) {
+  active_refresh_ = active_refresh;
+
   if (started_) {
-    DCHECK(client_->core());
-    client_->core()->RemoveListener(this);
+    if (!active_refresh_) {
+      next_refresh_.Cancel();
+    } else if (last_update_ != base::Time()) {
+      ScheduleNextRefresh();
+    }
   }
 }
 
 const std::string& MDnsListenerImpl::GetName() const {
   return name_;
 }
 
 uint16 MDnsListenerImpl::GetType() const {
   return rrtype_;
 }
 
-void MDnsListenerImpl::AlertDelegate(MDnsListener::UpdateType update_type,
-                                     const RecordParsed* record) {
+void MDnsListenerImpl::HandleRecordUpdate(MDnsCache::UpdateType update_type,
+                                          const RecordParsed* record) {
   DCHECK(started_);
-  delegate_->OnRecordUpdate(update_type, record);
+
+  if (update_type != MDnsCache::RecordRemoved) {
+    ttl_ = record->ttl();
+    last_update_ = base::Time::Now();
+
+    ScheduleNextRefresh();
+  }
+
+  if (update_type != MDnsCache::NoChange) {
+    MDnsListener::UpdateType update_external;
+
+    switch (update_type) {
+      case MDnsCache::RecordAdded:
+        update_external = MDnsListener::RECORD_ADDED;
+        break;
+      case MDnsCache::RecordChanged:
+        update_external = MDnsListener::RECORD_CHANGED;
+        break;
+      case MDnsCache::RecordRemoved:
+        update_external = MDnsListener::RECORD_REMOVED;
+        break;
+      case MDnsCache::NoChange:
+      default:
+        NOTREACHED();
+        // Dummy assignment to suppress compiler warning.
+        update_external = MDnsListener::RECORD_CHANGED;
+        break;
+    }
+
+    delegate_->OnRecordUpdate(update_external, record);
+  }
 }
 
 void MDnsListenerImpl::AlertNsecRecord() {
   DCHECK(started_);
   delegate_->OnNsecRecord(name_, rrtype_);
 }
 
+void MDnsListenerImpl::ScheduleNextRefresh() {

[szym, 2014/01/23 11:24:03]

This function silently assumes that if active_refresh_ then last_update_ !=
Time().

I suggest you either add DCHECK or explicit check and exit if last_update_ ==
Time().

[Noam Samuel, 2014/01/23 17:50:42]

Done.

+  if (active_refresh_) {

[szym, 2014/01/23 11:24:03]

By convention this should be:
if (!active_refresh_)
  return;
...

[Noam Samuel, 2014/01/23 17:50:42]

Done.

+    // A zero TTL is a goodbye packet and should not be refreshed.
+    if (ttl_ == 0) {
+      next_refresh_.Cancel();
+      return;
+    }
+
+    next_refresh_.Reset(base::Bind(&MDnsListenerImpl::DoRefresh,
+                                   AsWeakPtr()));
+
+    base::Time next_refresh1 = last_update_ + base::TimeDelta::FromMilliseconds(
+        static_cast<int>(kMillisecondsPerSecond *
+                         kListenerRefreshPercentage1 * ttl_));
+
+    base::Time next_refresh2 = last_update_ + base::TimeDelta::FromMilliseconds(

[szym, 2014/01/23 11:24:03]

You need a comment somewhere why two refresh tasks are scheduled at the same
time.

[Noam Samuel, 2014/01/23 17:50:42]

Done.

+        static_cast<int>(kMillisecondsPerSecond *
+                         kListenerRefreshPercentage2 * ttl_));
+
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        next_refresh_.callback(),
+        next_refresh1 - base::Time::Now());
+
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        next_refresh_.callback(),
+        next_refresh2 - base::Time::Now());
+  }
+}
+
+void MDnsListenerImpl::DoRefresh() {
+  client_->core()->SendQuery(rrtype_, name_);
+}
+
 MDnsTransactionImpl::MDnsTransactionImpl(
     uint16 rrtype,
     const std::string& name,
     int flags,
     const MDnsTransaction::ResultCallback& callback,
     MDnsClientImpl* client)
     : rrtype_(rrtype), name_(name), callback_(callback), client_(client),
       started_(false), flags_(flags) {
   DCHECK((flags_ & MDnsTransaction::FLAG_MASK) == flags_);
   DCHECK(flags_ & MDnsTransaction::QUERY_CACHE ||
@@ skipping 128 matching lines @@
 
 void MDnsTransactionImpl::OnNsecRecord(const std::string& name, unsigned type) {
   TriggerCallback(RESULT_NSEC, NULL);
 }
 
 void MDnsTransactionImpl::OnCachePurged() {
   // TODO(noamsml): Cache purge situations not yet implemented
 }
 
 }  // namespace net