HostResolver: don't interpret NULL callback argument as a request to do synchronous resolution.

HostResolver: don't interpret NULL callback argument as a request to do synchronous resolution. 

BUG=90547, 60149
TEST=net_unittests

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

[agayev, 2011-07-28 23:01:05]

What do you guys think of this API?  I thought I could finish bug 90547 since
the work is almost finished, but decided to take your opinions before moving on
to do more work on it.  Currently the problems with this CL are:

1) Duplicated code between HostResolverImpl::Resolve and
HostResolverImpl::ResolveFromCache, can be easily fixed.

2) It passes all unit tests except
HttpNetworkTransactionTest.UseIPConnectionPooling, which uses
OneTimeCachingHostResolver, which I did not explore but believe can also be
easily fixed.

Let me know what you think.

[eroman, 2011-07-28 23:15:40]

I like this approach of having a separate API.

Two high-level comments before I dive into the details:

(1) The behavior in SPDY when using an IP literal is no longer the same. Please
verify that this is not a bug. Specifically, before spdy would get a resolution
for "192.168.1.1", whereas now the cache lookup is going to fail. I suspect we
want to handle the IP literal case in the "cache" look-up too.

(2) I think the API should distinguish between "failed to find it in the cache"
and "cached entry was for a failure". We don't currently cache failures (I had
enabled this at one point, but it started a shitstorm of problems; even so, I
think we should assume it can happen). As such, I think returning a "bool", and
taking a NetError* (int) should work well.

[agayev, 2011-07-29 15:34:19]

On 2011/07/28 23:15:40, eroman wrote:
> I like this approach of having a separate API.
> 
> Two high-level comments before I dive into the details:
> 
> (1) The behavior in SPDY when using an IP literal is no longer the same.
Please
> verify that this is not a bug. Specifically, before spdy would get a
resolution
> for "192.168.1.1", whereas now the cache lookup is going to fail. I suspect we
> want to handle the IP literal case in the "cache" look-up too.

Okay, I will change it so that for both Resolve() and ResolveFromCache() IP
literal lookups succeed.
> 
> (2) I think the API should distinguish between "failed to find it in the
cache"
> and "cached entry was for a failure". We don't currently cache failures (I had
> enabled this at one point, but it started a shitstorm of problems; even so, I
> think we should assume it can happen). As such, I think returning a "bool",
and
> taking a NetError* (int) should work well.

HostResolverProc used to return ERR_NAME_NOT_RESOLVED if it failed without
calling getaddrinfo.  Now, with the change I've made (moving empty or long
hostname check to Resolve) it always calls getaddrinfo.  As a result, it should
always return either ERR_NAME_RESOLUTION_FAILED or OK.

That being said, we can return ERR_NAME_NOT_RESOLVED for "failed to find it in
the cache" and return whatever is cached for "found in the cache", which would
be OK for success and ERR_NAME_RESOLUTION_FAILED for failure.  What do you
think?  I like it this way, since it makes API consistent (both returning int)
but it's your call, since you'll be the consumer of it for a long time, while
I'll be gone in 2 weeks :-)

(FYI, AsyncHostResolver will be able to cache specific reason for negatives,
like ERR_NX_DOMAIN, ERR_SERVER_REQUIRES_TCP, etc.)

[agayev, 2011-07-29 16:19:38]

Okay, I wrote above without actually reading the code (just by looking at the
comments) but looks like you are returning ERR_NAME_NOT_RESOLVED for the case
when getaddrinfo() "host not found".

I'll go over the code in a more detailed way and see if what I described is
possible.

On 2011/07/29 15:34:19, agayev wrote:
> On 2011/07/28 23:15:40, eroman wrote:
> > I like this approach of having a separate API.
> > 
> > Two high-level comments before I dive into the details:
> > 
> > (1) The behavior in SPDY when using an IP literal is no longer the same.
> Please
> > verify that this is not a bug. Specifically, before spdy would get a
> resolution
> > for "192.168.1.1", whereas now the cache lookup is going to fail. I suspect
we
> > want to handle the IP literal case in the "cache" look-up too.
> 
> Okay, I will change it so that for both Resolve() and ResolveFromCache() IP
> literal lookups succeed.
> > 
> > (2) I think the API should distinguish between "failed to find it in the
> cache"
> > and "cached entry was for a failure". We don't currently cache failures (I
had
> > enabled this at one point, but it started a shitstorm of problems; even so,
I
> > think we should assume it can happen). As such, I think returning a "bool",
> and
> > taking a NetError* (int) should work well.
> 
> HostResolverProc used to return ERR_NAME_NOT_RESOLVED if it failed without
> calling getaddrinfo.  Now, with the change I've made (moving empty or long
> hostname check to Resolve) it always calls getaddrinfo.  As a result, it
should
> always return either ERR_NAME_RESOLUTION_FAILED or OK.
> 
> That being said, we can return ERR_NAME_NOT_RESOLVED for "failed to find it in
> the cache" and return whatever is cached for "found in the cache", which would
> be OK for success and ERR_NAME_RESOLUTION_FAILED for failure.  What do you
> think?  I like it this way, since it makes API consistent (both returning int)
> but it's your call, since you'll be the consumer of it for a long time, while
> I'll be gone in 2 weeks :-)
> 
> (FYI, AsyncHostResolver will be able to cache specific reason for negatives,
> like ERR_NX_DOMAIN, ERR_SERVER_REQUIRES_TCP, etc.)

