Adds custom ttl argument to HostCache::Set.

Adds custom ttl argument to HostCache::Set.

BUG=25472, 107880
TEST=net_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=118489

[szym, 2012-01-12 20:56:49]

Let me know if you feel it needs a test.

[cbentzel, 2012-01-13 01:06:13]

I think augmenting the Compact unit test for HostCache would be good here -
don't know if the new behavior requires standalones.

Should you DCHECK that the ttl >= 0?

http://codereview.chromium.org/9197009/diff/2001/net/base/host_cache.h
File net/base/host_cache.h (right):

http://codereview.chromium.org/9197009/diff/2001/net/base/host_cache.h#newcode91
net/base/host_cache.h:91: // (|error|, |addrlist|) is the value to set, |ttl| is
the "time to live",
How much work would it be to only allow this version of Set and not have the ttl
== 0 be equivalent to "use the default"?

I wonder because DNS TTLs of 0 are potentially allowed. It would also make the
API easier - could remove the defaults in the constructor, and the description
of what |ttl| represents becomes clearer.

[szym, 2012-01-14 16:30:54]

The effort to move default TTLs out of HostCache is small (see below), so I'll
just do that.

http://codereview.chromium.org/9197009/diff/2001/net/base/host_cache.h
File net/base/host_cache.h (right):

http://codereview.chromium.org/9197009/diff/2001/net/base/host_cache.h#newcode91
net/base/host_cache.h:91: // (|error|, |addrlist|) is the value to set, |ttl| is
the "time to live",
On 2012/01/13 01:06:14, cbentzel wrote:
> How much work would it be to only allow this version of Set and not have the
ttl
> == 0 be equivalent to "use the default"?
> 
> I wonder because DNS TTLs of 0 are potentially allowed. It would also make the
> API easier - could remove the defaults in the constructor, and the description
> of what |ttl| represents becomes clearer.

(Excluding unittests) There are three calls to HostCache::Set:
1) HostResolverImpl::OnJobComplete
2) AsyncHostResolver::OnDnsRequestComplete
3) (ProxyResolverJSBindings) DefaultJSBindings::DnsResolveHelper

There are two calls to HostCache::HostCache where the default TTL values are
set:
1) HostCache::CreateDefaultCache // success: 1 min, failure: 0
2) ProxyResolverV8::GetProxyURL  // success: 5 min, failure: 5 min

So we could easily move the default TTL values from HostCache to the sites
calling HostCache::Set. 

Regarding the other issue, entries with TTLs of 0 will never be returned by
HostCache::Lookup, so the only benefit of storing such entries afaik is
net-internals, even though they won't survive for long if the cache is at
capacity.

[szym, 2012-01-18 21:57:17]

Updated CL to remove default TTL values from HostCache. In the process I found
that the net_internals_ui_browsertest was misusing HostCache::Set (passing
expiration date as "now").

[mmenke, 2012-01-18 22:06:02]

On 2012/01/18 21:57:17, szym wrote:
> Updated CL to remove default TTL values from HostCache. In the process I found
> that the net_internals_ui_browsertest was misusing HostCache::Set (passing
> expiration date as "now").

Actually, I think it passes in an expiration date of 2 days ago.  Wanted to test
seeing expired entries, and avoid having to handle the case where they aren't
expired the first time or two we see them.

You're also going to need to update
src/chrome/test/data/webui/net_internals/dns_view.js and
src/chrome/browser/resources/net_internals/dns_view.js and
src/chrome/browser/resources/net_internals/dns_view.html to remove the fields
you removed in net_internals_ui.cc.

[szym, 2012-01-18 22:10:43]

I might have misunderstood the code. I read "expires" as the
expiration date for the newly added entry. It was passed as the "now"
argument to HostCache::Set which is then used to in HostCache::Compact
to determine if an entry is expired.

Regarding the fields that now will show up as "undefined",
HostResolverImpl will still need some defaults for addresses obtained
via ProcTask. So HostResolverImpl could provide accessors to those
defaults. If that does not seem necessary, I'll clean up and remove
them from view.

On Wed, Jan 18, 2012 at 5:06 PM,  <mmenke@chromium.org> wrote:
> On 2012/01/18 21:57:17, szym wrote:
>>
>> Updated CL to remove default TTL values from HostCache. In the process I
>> found
>> that the net_internals_ui_browsertest was misusing HostCache::Set (passing
>> expiration date as "now").
>
>
> Actually, I think it passes in an expiration date of 2 days ago.  Wanted to
> test
> seeing expired entries, and avoid having to handle the case where they
> aren't
> expired the first time or two we see them.
>
> You're also going to need to update
> src/chrome/test/data/webui/net_internals/dns_view.js and
> src/chrome/browser/resources/net_internals/dns_view.js and
> src/chrome/browser/resources/net_internals/dns_view.html to remove the
> fields
> you removed in net_internals_ui.cc.
>
> http://codereview.chromium.org/9197009/

