AsyncHostResolver: integrated HostCache, temporarily, until we have RR cache.

AsyncHostResolver: integrated HostCache, temporarily, until we have RR cache.

BUG=60149
TEST=net_unittest --gtest_filter="AsyncHostResolverTest*"

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

[cbentzel, 2011-07-21 15:31:10]

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver.cc
File net/base/async_host_resolver.cc (right):

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver...
net/base/async_host_resolver.cc:69: HostCache* CreateDefaultCache() {
This should be shared with the one in host_resolver_impl

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver...
net/base/async_host_resolver.cc:327: // recreating info from DnsTransaction::Key
adds a lot of temporary
It seems like it should still be possible to cache when requests is empty -
doesnt transaction->key() have enough information?

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver...
net/base/async_host_resolver.cc:368: DCHECK_EQ(OK, cache_entry->error);
Ah - I was going to ask you to handle non-OK error codes, but it looks like you
don't need to due to OnTransactionComplete logic.

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver...
File net/base/async_host_resolver_unittest.cc (right):

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver...
net/base/async_host_resolver_unittest.cc:201: // Now lookup |info0_| from cache
only, store results in |addrlist1_|,
Could you add tests for cache expiration as well?

[agayev, 2011-07-21 17:48:03]

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver.cc
File net/base/async_host_resolver.cc (right):

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver...
net/base/async_host_resolver.cc:69: HostCache* CreateDefaultCache() {
On 2011/07/21 15:31:10, cbentzel wrote:
> This should be shared with the one in host_resolver_impl

Done.

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver...
net/base/async_host_resolver.cc:327: // recreating info from DnsTransaction::Key
adds a lot of temporary
On 2011/07/21 15:31:10, cbentzel wrote:
> It seems like it should still be possible to cache when requests is empty -
> doesnt transaction->key() have enough information?

Well, it doesn't have host_resolver_flags, whose usefulness we need to rethink. 
This flag affects how the lookup is made, thus it affects how the AddressList is
formed.  In RR cache we would not store AddressList in cache, and
AsyncHostResolver would look at flags and use data in RR cache to form the
AddressList and pass it back.  E.g. if HOST_RESOLVER_CANONNAME is set, it would
set ai_canonname field of the first element of addrinfo chain to canonical name
of the host.

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver...
File net/base/async_host_resolver_unittest.cc (right):

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver...
net/base/async_host_resolver_unittest.cc:201: // Now lookup |info0_| from cache
only, store results in |addrlist1_|,
On 2011/07/21 15:31:10, cbentzel wrote:
> Could you add tests for cache expiration as well? 

Like creating a cache with 10ms TTL and then sleep and then make sure it is
expired?  Doesn't that belong to HostCache unit test?

[cbentzel, 2011-07-21 18:22:15]

LGTM

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver.cc
File net/base/async_host_resolver.cc (right):

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver...
net/base/async_host_resolver.cc:327: // recreating info from DnsTransaction::Key
adds a lot of temporary
On 2011/07/21 17:48:03, agayev wrote:
> On 2011/07/21 15:31:10, cbentzel wrote:
> > It seems like it should still be possible to cache when requests is empty -
> > doesnt transaction->key() have enough information?
> 
> Well, it doesn't have host_resolver_flags, whose usefulness we need to
rethink. 
> This flag affects how the lookup is made, thus it affects how the AddressList
is
> formed.  In RR cache we would not store AddressList in cache, and
> AsyncHostResolver would look at flags and use data in RR cache to form the
> AddressList and pass it back.  E.g. if HOST_RESOLVER_CANONNAME is set, it
would
> set ai_canonname field of the first element of addrinfo chain to canonical
name
> of the host.

Yes, I agree that's how it would be done. I think holding off until a real RR
cache is in place is good.

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver...
File net/base/async_host_resolver_unittest.cc (right):

http://codereview.chromium.org/7466031/diff/1017/net/base/async_host_resolver...
net/base/async_host_resolver_unittest.cc:201: // Now lookup |info0_| from cache
only, store results in |addrlist1_|,
On 2011/07/21 17:48:03, agayev wrote:
> On 2011/07/21 15:31:10, cbentzel wrote:
> > Could you add tests for cache expiration as well? 
> 
> Like creating a cache with 10ms TTL and then sleep and then make sure it is
> expired?  Doesn't that belong to HostCache unit test?

yeah, you're right.