[agayev, 2011-07-29 16:38:36]

We can return either ERR_CACHE_MISS (which would make sense for a method named
ResolveFromCache) or return bool and pass *NetError.  Let me know which you
like. 

On 2011/07/29 16:19:38, agayev wrote:
> Okay, I wrote above without actually reading the code (just by looking at the
> comments) but looks like you are returning ERR_NAME_NOT_RESOLVED for the case
> when getaddrinfo() "host not found".
> 
> I'll go over the code in a more detailed way and see if what I described is
> possible.
> 
> On 2011/07/29 15:34:19, agayev wrote:
> > On 2011/07/28 23:15:40, eroman wrote:
> > > I like this approach of having a separate API.
> > > 
> > > Two high-level comments before I dive into the details:
> > > 
> > > (1) The behavior in SPDY when using an IP literal is no longer the same.
> > Please
> > > verify that this is not a bug. Specifically, before spdy would get a
> > resolution
> > > for "192.168.1.1", whereas now the cache lookup is going to fail. I
suspect
> we
> > > want to handle the IP literal case in the "cache" look-up too.
> > 
> > Okay, I will change it so that for both Resolve() and ResolveFromCache() IP
> > literal lookups succeed.
> > > 
> > > (2) I think the API should distinguish between "failed to find it in the
> > cache"
> > > and "cached entry was for a failure". We don't currently cache failures (I
> had
> > > enabled this at one point, but it started a shitstorm of problems; even
so,
> I
> > > think we should assume it can happen). As such, I think returning a
"bool",
> > and
> > > taking a NetError* (int) should work well.
> > 
> > HostResolverProc used to return ERR_NAME_NOT_RESOLVED if it failed without
> > calling getaddrinfo.  Now, with the change I've made (moving empty or long
> > hostname check to Resolve) it always calls getaddrinfo.  As a result, it
> should
> > always return either ERR_NAME_RESOLUTION_FAILED or OK.
> > 
> > That being said, we can return ERR_NAME_NOT_RESOLVED for "failed to find it
in
> > the cache" and return whatever is cached for "found in the cache", which
would
> > be OK for success and ERR_NAME_RESOLUTION_FAILED for failure.  What do you
> > think?  I like it this way, since it makes API consistent (both returning
int)
> > but it's your call, since you'll be the consumer of it for a long time,
while
> > I'll be gone in 2 weeks :-)
> > 
> > (FYI, AsyncHostResolver will be able to cache specific reason for negatives,
> > like ERR_NX_DOMAIN, ERR_SERVER_REQUIRES_TCP, etc.)

[cbentzel, 2011-07-29 19:55:32]

I'm wondering [without looking at all] if we could push down the AddressList for
the SPDY case and remove the need for the ResolveFromCache.

I'll spend a few minutes investigating that option.

[cbentzel, 2011-07-29 20:11:39]

+rtenneti, who's been working on the SPDY session IP pooling.

Do you know why the resolution can't go over-the-wire [or be async] for the SPDY
session IP pooling case? Is this a latency issue or due to not wanting the pool
determination to be async?

[rtenneti, 2011-07-30 23:06:32]

spdy_session_pool changes look ok to me. 

LGTM.

[agayev, 2011-08-01 15:12:54]

rtenetti: I think you missed the question cbentzel asked.  Can you please take a
look again?  Thanks.

On 2011/07/30 23:06:32, rtenneti wrote:
> spdy_session_pool changes look ok to me. 
> 
> LGTM.

[ramant (doing other things), 2011-08-01 21:10:32]

>Do you know why the resolution can't go over-the-wire [or be
>async] for the SPDY session IP pooling case? 

Hi Chris,
  Mike Belshe and/or William Chan would know the answer.

>Is this a latency issue or due to not wanting the pool
>determination to be async?

My guess: Some of the existing code relies on pool determination (or getting the
spdy session) being synchronous (I think that could be changed. We need to
change callers of SpdySession Get, GetIfExists and HasSession to get the answer
async). We might have chosen this to be synchronous for speed (or latency
issue). Would like to defer it to Mike and William.