[mmenke, 2012-01-18 22:21:34]

On 2012/01/18 22:10:43, szym wrote:
> I might have misunderstood the code. I read "expires" as the
> expiration date for the newly added entry. It was passed as the "now"
> argument to HostCache::Set which is then used to in HostCache::Compact
> to determine if an entry is expired.

You're right about me misusing the interface.  I misread what you said (I
thought you said "passing now as the expiration", rather than the other way
around).

> Regarding the fields that now will show up as "undefined",
> HostResolverImpl will still need some defaults for addresses obtained
> via ProcTask. So HostResolverImpl could provide accessors to those
> defaults. If that does not seem necessary, I'll clean up and remove
> them from view.

The TTLs are just displayed for informational value, they aren't used for
anything.  Think we may as well keep them for the moment, though, since it's
pretty low cost, unless anyone else has strong feelings on the matter (I don't).

[cbentzel, 2012-01-19 14:57:06]

Looks like some of the NetInternals DNS browser tests are failing - I'll hold
off reviewing until those are fixed.

[cbentzel, 2012-01-19 14:57:07]

Looks like some of the NetInternals DNS browser tests are failing - I'll hold
off reviewing until those are fixed.

[szym, 2012-01-19 21:49:54]

On 2012/01/19 14:57:07, cbentzel wrote:
> Looks like some of the NetInternals DNS browser tests are failing - I'll hold
> off reviewing until those are fixed.

Ready for review.

[mmenke, 2012-01-19 22:12:43]

LGTM

http://codereview.chromium.org/9197009/diff/18001/net/base/host_cache.h
File net/base/host_cache.h (right):

http://codereview.chromium.org/9197009/diff/18001/net/base/host_cache.h#newco...
net/base/host_cache.h:93: base::TimeTicks now);
nit:  Cognitively, I think putting |now| before |ttl| is a little more
intuitive.  Base value before delta.

http://codereview.chromium.org/9197009/diff/18001/net/base/host_cache_unittes...
File net/base/host_cache_unittest.cc (right):

