DNS: Retain stale entries in HostCache and return when requested

net/dns/host_cache.h

Index: net/dns/host_cache.h
diff --git a/net/dns/host_cache.h b/net/dns/host_cache.h
index 752ebeb6c0732f32e6677babe7b517d11ebf0354..e37ce53a013b7892ceef5cf5222b3ab538a38dd0 100644
--- a/net/dns/host_cache.h
+++ b/net/dns/host_cache.h
@@ -31,6 +31,7 @@ class NET_EXPORT HostCache : NON_EXPORTED_BASE(public base::NonThreadSafe) {
     Entry(int error, const AddressList& addrlist, base::TimeDelta ttl);
     // Use when |ttl| is unknown.
     Entry(int error, const AddressList& addrlist);
+

[Randy Smith (Not in Mondays), 2016/04/28 16:24:16]

The comments and code seem crowded together enough in this class that I'm not
sure I see the point of whitespace to separate constructors and destructor.  But
it's aesthetics; up to you.

[Julia Tuttle, 2016/04/29 17:33:52]

Fixed.

     ~Entry();
 
     bool has_ttl() const { return ttl >= base::TimeDelta(); }
@@ -64,17 +65,40 @@ class NET_EXPORT HostCache : NON_EXPORTED_BASE(public base::NonThreadSafe) {
     HostResolverFlags host_resolver_flags;
   };
 
-  struct EvictionHandler {
-    void Handle(const Key& key,
-                const Entry& entry,
-                const base::TimeTicks& expiration,
-                const base::TimeTicks& now,
-                bool onGet) const;
+  struct NET_EXPORT StaleEntryInfo {
+    // Time since the entry's TTL has expired. Negative if not expired.

[Randy Smith (Not in Mondays), 2016/04/28 16:24:16]

If it might not be expired, it's not really StaleEntryInfo. 
EntryExpirationInfo?

[Julia Tuttle, 2016/04/29 17:33:52]

EntryStaleness.

+    base::TimeDelta expired_by;
+    // Number of network changes since this result was cached, or 0u if none.

[Randy Smith (Not in Mondays), 2016/04/28 16:24:16]

nit, suggestion: I find the comma following phrase redundant; what else would 0u
mean and how else would it be expressed?

[Julia Tuttle, 2016/04/29 17:33:52]

Removed.

+    unsigned network_changes;
+    // Number of hits to the cache entry while stale (expired or past-network).
+    unsigned stale_hits;

[Randy Smith (Not in Mondays), 2016/04/28 16:24:16]

nit: The style guide forbids using built-in types other than int.

[Julia Tuttle, 2016/04/29 17:33:52]

Done. (Does it not allow size_t either?)

[Randy Smith (Not in Mondays), 2016/05/04 18:15:25]

size_t isn't a built-in type :-}.  But yes, size_t is ok.  Quoting from the
guide:

"<stdint.h> defines types like int16_t, uint32_t, int64_t, etc. You should
always use those in preference to short, unsigned long long and the like, when
you need a guarantee on the size of an integer. Of the C integer types, only int
should be used. When appropriate, you are welcome to use standard types like
size_t and ptrdiff_t."

+
+    bool is_stale() const {
+      return expired_by >= base::TimeDelta() || network_changes > 0;

[Ryan Sleevi, 2016/04/28 01:44:25]

nit: network_changes > 0 first (to short-circuit the overhead), then the
construction of the object?

[Julia Tuttle, 2016/04/29 17:33:52]

Done.

+    }
+  };
+
+  // TODO: This should be private, but Histogram* functions in the .cc need to
+  // poke at it. At least it's not NET_EXPORTed.

[Ryan Sleevi, 2016/04/28 01:44:25]

Is the TODO meaning that you're planning to address this? Having private
member-function histogram functions leak into this header (e.g. via the
"private:" section) seems way better than spraying this into the header.

[Julia Tuttle, 2016/04/29 17:33:52]

I'm removing the TODO.

I need the struct here because I don't think I Can declare a map with an
incomplete type.

I'd move the histograms here to avoid having to pass in network_changes, but
then I'd need to move the enums here too.

+  struct EntryInternal : public Entry {
+    EntryInternal(const Entry& entry,
+                  base::TimeTicks now,
+                  base::TimeDelta ttl,
+                  unsigned network_changes);
+
+    base::TimeTicks added;

[Randy Smith (Not in Mondays), 2016/04/28 16:24:16]

nit: Document meanings of member variables?   The use of network_changes is
specifically slightly subtle.

[Julia Tuttle, 2016/04/29 17:33:52]

Done; I think the others are self-evident.

+    base::TimeTicks expires;
+    unsigned network_changes;
+    unsigned total_hits;
+    unsigned stale_hits;
+
+    bool CheckStale(base::TimeTicks now,
+                    unsigned network_changes,
+                    StaleEntryInfo* out) const;
   };
 
-  typedef ExpiringCache<Key, Entry, base::TimeTicks,
-                        std::less<base::TimeTicks>,
-                        EvictionHandler> EntryMap;
+  // Ditto.
+  typedef std::map<Key, EntryInternal> EntryMap;
 
   // Constructs a HostCache that stores up to |max_entries|.
   explicit HostCache(size_t max_entries);
@@ -85,6 +109,10 @@ class NET_EXPORT HostCache : NON_EXPORTED_BASE(public base::NonThreadSafe) {
   // |now|. If there is no such entry, returns NULL.
   const Entry* Lookup(const Key& key, base::TimeTicks now);
 
+  const Entry* LookupStale(const Key& key,

[Ryan Sleevi, 2016/04/28 01:44:25]

Document?

[Julia Tuttle, 2016/04/29 17:33:52]

Done.

+                           base::TimeTicks now,
+                           StaleEntryInfo* stale_out);
+
   // Overwrites or creates an entry for |key|.
   // |entry| is the value to set, |now| is the current time
   // |ttl| is the "time to live".
@@ -93,6 +121,10 @@ class NET_EXPORT HostCache : NON_EXPORTED_BASE(public base::NonThreadSafe) {
            base::TimeTicks now,
            base::TimeDelta ttl);
 
+  // Marks all entries as stale on account of a network change.
+  // (Does not actually remove them from memory.)

[Randy Smith (Not in Mondays), 2016/04/28 16:24:16]

nit, suggestion: I think the fact that the interface now exposes getters for
stale entries makes the parenthesized comment redundant.

[Julia Tuttle, 2016/04/29 17:33:52]

Removed.

+  void OnNetworkChange();
+
   // Empties the cache
   void clear();
 
@@ -102,7 +134,7 @@ class NET_EXPORT HostCache : NON_EXPORTED_BASE(public base::NonThreadSafe) {
   // Following are used by net_internals UI.
   size_t max_entries() const;
 
-  const EntryMap& entries() const;
+  std::unique_ptr<base::Value> GetEntriesAsValue() const;

[Ryan Sleevi, 2016/04/28 01:44:24]

Document?

This feels a little weird because it's now exposing something explicitly for
NetLog as an API contract. Lessons have been this can backfire when people
'depend' on it. So documentation here about the "here be dragons" may help

[Randy Smith (Not in Mondays), 2016/04/28 16:24:16]

I agree.  I'm willing to accept this interface, but the old way (in which the
net_log pickling routine used the public API to pull out information and put it
in Value/json form) strikes me as better if there's no major down sides to it.

[Julia Tuttle, 2016/04/29 17:33:52]

Would appending "ForNetLogOnly" do it?

I wanted to avoid having to re-export a bunch of entry metadata. I guess I can
make EntryInternal public (and rename it) and just expose the map itself if you
prefer that.

[Randy Smith (Not in Mondays), 2016/05/04 18:15:25]

So there's a balancing act here.  I'm ok with the interface as given if there's
real value in logging in the netlog stuff that's not available through the
public interface (which I take as being the Entry structure), and I'm happy to
take your word that there's value from a debugging perspective.  Having said
that, looking at what's returned by this function looks like it's all from
Entry--can you clarify what private information you're wanting to share with the
netlog that public consumers can't access through Entry?

(I'll also note that I personally think a friend declaration is another fine way
to solve this problem, since what we're trying to say is that we're giving
privileges to the net log that other consumers don't have.  But I'd run that by
Matt before doing it :-}.)

[Randy Smith (Not in Mondays), 2016/05/05 20:41:51]

Ping?  (I asked what the private information you're trying to share with NetLog
is.)

 
   // Creates a default cache.
   static std::unique_ptr<HostCache> CreateDefaultCache();
@@ -111,13 +143,17 @@ class NET_EXPORT HostCache : NON_EXPORTED_BASE(public base::NonThreadSafe) {
   FRIEND_TEST_ALL_PREFIXES(HostCacheTest, NoCache);
 
   // Returns true if this HostCache can contain no entries.
-  bool caching_is_disabled() const {
-    return entries_.max_entries() == 0;
-  }
+  bool caching_is_disabled() const { return max_entries_ == 0; }
+
+  void Evict(base::TimeTicks now);

[Randy Smith (Not in Mondays), 2016/04/28 16:24:16]

Document?  When I first encountered this, I presumed it was tossing all entries
that were stale, which I didn't like.  A single entry is better, but it should
be documented/named EvictAnEntry or something.

[Julia Tuttle, 2016/04/29 17:33:52]

Renamed to EvictOneEntry.

+
+  EntryMap::iterator SelectEntryToEvict();

[Ryan Sleevi, 2016/04/28 01:44:25]

Document?

[Randy Smith (Not in Mondays), 2016/04/28 16:24:16]

Why does this exist?  It's only called from Evict()?

[Julia Tuttle, 2016/04/29 17:33:52]

I was imagining a more complex algorithm, but there isn't one yet, so I've
merged it back in.

 
   // Map from hostname (presumably in lowercase canonicalized format) to
   // a resolved result entry.
   EntryMap entries_;
+  size_t max_entries_;
+  unsigned network_changes_;
 
   DISALLOW_COPY_AND_ASSIGN(HostCache);
 };