thanks,
raman (with 2 cents)

[agayev, 2011-08-02 14:35:30]

willchan is on vacation, mbelshe is not at google any more.  Followings are the
options, if people could agree on one I could proceed:

a) bool ResolveFromCache(..., int* net_error)
b) int ResolveFromCache() (returns ERR_CACHE_MISS)
c) Add TestCompletionCallback copycat like SyncCompletionHelper and use it in
SpdySessionPool::Lookup
d) let the CL rot

On 2011/08/01 21:10:32, ramant wrote:
> >Do you know why the resolution can't go over-the-wire [or be
> >async] for the SPDY session IP pooling case? 
> 
> Hi Chris,
>   Mike Belshe and/or William Chan would know the answer.
> 
> >Is this a latency issue or due to not wanting the pool
> >determination to be async?
> 
> My guess: Some of the existing code relies on pool determination (or getting
the
> spdy session) being synchronous (I think that could be changed. We need to
> change callers of SpdySession Get, GetIfExists and HasSession to get the
answer
> async). We might have chosen this to be synchronous for speed (or latency
> issue). Would like to defer it to Mike and William.
> 
> thanks,
> raman (with 2 cents)

[cbentzel, 2011-08-02 17:12:25]

I'm OK with b)

On Tue, Aug 2, 2011 at 10:35 AM,  <agayev@chromium.org> wrote:
> willchan is on vacation, mbelshe is not at google any more.  Followings are
> the
> options, if people could agree on one I could proceed:
>
> a) bool ResolveFromCache(..., int* net_error)
> b) int ResolveFromCache() (returns ERR_CACHE_MISS)
> c) Add TestCompletionCallback copycat like SyncCompletionHelper and use it
> in
> SpdySessionPool::Lookup
> d) let the CL rot
>
> On 2011/08/01 21:10:32, ramant wrote:
>>
>> >Do you know why the resolution can't go over-the-wire [or be
>> >async] for the SPDY session IP pooling case?
>
>> Hi Chris,
>>   Mike Belshe and/or William Chan would know the answer.
>
>> >Is this a latency issue or due to not wanting the pool
>> >determination to be async?
>
>> My guess: Some of the existing code relies on pool determination (or
>> getting
>
> the
>>
>> spdy session) being synchronous (I think that could be changed. We need to
>> change callers of SpdySession Get, GetIfExists and HasSession to get the
>
> answer
>>
>> async). We might have chosen this to be synchronous for speed (or latency
>> issue). Would like to defer it to Mike and William.
>
>> thanks,
>> raman (with 2 cents)
>
>
>
> http://codereview.chromium.org/7492059/
>

[agayev, 2011-08-04 16:26:21]

It turned out that I need this to go in before I could implement a fallback
mechanism for the AsyncHostResolver.

This passes all the net_unittests.  I have also added a new netlog source and
event type for cache lookups.

Please review, thanks.

[cbentzel, 2011-08-04 18:55:22]

http://codereview.chromium.org/7492059/diff/20025/net/base/host_resolver.h
File net/base/host_resolver.h (right):

http://codereview.chromium.org/7492059/diff/20025/net/base/host_resolver.h#ne...
net/base/host_resolver.h:161: // Resolves the given hostname (or IP address
literal), from cache only,
Please clean up this comment a little bit.

Suggestion:

// Resolves the given hostname (or IP address literal) out of cache only. 
// This is guaranteed to complete synchronously.
// This acts like |Resolve()| if the hostname is a literal or a cached value
exists.
// Otherwise, ERR_DNS_CACHE_MISS is returned.

http://codereview.chromium.org/7492059/diff/20025/net/base/host_resolver.h#ne...
net/base/host_resolver.h:168: // NOTE: both of the above methods return
ERR_NAME_NOT_RESOLVED when the
I'd add this to the Resolve documentation rather than here.

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

http://codereview.chromium.org/7492059/diff/20025/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1135: const BoundNetLog& source_net_log) {
You should add a helper function that both Resolve and ResolveFromCache call to
reduce duplicate code. Something like

int request_id = next_request_id_++;
BoundNetLog request_net_log = ...;
int rv = ResolveHelper(info, addresses, ..., request_id, request_net_log);
if (rv != ERR_DNS_CACHE_MISS)
  return rv;
// Handle the enqueueing here.

http://codereview.chromium.org/7492059/diff/20025/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1469: NetLog::TYPE_HOST_RESOLVER_IMPL_REQUEST;
I'm not sure if there's much benefit differentiating CACHE_LOOKUP versus REQUEST
here. 

If there is, I'd pass the event type in rather than a boolean to make it clearer
at the callsites.