http://codereview.chromium.org/9197009/diff/18001/net/base/host_cache_unittes...
net/base/host_cache_unittest.cc:16: namespace {
nit:  While you're here, could you add a line break here?

http://codereview.chromium.org/9197009/diff/18001/net/base/mock_host_resolver.cc
File net/base/mock_host_resolver.cc (right):

http://codereview.chromium.org/9197009/diff/18001/net/base/mock_host_resolver...
net/base/mock_host_resolver.cc:133: cache_.reset(new HostCache(100 /* max
entries */));
nit:  Suggest you put both constants (this and TTLs) at top of file, in
anonymous namespace, as is done in most other files.  Seems a good idea to do it
where they appear in the other two files, too, while you're are it.

[commit-bot: I haz the power, 2012-01-20 01:03:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/9197009/18004

[commit-bot: I haz the power, 2012-01-20 04:46:08]

Try job failure for 9197009-18004 (retry) (retry) (retry) on mac_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-01-20 06:04:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/9197009/18004

[commit-bot: I haz the power, 2012-01-20 08:33:16]

Try job failure for 9197009-18004 (retry) on mac_rel for steps "browser_tests,
ui_tests".
It's a second try, previously, steps "browser_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[cbentzel, 2012-01-20 11:04:53]

http://codereview.chromium.org/9197009/diff/18004/net/base/host_cache_unittes...
File net/base/host_cache_unittest.cc (right):

http://codereview.chromium.org/9197009/diff/18004/net/base/host_cache_unittes...
net/base/host_cache_unittest.cc:103: // We disallow use of negative entries.
This test isn't so much what happens with negative entries any more, but what
happens with 0 TTL entries. 0 TTL entries in the cache are not used, matching
RFC1035: "Zero values are interpreted to mean that the RR can only be used for
the transaction in progress, and should not be cached."

http://codereview.chromium.org/9197009/diff/18004/net/base/mock_host_resolver.cc
File net/base/mock_host_resolver.cc (right):

http://codereview.chromium.org/9197009/diff/18004/net/base/mock_host_resolver...
net/base/mock_host_resolver.cc:175: return rv;
Not sure you want to early exit here - doesn't match the default behavior, and a
negative entry should replace existing entries in the cache.

http://codereview.chromium.org/9197009/diff/18004/net/dns/async_host_resolver.cc
File net/dns/async_host_resolver.cc (right):

http://codereview.chromium.org/9197009/diff/18004/net/dns/async_host_resolver...
net/dns/async_host_resolver.cc:366: ttl = std::min(ttl, record.ttl);
I thought all of the RR's in the RRSet need to have identical TTL's. Did you
decide to be permissive here? Is this consistent with other resolvers?

http://codereview.chromium.org/9197009/diff/18004/net/dns/async_host_resolver...
net/dns/async_host_resolver.cc:386: // sense, since HostCache will be replaced
with RR cache soon.
This comment about replacing HostCache with RR cache can probably go away.

http://codereview.chromium.org/9197009/diff/18004/net/dns/async_host_resolver...
net/dns/async_host_resolver.cc:389: if (result == OK && cache_.get() &&
!requests.empty()) {
Should we insert into the cache even if the result is non-OK? This would be more
consistent with the non-async case.

[szym, 2012-01-20 16:31:45]

http://codereview.chromium.org/9197009/diff/18004/net/base/host_cache_unittes...
File net/base/host_cache_unittest.cc (right):

http://codereview.chromium.org/9197009/diff/18004/net/base/host_cache_unittes...
net/base/host_cache_unittest.cc:103: // We disallow use of negative entries.
On 2012/01/20 11:04:53, cbentzel wrote:
> This test isn't so much what happens with negative entries any more, but what
> happens with 0 TTL entries. 0 TTL entries in the cache are not used, matching
> RFC1035: "Zero values are interpreted to mean that the RR can only be used for
> the transaction in progress, and should not be cached."

Not really. Zero-TTL entries can never be served from cache (Lookup will not
return entries with TTL 0). Also, HostResolverImpl does not call
HostCache::Lookup when it just got the AddressList from getadddrinfo.

AFAIK (from eroman), the only reason for storing 0-TTL entries is for debugging
with net-internals.

Agreed that this test does not make sense any more. I haven't really filtered
out the spurious tests yet.

http://codereview.chromium.org/9197009/diff/18004/net/base/mock_host_resolver.cc
File net/base/mock_host_resolver.cc (right):

http://codereview.chromium.org/9197009/diff/18004/net/base/mock_host_resolver...
net/base/mock_host_resolver.cc:175: return rv;
On 2012/01/20 11:04:53, cbentzel wrote:
> Not sure you want to early exit here - doesn't match the default behavior, and
a
> negative entry should replace existing entries in the cache. 

Right, I didn't catch that Set(..., TTL = 0) is not the same as no call to
Set(...) at all.

http://codereview.chromium.org/9197009/diff/18004/net/dns/async_host_resolver.cc
File net/dns/async_host_resolver.cc (right):

http://codereview.chromium.org/9197009/diff/18004/net/dns/async_host_resolver...
net/dns/async_host_resolver.cc:366: ttl = std::min(ttl, record.ttl);
On 2012/01/20 11:04:53, cbentzel wrote:
> I thought all of the RR's in the RRSet need to have identical TTL's. Did you
> decide to be permissive here? Is this consistent with other resolvers?

This policy is just a stub. We really should check that the A/AAAA-RRs that we
are collecting match the QNAME or a CNAME alias. The TTL should then be the
min(CNAME.TTL, A.TTL).

I'm planning to write this policy and tests for it in HostResolverImpl.

http://codereview.chromium.org/9197009/diff/18004/net/dns/async_host_resolver...
net/dns/async_host_resolver.cc:389: if (result == OK && cache_.get() &&
!requests.empty()) {
On 2012/01/20 11:04:53, cbentzel wrote:
> Should we insert into the cache even if the result is non-OK? This would be
more
> consistent with the non-async case.

Probably, but rather than updating AsyncHostResolver, I'd wait until we can put
DnsConfigService + DnsTransactionFactory into HostResolverImpl, as to avoid
duplicated maintenance effort.

[cbentzel, 2012-01-20 16:33:39]

LGTM, assuming you make the change to MockHostResolver.

On Fri, Jan 20, 2012 at 11:31 AM, <szym@chromium.org> wrote:

>
> http://codereview.chromium.**org/9197009/diff/18004/net/**
>
base/host_cache_unittest.cc<http://codereview.chromium.org/9197009/diff/18004/net/base/host_cache_unittest.cc>
> File net/base/host_cache_unittest.**cc (right):
>
> http://codereview.chromium.**org/9197009/diff/18004/net/**
>
base/host_cache_unittest.cc#**newcode103<http://codereview.chromium.org/9197009/diff/18004/net/base/host_cache_unittest.cc#newcode103>
> net/base/host_cache_unittest.**cc:103: // We disallow use of negative
> entries.
> On 2012/01/20 11:04:53, cbentzel wrote:
>
>> This test isn't so much what happens with negative entries any more,
>>
> but what
>
>> happens with 0 TTL entries. 0 TTL entries in the cache are not used,
>>
> matching
>
>> RFC1035: "Zero values are interpreted to mean that the RR can only be
>>
> used for
>
>> the transaction in progress, and should not be cached."
>>
>
> Not really. Zero-TTL entries can never be served from cache (Lookup will
> not return entries with TTL 0). Also, HostResolverImpl does not call
> HostCache::Lookup when it just got the AddressList from getadddrinfo.
>
> AFAIK (from eroman), the only reason for storing 0-TTL entries is for
> debugging with net-internals.
>
> Agreed that this test does not make sense any more. I haven't really
> filtered out the spurious tests yet.
>
>
> http://codereview.chromium.**org/9197009/diff/18004/net/**
>
base/mock_host_resolver.cc<http://codereview.chromium.org/9197009/diff/18004/net/base/mock_host_resolver.cc>
> File net/base/mock_host_resolver.cc (right):
>
> http://codereview.chromium.**org/9197009/diff/18004/net/**
>
base/mock_host_resolver.cc#**newcode175<http://codereview.chromium.org/9197009/diff/18004/net/base/mock_host_resolver.cc#newcode175>
> net/base/mock_host_resolver.**cc:175: return rv;
> On 2012/01/20 11:04:53, cbentzel wrote:
>
>> Not sure you want to early exit here - doesn't match the default
>>
> behavior, and a
>
>> negative entry should replace existing entries in the cache.
>>
>
> Right, I didn't catch that Set(..., TTL = 0) is not the same as no call
> to Set(...) at all.
>
>
> http://codereview.chromium.**org/9197009/diff/18004/net/**
>
dns/async_host_resolver.cc<http://codereview.chromium.org/9197009/diff/18004/net/dns/async_host_resolver.cc>
> File net/dns/async_host_resolver.cc (right):
>
> http://codereview.chromium.**org/9197009/diff/18004/net/**
>
dns/async_host_resolver.cc#**newcode366<http://codereview.chromium.org/9197009/diff/18004/net/dns/async_host_resolver.cc#newcode366>
> net/dns/async_host_resolver.**cc:366: ttl = std::min(ttl, record.ttl);
> On 2012/01/20 11:04:53, cbentzel wrote:
>
>> I thought all of the RR's in the RRSet need to have identical TTL's.
>>
> Did you
>
>> decide to be permissive here? Is this consistent with other resolvers?
>>
>
> This policy is just a stub. We really should check that the A/AAAA-RRs
> that we are collecting match the QNAME or a CNAME alias. The TTL should
> then be the min(CNAME.TTL, A.TTL).
>
> I'm planning to write this policy and tests for it in HostResolverImpl.
>
>
> http://codereview.chromium.**org/9197009/diff/18004/net/**
>
dns/async_host_resolver.cc#**newcode389<http://codereview.chromium.org/9197009/diff/18004/net/dns/async_host_resolver.cc#newcode389>
> net/dns/async_host_resolver.**cc:389: if (result == OK && cache_.get() &&
> !requests.empty()) {
> On 2012/01/20 11:04:53, cbentzel wrote:
>
>> Should we insert into the cache even if the result is non-OK? This
>>
> would be more
>
>> consistent with the non-async case.
>>
>
> Probably, but rather than updating AsyncHostResolver, I'd wait until we
> can put DnsConfigService + DnsTransactionFactory into HostResolverImpl,
> as to avoid duplicated maintenance effort.
>
>
http://codereview.chromium.**org/9197009/<http://codereview.chromium.org/9197...
>

[commit-bot: I haz the power, 2012-01-20 18:12:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/9197009/29002

[commit-bot: I haz the power, 2012-01-20 20:40:23]

Change committed as 118489

[mmenke, 2012-01-20 22:02:16]

http://codereview.chromium.org/9197009/diff/29002/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9197009/diff/29002/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:54: const base::TimeDelta kCacheEntryTTL =
base::TimeDelta::FromMinutes(1);
The constants up here should just be the times as integers.

[szym, 2012-01-20 22:05:27]

On 2012/01/20 22:02:16, Matt Menke wrote:
>
http://codereview.chromium.org/9197009/diff/29002/net/base/host_resolver_impl.cc
> File net/base/host_resolver_impl.cc (right):
> 
>
http://codereview.chromium.org/9197009/diff/29002/net/base/host_resolver_impl...
> net/base/host_resolver_impl.cc:54: const base::TimeDelta kCacheEntryTTL =
> base::TimeDelta::FromMinutes(1);
> The constants up here should just be the times as integers.

Should it be 
kCacheEntryTTL_min = 1
kCacheEntryTTL_sec = 60
kCacheEntryTTL_ms = 60000