http://codereview.chromium.org/7492059/diff/20025/net/base/net_log_event_type...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/7492059/diff/20025/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:50: // The start/end of a DNS cache lookup
request.
I don't know if it makes sense to differentiate between an IMPL_REQUEST and a
CACHE_LOOKUP. I guess it would make some sense if a person is confused why there
wasn't an over-the-wire request, but they should only be seeing a DNS_CACHE_MISS
return code in the ResolveFromCache case.

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

http://codereview.chromium.org/7492059/diff/20025/net/dns/async_host_resolver...
net/dns/async_host_resolver.cc:126: bool ResolveAsIp() {
Should this just be a helper function which is shared between host_resolver_impl
and async_host_resolver?

http://codereview.chromium.org/7492059/diff/20025/net/dns/async_host_resolver...
net/dns/async_host_resolver.cc:143: bool ServeFromCache() {
Ditto for this.

[agayev, 2011-08-04 20:18:00]

http://codereview.chromium.org/7492059/diff/20025/net/base/host_resolver.h
File net/base/host_resolver.h (right):

http://codereview.chromium.org/7492059/diff/20025/net/base/host_resolver.h#ne...
net/base/host_resolver.h:161: // Resolves the given hostname (or IP address
literal), from cache only,
On 2011/08/04 18:55:22, cbentzel wrote:
> Please clean up this comment a little bit.
> 
> Suggestion:
> 
> // Resolves the given hostname (or IP address literal) out of cache only. 
> // This is guaranteed to complete synchronously.
> // This acts like |Resolve()| if the hostname is a literal or a cached value
> exists.
> // Otherwise, ERR_DNS_CACHE_MISS is returned.

Done.

http://codereview.chromium.org/7492059/diff/20025/net/base/host_resolver.h#ne...
net/base/host_resolver.h:168: // NOTE: both of the above methods return
ERR_NAME_NOT_RESOLVED when the
On 2011/08/04 18:55:22, cbentzel wrote:
> I'd add this to the Resolve documentation rather than here.

Done.

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

http://codereview.chromium.org/7492059/diff/20025/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1135: const BoundNetLog& source_net_log) {
On 2011/08/04 18:55:22, cbentzel wrote:
> You should add a helper function that both Resolve and ResolveFromCache call
to
> reduce duplicate code. Something like
> 
> int request_id = next_request_id_++;
> BoundNetLog request_net_log = ...;
> int rv = ResolveHelper(info, addresses, ..., request_id, request_net_log);
> if (rv != ERR_DNS_CACHE_MISS)
>   return rv;
> // Handle the enqueueing here.

Done.

http://codereview.chromium.org/7492059/diff/20025/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1469: NetLog::TYPE_HOST_RESOLVER_IMPL_REQUEST;
On 2011/08/04 18:55:22, cbentzel wrote:
> I'm not sure if there's much benefit differentiating CACHE_LOOKUP versus
REQUEST
> here. 
> 
> If there is, I'd pass the event type in rather than a boolean to make it
clearer
> at the callsites. 

Removed it.

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

http://codereview.chromium.org/7492059/diff/20025/net/dns/async_host_resolver...
net/dns/async_host_resolver.cc:126: bool ResolveAsIp() {
On 2011/08/04 18:55:22, cbentzel wrote:
> Should this just be a helper function which is shared between
host_resolver_impl
> and async_host_resolver?

Not really.  They have different logic, maybe this can be merged once IPv6 is
fully implemented for AsyncHostResolver.

http://codereview.chromium.org/7492059/diff/20025/net/dns/async_host_resolver...
net/dns/async_host_resolver.cc:143: bool ServeFromCache() {
On 2011/08/04 18:55:22, cbentzel wrote:
> Ditto for this. 

Not for sure.  ServeFromCache will be very different for AsyncHostResolver case
when RR cache is added.  It already is somewhat different.

[cbentzel, 2011-08-04 21:18:36]

LGTM

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

http://codereview.chromium.org/7492059/diff/21034/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1193: const Key& key,
Ah, I guess the OnStartRequest/OnFinishRequest can't go in here. Not quite as
useful as I hoped but thanks for doing it.

[wtc, 2011-10-11 02:42:53]

agayev, cbentzel: I found two problems with the two NetLog
parameters of HostResolverImpl::ResolveHelper.

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

http://codereview.chromium.org/7492059/diff/21034/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1155: source_net_log, request_net_log);

This ResolveHelper call passes source_net_log, request_net_log
in the opposite order of the function's prototype.

I wrote a CL http://codereview.chromium.org/8220028 to
fix this, assuming the order in the function's prototype
is right.  But I suspect the function prototype may have
the wrong order, because the source_net_log, request_net_log
order is prevalent in this file.

http://codereview.chromium.org/7492059/diff/21034/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:1197: const BoundNetLog& source_net_log) {

ResolveHelper does not use its source_net_log argument.