Cronet: Use stale DNS cache entries experimentally.

Cronet: Use stale DNS cache entries experimentally.

Create a Cronet-specific HostResolver that uses stale DNS cache entries
when available after a delay, to reduce DNS latency at the expense of
accuracy.

Configure it in place of (actually wrapped around) the default resolver
when configured by Cronet experimental options.

Design doc:
https://docs.google.com/document/d/1qmxDNfNUIK2cQQi9u_-Qv3PKxEbEtBKhEvIOza2s7wU/view

BUG=605138
Committed: https://crrev.com/50d9c4b48edf4cb160b426fea7475e689cd1816b
Cr-Commit-Position: refs/heads/master@{#413875}

[Julia Tuttle, 2016-04-26 02:12:09]

Description was changed from

==========
WIP: DNS: Create StaleHostResolver

With ResolveStale{,FromCache} exposed in HostResolver, create a
StaleHostResolver that wraps an existing HostResolver and uses those
methods to return stale results as if they were fresh if a certain
delay has elapsed.

BUG=605138
==========

to

==========
DNS: Create StaleHostResolver

With ResolveStale{,FromCache} exposed in HostResolver, create a
StaleHostResolver that wraps an existing HostResolver and uses those
methods to return stale results as if they were fresh if a certain
delay has elapsed.

BUG=605138
==========

[Julia Tuttle, 2016-04-26 02:12:23]

juliatuttle@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Julia Tuttle, 2016-04-26 02:12:24]

asvitkine, PTAL at histograms.

[Julia Tuttle, 2016-04-26 02:14:07]

Description was changed from

==========
DNS: Create StaleHostResolver

With ResolveStale{,FromCache} exposed in HostResolver, create a
StaleHostResolver that wraps an existing HostResolver and uses those
methods to return stale results as if they were fresh if a certain
delay has elapsed.

BUG=605138
==========

to

==========
DNS: Create StaleHostResolver.

With ResolveStale{,FromCache} exposed in HostResolver, create a
StaleHostResolver that wraps an existing HostResolver and uses those
methods to return stale results as if they were fresh if a certain
delay has elapsed.

BUG=605138
==========

[Julia Tuttle, 2016-04-26 02:14:15]

juliatuttle@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Julia Tuttle, 2016-04-26 02:14:16]

PTAL, rdsmith.

[Ryan Sleevi, 2016-04-26 02:25:35]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2016-04-26 02:25:36]

Drive by, since you solicited feedback on net-dev ;)

https://codereview.chromium.org/1898873006/diff/20001/net/dns/stale_host_reso...
File net/dns/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/20001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:20: if (entry.expired_by >
options.max_expired_time)
Can't you condense these conditionals?

https://codereview.chromium.org/1898873006/diff/20001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:63: class StaleHostResolver::Request {
Document (throughout)?

I'd like to suggest it may make it more readable to separate the definition and
declaration here, so you can one-pass read what the interface is for
StaleHostResolver::Request, and then see the actual implementation.

https://codereview.chromium.org/1898873006/diff/20001/net/dns/stale_host_reso...
File net/dns/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/20001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:12: class NET_EXPORT StaleHostResolver : public
HostResolver {
Document (throughout this file; methods & class)

[Julia Tuttle, 2016-04-27 14:50:26]

https://codereview.chromium.org/1898873006/diff/20001/net/dns/stale_host_reso...
File net/dns/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/20001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:20: if (entry.expired_by >
options.max_expired_time)
On 2016/04/26 at 02:25:36, Ryan Sleevi wrote:
> Can't you condense these conditionals?

Done.

https://codereview.chromium.org/1898873006/diff/20001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:63: class StaleHostResolver::Request {
On 2016/04/26 at 02:25:36, Ryan Sleevi wrote:
> Document (throughout)?
> 
> I'd like to suggest it may make it more readable to separate the definition
and declaration here, so you can one-pass read what the interface is for
StaleHostResolver::Request, and then see the actual implementation.

Done.

https://codereview.chromium.org/1898873006/diff/20001/net/dns/stale_host_reso...
File net/dns/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/20001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:12: class NET_EXPORT StaleHostResolver : public
HostResolver {
On 2016/04/26 at 02:25:36, Ryan Sleevi wrote:
> Document (throughout this file; methods & class)

Done.

[Randy Smith (Not in Mondays), 2016-04-28 17:48:26]

Initial comments.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
File net/dns/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:24: enum RequestOutcome {
nit, suggestion (I'll stop making these once you argue with me once :-}): I find
it hard to parse out what comments belong with what enums in this type of
commenting style and would prefer a space between an enum and the next enums
comment.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:44: void
HistogramAddressListDelta(AddressListDeltaType delta) {
Ah.  Feel free to ignore my comment on the other CL :-}.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:68: // Starts the request. May call
|usable_callback| inline if |resolver| returns
I'm scared about interfaces that are variations (especially more complex
variations) on the usual netstack "return value in synchronous return, call
callback if async".  There's potential complex re-entry problems with calling
callbacks synchronously (Elly tried for filters, Matt shot her down, I don't
remember the issues) and there's potentially complex race issues if you've got
several callbacks running around.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:69: // stale data. May call the callback passed
into the constructor inline if
What callback passed into the constructor?

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:77: const StaleEntryUsableCallback&
usable_callback,
I'm having a hard time mapping the implementation to the above documentation
around this callback.  It looks like it's used for information, i.e. to return a
bool to determine if a stale entry should be returned.  If that's the case,
could you update the doc above to make it clearer?  (I don't have any objections
to using the callback that way--I thought from the above description that it was
being used to pass results back to the caller, which I have more problems with.)

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
File net/dns/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:34: // request potentially stale data.)
nit: How hard would it be to change this?  I suspect your current use cases
don't require it, but to my mind it's a layering violation, so I'd like to at
least explore doing it right.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:47: // after the specified delay instead.
I presume if fresh data is available in that time period, it's returned
preferentially?  The doc doesn't make that clear.

[Ryan Sleevi, 2016-04-29 01:04:09]

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
File net/dns/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:85: void OnStaleDelayElapsed();
Pedantry: Seems like it could be private?

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:88: void OnNetworkRequestComplete(int error);
Pedantry: Seems like it could be private?

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:180: callback_.Reset();
Maybe add a note as to why you don't delete this

I was trying to figure it out, but my guess is that by not deleting this, not
deleting this is what we use to continue the network request, with the
expectation that
1) No one will ever call Cancel (since we called HandleResult)
2) Eventually the request will complete

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:210: if (stale_timer_.IsRunning() ||
returned_stale_result()) {
nit: no braces for consistency with the rest of the file

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:259: for (auto request : pending_requests_)
Shouldn't this be "auto&" or "auto*" (based on https://chromium-cpp.appspot.com/
- I believe auto*)

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
File net/dns/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:28: // If set, stale data from previous networks
is usable; if clear, it's not.
Suggestion: Explain a little more about the trade-offs here - what the risks are
about using stale data from other networks. It's not clear from here (but
perhaps I missed it in another CL or documentation)?

Maybe a scenario where it can improve good results, and a scenario where it can
cause bad results, so that callers have some conceptual overtones about the
trade-offs involved.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:34: // request potentially stale data.)
What HostCache? Who owns it? Is it a global?

(My guess, from this header, is that it's stored on inner_resolver's HostCache -
so the count is only unique for objects sharing the same inner_resolver /
sharing the same HostCache as the inner_resolver. Is that correct?)

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:39: // resolution but potentially returns stale
data according to |stale_options|.
s/resolution but/resolution, but/

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:66: 
delete newline?

[Julia Tuttle, 2016-04-29 18:04:30]

PTAL.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
File net/dns/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:24: enum RequestOutcome {
On 2016/04/28 17:48:25, Randy Smith - Not in Fridays wrote:
> nit, suggestion (I'll stop making these once you argue with me once :-}): I
find
> it hard to parse out what comments belong with what enums in this type of
> commenting style and would prefer a space between an enum and the next enums
> comment.

Happily done, as I still get my 80 characters to write a description :)

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:44: void
HistogramAddressListDelta(AddressListDeltaType delta) {
On 2016/04/28 17:48:25, Randy Smith - Not in Fridays wrote:
> Ah.  Feel free to ignore my comment on the other CL :-}.

Done.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:68: // Starts the request. May call
|usable_callback| inline if |resolver| returns
On 2016/04/28 17:48:25, Randy Smith - Not in Fridays wrote:
> I'm scared about interfaces that are variations (especially more complex
> variations) on the usual netstack "return value in synchronous return, call
> callback if async".  There's potential complex re-entry problems with calling
> callbacks synchronously (Elly tried for filters, Matt shot her down, I don't
> remember the issues) and there's potentially complex race issues if you've got
> several callbacks running around.

|usable_callback| isn't a completion callback; it's a lambda to compare the
staleness to some cutoffs.

|callback| isn't actually called from Start; the comment is wrong. (I fixed it.)

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:69: // stale data. May call the callback passed
into the constructor inline if
On 2016/04/28 17:48:25, Randy Smith - Not in Fridays wrote:
> What callback passed into the constructor?

Done.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:77: const StaleEntryUsableCallback&
usable_callback,
On 2016/04/28 17:48:25, Randy Smith - Not in Fridays wrote:
> I'm having a hard time mapping the implementation to the above documentation
> around this callback.  It looks like it's used for information, i.e. to return
a
> bool to determine if a stale entry should be returned.  If that's the case,
> could you update the doc above to make it clearer?  (I don't have any
objections
> to using the callback that way--I thought from the above description that it
was
> being used to pass results back to the caller, which I have more problems
with.)

Done.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:85: void OnStaleDelayElapsed();
On 2016/04/29 01:04:09, Ryan Sleevi wrote:
> Pedantry: Seems like it could be private?

Done.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:88: void OnNetworkRequestComplete(int error);
On 2016/04/29 01:04:09, Ryan Sleevi wrote:
> Pedantry: Seems like it could be private?

Done.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:180: callback_.Reset();
On 2016/04/29 01:04:08, Ryan Sleevi wrote:
> Maybe add a note as to why you don't delete this
> 
> I was trying to figure it out, but my guess is that by not deleting this, not
> deleting this is what we use to continue the network request, with the
> expectation that
> 1) No one will ever call Cancel (since we called HandleResult)
> 2) Eventually the request will complete

Done.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:210: if (stale_timer_.IsRunning() ||
returned_stale_result()) {
On 2016/04/29 01:04:09, Ryan Sleevi wrote:
> nit: no braces for consistency with the rest of the file

Done.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.cc:259: for (auto request : pending_requests_)
On 2016/04/29 01:04:09, Ryan Sleevi wrote:
> Shouldn't this be "auto&" or "auto*" (based on
https://chromium-cpp.appspot.com/
> - I believe auto*)

Done.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
File net/dns/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:28: // If set, stale data from previous networks
is usable; if clear, it's not.
On 2016/04/29 01:04:09, Ryan Sleevi wrote:
> Suggestion: Explain a little more about the trade-offs here - what the risks
are
> about using stale data from other networks. It's not clear from here (but
> perhaps I missed it in another CL or documentation)?
> 
> Maybe a scenario where it can improve good results, and a scenario where it
can
> cause bad results, so that callers have some conceptual overtones about the
> trade-offs involved.

Done.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:34: // request potentially stale data.)
On 2016/04/28 17:48:25, Randy Smith - Not in Fridays wrote:
> nit: How hard would it be to change this?  I suspect your current use cases
> don't require it, but to my mind it's a layering violation, so I'd like to at
> least explore doing it right.

We could keep a std::map<HostCache::Key, int> stale_uses_ in the
StaleHostResolver, increment it on returning a stale entry, and clear it on...
setting an entry? but we don't actually know when that happens, because it
happens in the inner HostResolverImpl.

It's a layering violation, except the HostCache is created by the
HostResolverImpl, and then the StaleHostResolver takes ownership of the
HostResolverImpl, so it can't actually be shared once the StaleHostResolver is
created unless someone kept a pointer to the HostResolverImpl.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:39: // resolution but potentially returns stale
data according to |stale_options|.
On 2016/04/29 01:04:09, Ryan Sleevi wrote:
> s/resolution but/resolution, but/

Done.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:47: // after the specified delay instead.
On 2016/04/28 17:48:25, Randy Smith - Not in Fridays wrote:
> I presume if fresh data is available in that time period, it's returned
> preferentially?  The doc doesn't make that clear.

Clarified.

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:66: 
On 2016/04/29 01:04:09, Ryan Sleevi wrote:
> delete newline?

Done.

[Ryan Sleevi, 2016-04-29 23:04:14]

I'll let Randy take the rest, but a few drivebys. Otherwise, Looks Good To Me.

https://codereview.chromium.org/1898873006/diff/100001/net/dns/stale_host_res...
File net/dns/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/100001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:188: callback_.Reset();
fwiw, you could use

base::ResetAndReturn(&callback_).Run(HandleResult(stale_error_,
stale_addresses_));

https://codereview.chromium.org/1898873006/diff/100001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:254: }
consistency: no braces for single-line

https://codereview.chromium.org/1898873006/diff/100001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:295: *out_req =
reinterpret_cast<RequestHandle>(request);
What if rv == ERR_IO_PENDING and there is no |*out_req|. You leak |request|
then.

Suggestion: |out_req| should never be optional for a method like this - DCHECK
it on coming in to make sure someone's ready to handle async.

[Julia Tuttle, 2016-05-03 20:47:26]

PTAL.

https://codereview.chromium.org/1898873006/diff/100001/net/dns/stale_host_res...
File net/dns/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/100001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:188: callback_.Reset();
On 2016/04/29 23:04:14, Ryan Sleevi wrote:
> fwiw, you could use
> 
> base::ResetAndReturn(&callback_).Run(HandleResult(stale_error_,
> stale_addresses_));

Awesome, thanks.

https://codereview.chromium.org/1898873006/diff/100001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:254: }
On 2016/04/29 23:04:14, Ryan Sleevi wrote:
> consistency: no braces for single-line

Done.

https://codereview.chromium.org/1898873006/diff/100001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:295: *out_req =
reinterpret_cast<RequestHandle>(request);
On 2016/04/29 23:04:14, Ryan Sleevi wrote:
> What if rv == ERR_IO_PENDING and there is no |*out_req|. You leak |request|
> then.
> 
> Suggestion: |out_req| should never be optional for a method like this - DCHECK
> it on coming in to make sure someone's ready to handle async.

It doesn't leak -- it just means the caller can't cancel it.

It will delete *itself* when the request finishes, just like
HostResolverImpl::Job does.

[Randy Smith (Not in Mondays), 2016-05-04 21:00:29]

Next round of comments.  Ryan, if you're still reading, I wouldn't mind having
your input in the last issue I raise below (the conflict, at least in my mind,
between using inheritance of the HostResolver interface to change the behavior
of Resolve while still exposing staleness primitives).

Note to self: I still need to evaluate the histograming.

What are you plans for testing?

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
File net/dns/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/80001/net/dns/stale_host_reso...
net/dns/stale_host_resolver.h:34: // request potentially stale data.)
On 2016/04/29 18:04:30, Julia Tuttle wrote:
> On 2016/04/28 17:48:25, Randy Smith - Not in Fridays wrote:
> > nit: How hard would it be to change this?  I suspect your current use cases
> > don't require it, but to my mind it's a layering violation, so I'd like to
at
> > least explore doing it right.
> 
> We could keep a std::map<HostCache::Key, int> stale_uses_ in the
> StaleHostResolver, increment it on returning a stale entry, and clear it on...
> setting an entry? but we don't actually know when that happens, because it
> happens in the inner HostResolverImpl.
> 
> It's a layering violation, except the HostCache is created by the
> HostResolverImpl, and then the StaleHostResolver takes ownership of the
> HostResolverImpl, so it can't actually be shared once the StaleHostResolver is
> created unless someone kept a pointer to the HostResolverImpl.

So currently StaleHostResolver owns (does not share) the inner HostResolver,
and, when that inner HostResolver is actually a HostResolverImpl, the
HostResolverImpl owns (does not share) a HostCache.  So in that case, there's no
problem, because the only place that hits can come from is through this class. 
That right?

If so, I think we can solve this problem by simply documenting that
HostResolver::ResolveStale will return hit information in |*stale_info| only
based on lookups through that HostResolver.  That right?

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
File net/dns/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:66: // network data, or stale cached data.
Please document the ownership and lifetime model for Request, especially since
at first glance it seems a little weird :-}.  (I'd feel a bit more comfortable
if it was owned and deleted by StaleHostResolver, instead of self-owned, but
that's just because it's a model I'm more used to.)

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:76: // stale data to let the caller decide
whether the data is usable.
I'm assuming that this is the standard "return OK+set *addresses || return
ERR_IO_PENDING -> calling callback, having filled in *addresses || return other
net error code"; that right?  I have a slight preference for documenting that
even if it is the standard, but I'm willing to leave that up to you (internal
class in .cc file and all that).

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:80: AddressList* addresses,
nit: Could you document that the caller must guarantee that |*addresses| remains
valid for the lifetime of the Request?

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:110: void HistogramSynchronousRequest();
nit, suggestion: Change these names to match the other CL, i.e. using "Record"
as the verb rather than "Histogram"?

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:145: base::Unretained(this));
What guarantees that the Request will stick around until the network_callback
returns?  I don't see anything canceling the request at the host resolver level
in the destructor.  (There are totally other ways to do it, I just pull towards
that one since it keeps the logic all in the Request class.)

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:147:
&StaleHostResolver::Request::OnStaleDelayElapsed, base::Unretained(this));
nit, suggestion: If you don't bother with a local variable for this and instead
just include the bind in the argument to stale_timer_.Start(), you implicit
answer the question around why base::Unretained() is safe.  Otherwise, I might
request a comment :-}.

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:296: return rv;
Doesn't this leak request if request->Start() returns synchronously?

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
File net/dns/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.h:70: // The remaining public methods pass through
to the inner resolver:
So there's a code smell here, and I'm really not sure what to do about it.

The problem, as I see it, is that as of this CL, we're officially using the
HostResolver interface in two different ways.  We are *both* exposing primitives
of resolution (ResolveFromCache, ResolveStaleFromCache) and exposing a "wrap-up"
function Resolve() that exports some behavior that is implementation-dependent. 
The implementation dependent part is fine (it's sorta why abstract base classes
exist), but it's weird that we're doing both in the same interface; I'm used to
the entire interface changing in some reasonable way based on the underlying
implementation, not just one function.

I see this as being basically about the different consumers for HostResolver. 
Within the network stack, we want access to the primitives, to do neat and
interesting things with them (e.g. racing a connection to a stale address with
verifying it with the DNS resolver), but for consumers of the network stack, we
want a simple interface that can be used to "just give me the address associated
with this hostname, already".  Arguably, we shouldn't be using HostResolver for
both of those goals, but I haven't come up with an inheritance model that
wouldn't be over-complex that we could move to. 

Worse, I say "Resolve()" is a wrap-up function, but if I look at the comments in
HostResolver, ResolveStale() is explicitly defined as a function that wraps
Resolve(), and ResolveStaleFromCache() to get a combination of fresh & stale
data.  That comment implicitly documents Resolve() as *not* returning stale
data, so I'm a bit concerned about then wrapping it in a derived class so that
it sometimes *does* return stale data.

So let me make a proposal (but I'm not wedded to this, feel absolutely free to
make a counter-proposal): Create a base class method ResolveFresh() that
explicitly never returns stale data.  Make Resolve() be a pure virtual function
that derived classes have to implement.  Document ResolveStale() as being based
on ResolveFresh() and ResolveStaleFromCache() (though maybe it's name should be
changed so it's not parallel to ResolveFresh()).  This keeps the "One interface
serving two consumers" problem, but it keeps the documentation of the different
interfaces cleaner, at least in my mind. 

WDYT?

[Ryan Sleevi, 2016-05-04 21:14:38]

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
File net/dns/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.h:70: // The remaining public methods pass through
to the inner resolver:
On 2016/05/04 21:00:28, Randy Smith - Not in Fridays wrote:
> So there's a code smell here, and I'm really not sure what to do about it.
> 
> The problem, as I see it, is that as of this CL, we're officially using the
> HostResolver interface in two different ways.  We are *both* exposing
primitives
> of resolution (ResolveFromCache, ResolveStaleFromCache) and exposing a
"wrap-up"
> function Resolve() that exports some behavior that is
implementation-dependent. 
> The implementation dependent part is fine (it's sorta why abstract base
classes
> exist), but it's weird that we're doing both in the same interface; I'm used
to
> the entire interface changing in some reasonable way based on the underlying
> implementation, not just one function.
> 
> I see this as being basically about the different consumers for HostResolver. 
> Within the network stack, we want access to the primitives, to do neat and
> interesting things with them (e.g. racing a connection to a stale address with
> verifying it with the DNS resolver), but for consumers of the network stack,
we
> want a simple interface that can be used to "just give me the address
associated
> with this hostname, already".  Arguably, we shouldn't be using HostResolver
for
> both of those goals, but I haven't come up with an inheritance model that
> wouldn't be over-complex that we could move to. 
> 
> Worse, I say "Resolve()" is a wrap-up function, but if I look at the comments
in
> HostResolver, ResolveStale() is explicitly defined as a function that wraps
> Resolve(), and ResolveStaleFromCache() to get a combination of fresh & stale
> data.  That comment implicitly documents Resolve() as *not* returning stale
> data, so I'm a bit concerned about then wrapping it in a derived class so that
> it sometimes *does* return stale data.
> 
> So let me make a proposal (but I'm not wedded to this, feel absolutely free to
> make a counter-proposal): Create a base class method ResolveFresh() that
> explicitly never returns stale data.  Make Resolve() be a pure virtual
function
> that derived classes have to implement.  Document ResolveStale() as being
based
> on ResolveFresh() and ResolveStaleFromCache() (though maybe it's name should
be
> changed so it's not parallel to ResolveFresh()).  This keeps the "One
interface
> serving two consumers" problem, but it keeps the documentation of the
different
> interfaces cleaner, at least in my mind. 
> 
> WDYT?

I'm not going to be able to offer review feedback on this until next  Monday,
but I'm not a big fan of this, although I may not be understanding it.

That said, Randy, use your judgement. I may have misunderstood your arguments,
but alas, I won't have the time today to deal with the meat of them, and I'm OOO
Th/Fri.

[Julia Tuttle, 2016-05-10 16:13:14]

Hey rsleevi, can you comment further on how you feel about the HostResolver
interface?

Were you expressing distaste for my original version, or Randy's proposed
change?

I'm open to other ideas, although I think we have to maintain the invariant that
StaleHostResolver can masquerade as a regular HostResolver to avoid having to
rewrite every call site.

[Randy Smith (Not in Mondays), 2016-05-10 16:32:30]

On 2016/05/10 16:13:14, Julia Tuttle wrote:
> Hey rsleevi, can you comment further on how you feel about the HostResolver
> interface?
> 
> Were you expressing distaste for my original version, or Randy's proposed
> change?
> 
> I'm open to other ideas, although I think we have to maintain the invariant
that
> StaleHostResolver can masquerade as a regular HostResolver to avoid having to
> rewrite every call site.

To inform this discussion, can you summarize what consumers use what interfaces?
 I'm specifically wondering about the use of the ResolveStale and
ResolveStaleFromCache interfaces; maybe we can peel those off?

[Julia Tuttle, 2016-05-10 17:21:38]

On 2016/05/10 16:32:30, Randy Smith - Not in Fridays wrote:
> On 2016/05/10 16:13:14, Julia Tuttle wrote:
> > Hey rsleevi, can you comment further on how you feel about the HostResolver
> > interface?
> > 
> > Were you expressing distaste for my original version, or Randy's proposed
> > change?
> > 
> > I'm open to other ideas, although I think we have to maintain the invariant
> that
> > StaleHostResolver can masquerade as a regular HostResolver to avoid having
to
> > rewrite every call site.
> 
> To inform this discussion, can you summarize what consumers use what
interfaces?
>  I'm specifically wondering about the use of the ResolveStale and
> ResolveStaleFromCache interfaces; maybe we can peel those off?

Sure, here's what will call them:

1. Soon, StaleHostResolver.
2. Next, TransportConnectJob, moving the stale reuse logic up a layer so it can
act on the fresh DNS response too.
3. Eventually, other parts of Chrome (notably, the QUIC code) that want to
perform the same optimization.

[Randy Smith (Not in Mondays), 2016-05-10 18:02:19]

On 2016/05/10 17:21:38, Julia Tuttle wrote:
> On 2016/05/10 16:32:30, Randy Smith - Not in Fridays wrote:
> > On 2016/05/10 16:13:14, Julia Tuttle wrote:
> > > Hey rsleevi, can you comment further on how you feel about the
HostResolver
> > > interface?
> > > 
> > > Were you expressing distaste for my original version, or Randy's proposed
> > > change?
> > > 
> > > I'm open to other ideas, although I think we have to maintain the
invariant
> > that
> > > StaleHostResolver can masquerade as a regular HostResolver to avoid having
> to
> > > rewrite every call site.
> > 
> > To inform this discussion, can you summarize what consumers use what
> interfaces?
> >  I'm specifically wondering about the use of the ResolveStale and
> > ResolveStaleFromCache interfaces; maybe we can peel those off?
> 
> Sure, here's what will call them:
> 
> 1. Soon, StaleHostResolver.
> 2. Next, TransportConnectJob, moving the stale reuse logic up a layer so it
can
> act on the fresh DNS response too.
> 3. Eventually, other parts of Chrome (notably, the QUIC code) that want to
> perform the same optimization.

How is StaleHostResolver going to change when the TransportConnectJob starts
explicitly using the stale entry points?  Will it stop calling Resolve()?  If
so, what will call Resolve?

[Julia Tuttle, 2016-05-10 18:17:00]

On 2016/05/10 18:02:19, Randy Smith - Not in Fridays wrote:
> On 2016/05/10 17:21:38, Julia Tuttle wrote:
> > On 2016/05/10 16:32:30, Randy Smith - Not in Fridays wrote:
> > > On 2016/05/10 16:13:14, Julia Tuttle wrote:
> > > > Hey rsleevi, can you comment further on how you feel about the
> HostResolver
> > > > interface?
> > > > 
> > > > Were you expressing distaste for my original version, or Randy's
proposed
> > > > change?
> > > > 
> > > > I'm open to other ideas, although I think we have to maintain the
> invariant
> > > that
> > > > StaleHostResolver can masquerade as a regular HostResolver to avoid
having
> > to
> > > > rewrite every call site.
> > > 
> > > To inform this discussion, can you summarize what consumers use what
> > interfaces?
> > >  I'm specifically wondering about the use of the ResolveStale and
> > > ResolveStaleFromCache interfaces; maybe we can peel those off?
> > 
> > Sure, here's what will call them:
> > 
> > 1. Soon, StaleHostResolver.
> > 2. Next, TransportConnectJob, moving the stale reuse logic up a layer so it
> can
> > act on the fresh DNS response too.
> > 3. Eventually, other parts of Chrome (notably, the QUIC code) that want to
> > perform the same optimization.
> 
> How is StaleHostResolver going to change when the TransportConnectJob starts
> explicitly using the stale entry points?  Will it stop calling Resolve()?  If
> so, what will call Resolve?

Eventually, once TransportConnectJob and company start using the stale entry
points, StaleHostResolver could go away. It's basically a short-term shim to get
a good fraction of the stale DNS behavior with less work.

[Ryan Sleevi, 2016-05-10 18:52:15]

On 2016/05/10 18:17:00, Julia Tuttle wrote:
> Eventually, once TransportConnectJob and company start using the stale entry
> points, StaleHostResolver could go away. It's basically a short-term shim to
get
> a good fraction of the stale DNS behavior with less work.

Now I'm even more confused; what sort of 'less work' are we talking about?

It sounds like you're saying the plan is:
- Introduce new methods to the interface (HostResolver)
- Move all callers to the new methods
- Change the object to use a different interface (???)
- Remove the new method from the old interface (HostResolver)

It's not clear how that's fundamentally different from:
- Introduce a new object with a specific (composed) interface that has methods
similar to the old interface (HostResolver)
- Change the object being passed to use the new interface
- Move all callers to the new (Stale) methods

Seems like that's less work (or less summarized steps)

And my concerns were with Randy's proposal.

Do you have a (public) Design Doc or Implementation Strategy that might be
reviewed to understand? In general, these sorts of "transition from the old to
the new" get stalled out for all sorts of reason, so having a clear documented
plan of record and an end goal can help keep things on progress, and when we run
into dead ends, also clarify what work needs to be rolled back to try a
different strategy.

[Ryan Sleevi, 2016-05-10 18:59:13]

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
File net/dns/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.h:70: // The remaining public methods pass through
to the inner resolver:
On 2016/05/04 21:00:28, Randy Smith - Not in Fridays wrote:
> So there's a code smell here, and I'm really not sure what to do about it.
> 
> The problem, as I see it, is that as of this CL, we're officially using the
> HostResolver interface in two different ways.  We are *both* exposing
primitives
> of resolution (ResolveFromCache, ResolveStaleFromCache) and exposing a
"wrap-up"
> function Resolve() that exports some behavior that is
implementation-dependent. 
> The implementation dependent part is fine (it's sorta why abstract base
classes
> exist), but it's weird that we're doing both in the same interface; I'm used
to
> the entire interface changing in some reasonable way based on the underlying
> implementation, not just one function.

Right, I totally agree with Randy here.

> I see this as being basically about the different consumers for HostResolver. 
> Within the network stack, we want access to the primitives, to do neat and
> interesting things with them (e.g. racing a connection to a stale address with
> verifying it with the DNS resolver), but for consumers of the network stack,
we
> want a simple interface that can be used to "just give me the address
associated
> with this hostname, already".  Arguably, we shouldn't be using HostResolver
for
> both of those goals, but I haven't come up with an inheritance model that
> wouldn't be over-complex that we could move to. 

Right, I totally agree with Randy here.

> Worse, I say "Resolve()" is a wrap-up function, but if I look at the comments
in
> HostResolver, ResolveStale() is explicitly defined as a function that wraps
> Resolve(), and ResolveStaleFromCache() to get a combination of fresh & stale
> data.  That comment implicitly documents Resolve() as *not* returning stale
> data, so I'm a bit concerned about then wrapping it in a derived class so that
> it sometimes *does* return stale data.

Right, I totally agree with Randy here.

> So let me make a proposal (but I'm not wedded to this, feel absolutely free to
> make a counter-proposal): Create a base class method ResolveFresh() that
> explicitly never returns stale data.  Make Resolve() be a pure virtual
function
> that derived classes have to implement.  Document ResolveStale() as being
based
> on ResolveFresh() and ResolveStaleFromCache() (though maybe it's name should
be
> changed so it's not parallel to ResolveFresh()).  This keeps the "One
interface
> serving two consumers" problem, but it keeps the documentation of the
different
> interfaces cleaner, at least in my mind. 
> 
> WDYT?

This is where I got uncomfortable. "One interface serving two consumers" is
still feeling like a nasty code-smell (good ol "Single Responsibility Principle"
in SOLID). Another reason I'm sort of squicky about this is it feels like it
exposes details about resolution strategies on the base class; it really feels
like you should be dealing with a separate object that composes over
HostResolver and does the (ResolveFromCache/Resolve) calls, and I'm not sure
"HostResolver" is the right interface for that.

That said, after re-reading, I think it's better than what's present, but it
still leaves me a bit squicky about the interfaces and behaviours and such.

[Randy Smith (Not in Mondays), 2016-05-11 17:46:38]

On 2016/05/10 18:59:13, Ryan Sleevi wrote:
>
https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
> File net/dns/stale_host_resolver.h (right):
> 
>
https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
> net/dns/stale_host_resolver.h:70: // The remaining public methods pass through
> to the inner resolver:
> On 2016/05/04 21:00:28, Randy Smith - Not in Fridays wrote:
> > So there's a code smell here, and I'm really not sure what to do about it.
> > 
> > The problem, as I see it, is that as of this CL, we're officially using the
> > HostResolver interface in two different ways.  We are *both* exposing
> primitives
> > of resolution (ResolveFromCache, ResolveStaleFromCache) and exposing a
> "wrap-up"
> > function Resolve() that exports some behavior that is
> implementation-dependent. 
> > The implementation dependent part is fine (it's sorta why abstract base
> classes
> > exist), but it's weird that we're doing both in the same interface; I'm used
> to
> > the entire interface changing in some reasonable way based on the underlying
> > implementation, not just one function.
> 
> Right, I totally agree with Randy here.
> 
> > I see this as being basically about the different consumers for
HostResolver. 
> > Within the network stack, we want access to the primitives, to do neat and
> > interesting things with them (e.g. racing a connection to a stale address
with
> > verifying it with the DNS resolver), but for consumers of the network stack,
> we
> > want a simple interface that can be used to "just give me the address
> associated
> > with this hostname, already".  Arguably, we shouldn't be using HostResolver
> for
> > both of those goals, but I haven't come up with an inheritance model that
> > wouldn't be over-complex that we could move to. 
> 
> Right, I totally agree with Randy here.
> 
> > Worse, I say "Resolve()" is a wrap-up function, but if I look at the
comments
> in
> > HostResolver, ResolveStale() is explicitly defined as a function that wraps
> > Resolve(), and ResolveStaleFromCache() to get a combination of fresh & stale
> > data.  That comment implicitly documents Resolve() as *not* returning stale
> > data, so I'm a bit concerned about then wrapping it in a derived class so
that
> > it sometimes *does* return stale data.
> 
> Right, I totally agree with Randy here.
> 
> > So let me make a proposal (but I'm not wedded to this, feel absolutely free
to
> > make a counter-proposal): Create a base class method ResolveFresh() that
> > explicitly never returns stale data.  Make Resolve() be a pure virtual
> function
> > that derived classes have to implement.  Document ResolveStale() as being
> based
> > on ResolveFresh() and ResolveStaleFromCache() (though maybe it's name should
> be
> > changed so it's not parallel to ResolveFresh()).  This keeps the "One
> interface
> > serving two consumers" problem, but it keeps the documentation of the
> different
> > interfaces cleaner, at least in my mind. 
> > 
> > WDYT?
> 
> This is where I got uncomfortable. "One interface serving two consumers" is
> still feeling like a nasty code-smell (good ol "Single Responsibility
Principle"
> in SOLID). Another reason I'm sort of squicky about this is it feels like it
> exposes details about resolution strategies on the base class; it really feels
> like you should be dealing with a separate object that composes over
> HostResolver and does the (ResolveFromCache/Resolve) calls, and I'm not sure
> "HostResolver" is the right interface for that.
> 
> That said, after re-reading, I think it's better than what's present, but it
> still leaves me a bit squicky about the interfaces and behaviours and such.

So one obvious choice is to accept that we're in hack territory and try to put
in context and constraints so that it's temporary.  And I'm ok with that, if the
alternatives are large amounts of work.  But I want to float another possible
high level alternative before giving up.

I think that we're in the position of wanting to provide the StaleHostResolver
implementation of the interface in a temporary fashion to change behavior for
some clients pending actually integrating use of stale information in a more
nuanced way in the network stack.  I.e. right now we want the simple interface
with abstraction used to change behavior, and in the future we want the complex
interface which we will not then use to make Resolve() behave in a funny way.

So why don't we actually plan on transitioning between those two worlds in one
CL?  Concretely, why don't we *not* expose the various stale primitives on
HostResolver (only on HostResolverImpl), and have StaleHostResolver wrap
HostResolverImpl (rather than an arbitrary HostResolver).  When the
TransportConnectJob (or other consumers of the interface) need the detailed
methods, at that point we nuke StaleHostResolver because we'll be providing the
same functionality (in a different way) to the clients we created
StaleHostResovler for?

I'm sure there are problems with this plan, but I want to at least explore it
before compromising my principles :-}.

[Ryan Sleevi, 2016-05-11 17:51:13]

On 2016/05/11 17:46:38, Randy Smith - Not in Fridays wrote:
> So why don't we actually plan on transitioning between those two worlds in one
> CL?  Concretely, why don't we *not* expose the various stale primitives on
> HostResolver (only on HostResolverImpl), and have StaleHostResolver wrap
> HostResolverImpl (rather than an arbitrary HostResolver).  When the
> TransportConnectJob (or other consumers of the interface) need the detailed
> methods, at that point we nuke StaleHostResolver because we'll be providing
the
> same functionality (in a different way) to the clients we created
> StaleHostResovler for?
> 
> I'm sure there are problems with this plan, but I want to at least explore it
> before compromising my principles :-}.

I'm with you up until the "When the" bits, and I'm having trouble making sense
of what you see the world looking like. Could you perhaps spell out at a
high-level some of the changes you see (e.g. change X to take Y/call Y::Z()) ?

If I'm parsing correctly, you see changing the dependency injected from being a
HostResolver* to a StaleHostResolver* (an entirely separate class/abstraction
that shares similar method names, but no shared class ancestry). However, I'm
not sure what the "nuke StaleHostResolver" implies - changing the dependency
back to a HostResolver*, and injecting some new object? Having
TransportConnectJob reach directly into the HostResolverImpl? Some other thing?

[Randy Smith (Not in Mondays), 2016-05-11 18:02:29]

On 2016/05/11 17:51:13, Ryan Sleevi wrote:
> On 2016/05/11 17:46:38, Randy Smith - Not in Fridays wrote:
> > So why don't we actually plan on transitioning between those two worlds in
one
> > CL?  Concretely, why don't we *not* expose the various stale primitives on
> > HostResolver (only on HostResolverImpl), and have StaleHostResolver wrap
> > HostResolverImpl (rather than an arbitrary HostResolver).  When the
> > TransportConnectJob (or other consumers of the interface) need the detailed
> > methods, at that point we nuke StaleHostResolver because we'll be providing
> the
> > same functionality (in a different way) to the clients we created
> > StaleHostResovler for?
> > 
> > I'm sure there are problems with this plan, but I want to at least explore
it
> > before compromising my principles :-}.
> 
> I'm with you up until the "When the" bits, and I'm having trouble making sense
> of what you see the world looking like. Could you perhaps spell out at a
> high-level some of the changes you see (e.g. change X to take Y/call Y::Z()) ?
> 
> If I'm parsing correctly, you see changing the dependency injected from being
a
> HostResolver* to a StaleHostResolver* (an entirely separate class/abstraction
> that shares similar method names, but no shared class ancestry). However, I'm
> not sure what the "nuke StaleHostResolver" implies - changing the dependency
> back to a HostResolver*, and injecting some new object? Having
> TransportConnectJob reach directly into the HostResolverImpl? Some other
thing?

No, sorry, I'm saying the dependency injection stays "HostResolver", we just
switch how that ABC is used atomically at a future CL.  So when we need the
detailed methods to be available via HostResolver from, e.g., the
TransportConnectJob, we can also at that point get rid of StaleHostResolver
because the abstraction that StaleHostResolver was for will be done better in
TCJ.

This is based on a realization I had a little ways back, which is that while
StaleHostResolver is being created for the use by specific clients who want to
change how DNS lookups are done, the primary *client* for StaleHostResolver is
TransportConnectJob; those "specific clients" aren't calling into it directly. 
When TCJ actually starts being aware of stale entries and manipulating them
directly, that seems like exactly the right time to get rid of the
StaleHostResolver implementation and its implications for the ways in which
HostResolver::Resolve() might be abstracted.

[Ryan Sleevi, 2016-05-11 18:40:16]

On 2016/05/11 18:02:29, Randy Smith - Not in Fridays wrote:
> No, sorry, I'm saying the dependency injection stays "HostResolver", we just
> switch how that ABC is used atomically at a future CL.  

Again, confused about what "ABC" is

> So when we need the
> detailed methods to be available via HostResolver from, e.g., the
> TransportConnectJob, we can also at that point get rid of StaleHostResolver
> because the abstraction that StaleHostResolver was for will be done better in
> TCJ.

So, I'm confused here (and perhaps there's context spread over multiple CLs that
I'm missing, or perhaps there's context on this CL that I'm missing) - why have
the StaleHostResolver at all?

This is where the design doc/explanation helps, because the bug doesn't, and
perhaps you have more mental state mapped in then this CL. Are you suggesting
that the behaviour of StaleHostResolver::Resolve is that it always returns Stale
(if available) or refreshes (if not)? Meaning, it implements some pattern of
behaviour different from HostResolver? I'm not sure if that's compatible with
the is-a HostResolver, but maybe it entirely is (again, seems there's broader
state). I guess what's unclear to me is how you're suggesting StaleHostResolver
behave, and is used, if it lacks the additional methods.

It's also not clear how that transition plan looks like in your head, so I'm
still hoping you can share details. My incomplete understanding is you're
suggesting
- StaleHostResolver::Resolve internally encapsulates all the magic logic (no
changes to HostResolver or HostResolverImpl)
- At some later point, HostResolver gets additional methods (which implies
StaleHostResolver also gets those methods)
- At some later point after that, TCJ starts using those additional methods in
some TCJ-specific way
- At some later point after that, StaleHostResolver goes away

It's not clear to me that this logic about staleness belongs in TCJ - since, as
you note, it seems about specific clients' behaviours rather than general //net
behaviour, but perhaps I'm missing how intrinsically linked the notion of stale
resolution is.

I'm generally uncomfortable with using stale results in Chrome, but then again,
I'm generally uncomfortable with most "performance hacks". The use of stale
information seems like it makes it harder to reason about what the browser will
do, or how servers can reduce load, or any of the other guarantees that work by
layering DNS, sockets, etc - but maybe I'm just an ideological purist. It is in
that context that I assumed this was "Cronet only" - but it sounds like you see
this being "Chrome as well".

If it is for Chrome, why not do the ordered thing - why does StaleHostResolver
need to be introduced at all? Why can't HostResolver add the methods, TCJ start
using them, everyone benefits? What's the value in the transition plan - it
seems like StaleHostResolver is just an attempt to rush code to 'shipping'
faster, but adds more complexity. But again, I'm almost certainly missing some
bigger picture context.

[Randy Smith (Not in Mondays), 2016-05-11 18:46:38]

On 2016/05/11 18:40:16, Ryan Sleevi wrote:
> On 2016/05/11 18:02:29, Randy Smith - Not in Fridays wrote:
> > No, sorry, I'm saying the dependency injection stays "HostResolver", we just
> > switch how that ABC is used atomically at a future CL.  
> 
> Again, confused about what "ABC" is
> 
> > So when we need the
> > detailed methods to be available via HostResolver from, e.g., the
> > TransportConnectJob, we can also at that point get rid of StaleHostResolver
> > because the abstraction that StaleHostResolver was for will be done better
in
> > TCJ.
> 
> So, I'm confused here (and perhaps there's context spread over multiple CLs
that
> I'm missing, or perhaps there's context on this CL that I'm missing) - why
have
> the StaleHostResolver at all?
> 
> This is where the design doc/explanation helps, because the bug doesn't, and
> perhaps you have more mental state mapped in then this CL. Are you suggesting
> that the behaviour of StaleHostResolver::Resolve is that it always returns
Stale
> (if available) or refreshes (if not)? Meaning, it implements some pattern of
> behaviour different from HostResolver? I'm not sure if that's compatible with
> the is-a HostResolver, but maybe it entirely is (again, seems there's broader
> state). I guess what's unclear to me is how you're suggesting
StaleHostResolver
> behave, and is used, if it lacks the additional methods.
> 
> It's also not clear how that transition plan looks like in your head, so I'm
> still hoping you can share details. My incomplete understanding is you're
> suggesting
> - StaleHostResolver::Resolve internally encapsulates all the magic logic (no
> changes to HostResolver or HostResolverImpl)
> - At some later point, HostResolver gets additional methods (which implies
> StaleHostResolver also gets those methods)
> - At some later point after that, TCJ starts using those additional methods in
> some TCJ-specific way
> - At some later point after that, StaleHostResolver goes away
> 
> It's not clear to me that this logic about staleness belongs in TCJ - since,
as
> you note, it seems about specific clients' behaviours rather than general
//net
> behaviour, but perhaps I'm missing how intrinsically linked the notion of
stale
> resolution is.
> 
> I'm generally uncomfortable with using stale results in Chrome, but then
again,
> I'm generally uncomfortable with most "performance hacks". The use of stale
> information seems like it makes it harder to reason about what the browser
will
> do, or how servers can reduce load, or any of the other guarantees that work
by
> layering DNS, sockets, etc - but maybe I'm just an ideological purist. It is
in
> that context that I assumed this was "Cronet only" - but it sounds like you
see
> this being "Chrome as well".
> 
> If it is for Chrome, why not do the ordered thing - why does StaleHostResolver
> need to be introduced at all? Why can't HostResolver add the methods, TCJ
start
> using them, everyone benefits? What's the value in the transition plan - it
> seems like StaleHostResolver is just an attempt to rush code to 'shipping'
> faster, but adds more complexity. But again, I'm almost certainly missing some
> bigger picture context.

I think these are all good questions, but I'm going to leave most of the
response to Julia; I'll just say "ABC == Abstract Base Class".

[Julia Tuttle, 2016-05-11 20:32:40]

It is an entirely fair concern that this is unacceptably rushed, and we should
just do it in TransportConnectJob, but this is also somewhat simpler. (Doing it
in TransportConnectJob presents us with other questions, like what to do if we
act on the stale data but then fresh data arrives while the stale connection is
still pending.)

My plan is that Chrome will never use stale results unverified; it will simply
start the TCP connection earlier by speculating that the stale data might be
right, and will return that socket *once the DNS response comes back* if it's
connected to one of the addresses in the response.

[Julia Tuttle, 2016-05-11 20:35:10]

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
File net/dns/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:66: // network data, or stale cached data.
On 2016/05/04 21:00:28, Randy Smith - Not in Fridays wrote:
> Please document the ownership and lifetime model for Request, especially since
> at first glance it seems a little weird :-}.  (I'd feel a bit more comfortable
> if it was owned and deleted by StaleHostResolver, instead of self-owned, but
> that's just because it's a model I'm more used to.)

It is a little of both :)

Request is self-owned, but StaleHostResolver will cancel pending jobs at
destruction.

(See the creation of |wrapped_callback| in Resolve -- that's how
StaleHostResolver keeps track of pending jobs.)

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:76: // stale data to let the caller decide
whether the data is usable.
On 2016/05/04 21:00:28, Randy Smith - Not in Fridays wrote:
> I'm assuming that this is the standard "return OK+set *addresses || return
> ERR_IO_PENDING -> calling callback, having filled in *addresses || return
other
> net error code"; that right?  I have a slight preference for documenting that
> even if it is the standard, but I'm willing to leave that up to you (internal
> class in .cc file and all that).

Done.

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:80: AddressList* addresses,
On 2016/05/04 21:00:28, Randy Smith - Not in Fridays wrote:
> nit: Could you document that the caller must guarantee that |*addresses|
remains
> valid for the lifetime of the Request?

Done.

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:110: void HistogramSynchronousRequest();
On 2016/05/04 21:00:28, Randy Smith - Not in Fridays wrote:
> nit, suggestion: Change these names to match the other CL, i.e. using "Record"
> as the verb rather than "Histogram"?

Done.

https://codereview.chromium.org/1898873006/diff/140001/net/dns/stale_host_res...
net/dns/stale_host_resolver.cc:147:
&StaleHostResolver::Request::OnStaleDelayElapsed, base::Unretained(this));
On 2016/05/04 21:00:28, Randy Smith - Not in Fridays wrote:
> nit, suggestion: If you don't bother with a local variable for this and
instead
> just include the bind in the argument to stale_timer_.Start(), you implicit
> answer the question around why base::Unretained() is safe.  Otherwise, I might
> request a comment :-}.

Done.

[mmenke, 2016-05-11 20:35:59]

On 2016/05/11 20:32:40, Julia Tuttle wrote:
> It is an entirely fair concern that this is unacceptably rushed, and we should
> just do it in TransportConnectJob, but this is also somewhat simpler. (Doing
it
> in TransportConnectJob presents us with other questions, like what to do if we
> act on the stale data but then fresh data arrives while the stale connection
is
> still pending.)
> 
> My plan is that Chrome will never use stale results unverified; it will simply
> start the TCP connection earlier by speculating that the stale data might be
> right, and will return that socket *once the DNS response comes back* if it's
> connected to one of the addresses in the response.

If that's the plan, should we collect histograms on how often the stale result
is still usable, by that metric, before enabling?

I haven't been keeping up, but I share Ryan's concern about the approach of
adding a new host resolver for this.

[Julia Tuttle, 2016-05-20 19:32:20]

Description was changed from

==========
DNS: Create StaleHostResolver.

With ResolveStale{,FromCache} exposed in HostResolver, create a
StaleHostResolver that wraps an existing HostResolver and uses those
methods to return stale results as if they were fresh if a certain
delay has elapsed.

BUG=605138
==========

to

==========
DNS: Create StaleHostResolver.

With ResolveStale{,FromCache} exposed in HostResolver, create a
StaleHostResolver that wraps an existing HostResolver and uses those
methods to return stale results as if they were fresh if a certain
delay has elapsed.

BUG=605138
==========

[Randy Smith (Not in Mondays), 2016-06-08 19:20:03]

High level comments:
* Tests?

* You'll want a cronet reviewer.

* The CL description should at least mention that this is a cronet restricted
feature.

* Can you sketch our for me why four build file changes are needed?  (I get gn
vs. gyp, it's the other 2x I don't get.)

* I'm holding off on reviewing the histograms since there's at least one
suggestions/request below that might result in relevant changes (where the
Request deletions happen).  I don't expect that review will find anything, since
I've done it once before, so hopefully this isn't relevant.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:32: using net::RequestPriority;
https://google.github.io/styleguide/cppguide.html#Namespaces says "Do not use
using-directives" and my biases agree; we in general accept the cost of longer
lines in order to have all the context for references at the point of coding. 
Having said that, I believe the coding guide reference is mostly about
using-directives that import entire namespaces, and I have see using-directives
elsewhere in the code.  So call this a "strong suggestion"; if you feel
strongly, I'll yield (and as always if you provide a good counter-argument I'll
yield) but I'd rather see these all expanded inline.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:76: int ResolveStale(HostResolverImpl*
resolver,
nit, suggestion: Since this is a helper routine used later in the file, I could
imagine it would be useful to include a comment describing it's intended use
(interface contract style, like what we do in class decls).  But it's a pretty
simple function, and I think it's only used in one place nearby so it's easy to
use the code as the reference, so up to you.

ETA: Though thinking about it, a simple function that's only used in one place
might be a candidate for hoisting into the caller?  (Again, up to you.)

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:79: AddressList* addresses,
nitty nit: Slight preference for calling this network_addresses rather than
addresses, just to match code elsewhere in the file.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:159: // Populates |addresses_| from
|addresses| if and only if rv is OK, clears
nit: |*addresses_|.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:199: base::Unretained(this));
semi-nit: Comments above the base::Unretaineds as to why they are safe? 
(Everywhere in the file.)

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:231: delete this;
This is unusual enough behavior that it really should be called out in the
interface contract comment in the declaration.

Also, do we really need this behavior?  I consider it sometimes necessary but
always dangerous (you're generally leaving *something* with a pointer to freed
memory when you do this); could we simply have the destructor cancel if
necessary, and have the owning StaleHostResolver destroy on cancel? 
Alternatively, if you want to keep this logic under a routine named "Cancel()"
can you just hoist the destruction up into the caller?

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:246: // host cache.
Hmm.  To me, this comment implies that we *could* delete this here without ill
effects, but I don't think we can, since the StaleHostResolver still has a
pointer to the object?

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:258: delete this;
Um ... this isn't wrong, but it is scary.  The fact that this doesn't cause any
problems has to do with what callback_ is set to (which is something that will
get rid of the owner's pointer to this object without indirecting through it),
and counting on that specific property of callback_ inside a method of a utility
class seems dangerous.  At a minimum, could you document that requirement on
callback_ in the comment about the Start() declaration?  Preferably (yes :-}),
I'd love for you to hoist the deletion up into StaleHostResolver in some
fashion.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
File components/cronet/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.h:15: // A HostResolver that wraps another
HostResolver that uses it to make requests
nit: We're no longer wrapping an arbitrary HostResolver, so this should probably
refer to HostResolverImpl.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.h:57: // request to continue in order to
repopulate the cache.
I'm a bit torn about all the commenting on the methods in this file.  My ideal
image of how implementing a defined interface works is that you document how the
class handles the interface at the top of the class, and any method specific
comments are in the base class file.  On the other hand, that shouldn't hamper
readability.  OTTH, I'm not sure it does.  What would you think about simply
moving the comments that belong there (see notes later) down into the base
class, and just removing all of the other comments and making sure that the
notes describing the class and on the constructor make clear what's special
about how those methods are handled in this class?  I'm happy to go with your
judgement, but I want to raise the issue so you make that judgement.

Separate but related:  You need a note somewhere in here that the following set
of methods are implementing HostResolver.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.h:61: // |~StaleHostResolver()|).
nit: Looks like line break in the wrong place?

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.h:61: // |~StaleHostResolver()|).
This is actually a comment on the interface, not the implementation, and thus I
think should be in host_resolver.h.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.h:73: // is still running.
This seems like a comment that applies to the interface, not the implementation,
and hence should be in host_resolver.h?

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.h:93: std::set<Request*>
pending_requests_;
These are owned by this class, right?  Would std::set<std::unique_ptr<Request>>
work?  I think C++ 11 has a fair amount of support for movable types in the STL
now.

[Julia Tuttle, 2016-06-10 17:27:15]

Description was changed from

==========
DNS: Create StaleHostResolver.

With ResolveStale{,FromCache} exposed in HostResolver, create a
StaleHostResolver that wraps an existing HostResolver and uses those
methods to return stale results as if they were fresh if a certain
delay has elapsed.

BUG=605138
==========

to

==========
Cronet: Use stale DNS cache entries experimentally.

Create a Cronet-specific HostResolver that uses stale DNS cache entries
when available after a delay, to reduce DNS latency at the expense of
accuracy.

Configure it in place of (actually wrapped around) the default reoslver
when configured by Cronet experimental options.

BUG=605138
==========

[Julia Tuttle, 2016-06-10 17:28:10]

Description was changed from

==========
Cronet: Use stale DNS cache entries experimentally.

Create a Cronet-specific HostResolver that uses stale DNS cache entries
when available after a delay, to reduce DNS latency at the expense of
accuracy.

Configure it in place of (actually wrapped around) the default reoslver
when configured by Cronet experimental options.

BUG=605138
==========

to

==========
Cronet: Use stale DNS cache entries experimentally.

Create a Cronet-specific HostResolver that uses stale DNS cache entries
when available after a delay, to reduce DNS latency at the expense of
accuracy.

Configure it in place of (actually wrapped around) the default reoslver
when configured by Cronet experimental options.

TODO:
Tests.

BUG=605138
==========

[Julia Tuttle, 2016-06-10 17:31:43]

juliatuttle@chromium.org changed reviewers:
+ xunjieli@chromium.org

[Julia Tuttle, 2016-06-10 17:31:45]

PTAL, rdsmith and xunjieli.

Tests: Still working on it.
cronet reviewer: Added Helen.
CL description: Rewritten entirely for new scope.
Build file changes: cronet itself has gyp and gn, but then there are also
android and ios submodules. Basically, I added stale_host_resolver.* every place
I found url_request_context_config.*, since the latter will depend on the former
after this CL.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:32: using net::RequestPriority;
On 2016/06/08 19:20:03, Randy Smith - Not in Fridays wrote:
> https://google.github.io/styleguide/cppguide.html#Namespaces says "Do not use
> using-directives" and my biases agree; we in general accept the cost of longer
> lines in order to have all the context for references at the point of coding. 
> Having said that, I believe the coding guide reference is mostly about
> using-directives that import entire namespaces, and I have see
using-directives
> elsewhere in the code.  So call this a "strong suggestion"; if you feel
> strongly, I'll yield (and as always if you provide a good counter-argument
I'll
> yield) but I'd rather see these all expanded inline.

Done, but to me, this is morally part of net/, just something that's only used
by cronet and therefore contained in that directory.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:76: int ResolveStale(HostResolverImpl*
resolver,
On 2016/06/08 19:20:03, Randy Smith - Not in Fridays wrote:
> nit, suggestion: Since this is a helper routine used later in the file, I
could
> imagine it would be useful to include a comment describing it's intended use
> (interface contract style, like what we do in class decls).  But it's a pretty
> simple function, and I think it's only used in one place nearby so it's easy
to
> use the code as the reference, so up to you.
> 
> ETA: Though thinking about it, a simple function that's only used in one place
> might be a candidate for hoisting into the caller?  (Again, up to you.)

I had it in the caller, and it felt a little too cluttered? I wanted to make it
clear what I was doing with the fresh and stale results, respectively, as
opposed to the details of how I'm getting them. I'll move it into Request as a
helper method, so it preserves that cleanliness but doesn't clutter the file.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:79: AddressList* addresses,
On 2016/06/08 19:20:02, Randy Smith - Not in Fridays wrote:
> nitty nit: Slight preference for calling this network_addresses rather than
> addresses, just to match code elsewhere in the file.

It's not network addresses if there is a fresh cache entry. Maybe
fresh_addresses?

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:159: // Populates |addresses_| from
|addresses| if and only if rv is OK, clears
On 2016/06/08 19:20:02, Randy Smith - Not in Fridays wrote:
> nit: |*addresses_|.

Done.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:199: base::Unretained(this));
On 2016/06/08 19:20:03, Randy Smith - Not in Fridays wrote:
> semi-nit: Comments above the base::Unretaineds as to why they are safe? 
> (Everywhere in the file.)

Done.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:231: delete this;
On 2016/06/08 19:20:02, Randy Smith - Not in Fridays wrote:
> This is unusual enough behavior that it really should be called out in the
> interface contract comment in the declaration.
> 
> Also, do we really need this behavior?  I consider it sometimes necessary but
> always dangerous (you're generally leaving *something* with a pointer to freed
> memory when you do this); could we simply have the destructor cancel if
> necessary, and have the owning StaleHostResolver destroy on cancel? 
> Alternatively, if you want to keep this logic under a routine named "Cancel()"
> can you just hoist the destruction up into the caller?

See below; right now, the caller is *happily* unaware of Requests that are
lingering to let underlying network requests finish. It would complicate
StaleHostResolver if it had to explicitly manage the lifetimes of its Requests.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:246: // host cache.
On 2016/06/08 19:20:03, Randy Smith - Not in Fridays wrote:
> Hmm.  To me, this comment implies that we *could* delete this here without ill
> effects, but I don't think we can, since the StaleHostResolver still has a
> pointer to the object?

Done.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:258: delete this;
On 2016/06/08 19:20:02, Randy Smith - Not in Fridays wrote:
> Um ... this isn't wrong, but it is scary.  The fact that this doesn't cause
any
> problems has to do with what callback_ is set to (which is something that will
> get rid of the owner's pointer to this object without indirecting through it),
> and counting on that specific property of callback_ inside a method of a
utility
> class seems dangerous.  At a minimum, could you document that requirement on
> callback_ in the comment about the Start() declaration?  Preferably (yes :-}),
> I'd love for you to hoist the deletion up into StaleHostResolver in some
> fashion.

Whoa, hold on, I'm not sure I agree. Indirecting through it is fine; the
callback is run three lines *before* the Request is deleted.

I tried hoisting it up into StaleHostResolver, but then it'd have to do the
juggling of "we have to wait for the network request even if the request returns
stale cached results" instead/as well.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
File components/cronet/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.h:15: // A HostResolver that wraps another
HostResolver that uses it to make requests
On 2016/06/08 19:20:03, Randy Smith - Not in Fridays wrote:
> nit: We're no longer wrapping an arbitrary HostResolver, so this should
probably
> refer to HostResolverImpl.

Done.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.h:57: // request to continue in order to
repopulate the cache.
On 2016/06/08 19:20:03, Randy Smith - Not in Fridays wrote:
> I'm a bit torn about all the commenting on the methods in this file.  My ideal
> image of how implementing a defined interface works is that you document how
the
> class handles the interface at the top of the class, and any method specific
> comments are in the base class file.  On the other hand, that shouldn't hamper
> readability.  OTTH, I'm not sure it does.  What would you think about simply
> moving the comments that belong there (see notes later) down into the base
> class, and just removing all of the other comments and making sure that the
> notes describing the class and on the constructor make clear what's special
> about how those methods are handled in this class?  I'm happy to go with your
> judgement, but I want to raise the issue so you make that judgement.
> 
> Separate but related:  You need a note somewhere in here that the following
set
> of methods are implementing HostResolver.

Done.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.h:61: // |~StaleHostResolver()|).
On 2016/06/08 19:20:03, Randy Smith - Not in Fridays wrote:
> This is actually a comment on the interface, not the implementation, and thus
I
> think should be in host_resolver.h.

Done.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.h:61: // |~StaleHostResolver()|).
On 2016/06/08 19:20:03, Randy Smith - Not in Fridays wrote:
> This is actually a comment on the interface, not the implementation, and thus
I
> think should be in host_resolver.h.

Done.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.h:73: // is still running.
On 2016/06/08 19:20:03, Randy Smith - Not in Fridays wrote:
> This seems like a comment that applies to the interface, not the
implementation,
> and hence should be in host_resolver.h?

Done.

https://codereview.chromium.org/1898873006/diff/220001/components/cronet/stal...
components/cronet/stale_host_resolver.h:93: std::set<Request*>
pending_requests_;
On 2016/06/08 19:20:03, Randy Smith - Not in Fridays wrote:
> These are owned by this class, right?  Would
std::set<std::unique_ptr<Request>>
> work?  I think C++ 11 has a fair amount of support for movable types in the
STL
> now.

It wouldn't be ideal; Requests delete themselves when they finish, so the
creator shouldn't delete them, even (maybe?) at destruction time.

[xunjieli, 2016-06-10 19:19:14]

Can we do the Cronet-specific parts in a separate CL? For Android, we talked
about the need for C++ unit tests and Java unit tests. I would like to see these
tests. The experimental options are fragile, so we need tests in place. For iOS,
you need to check with kapishnikov@ or mef@, who understand iOS much better than
me. I'd suggest starting with Android first, iOS in a separate CL. Additionally,
can we have a short design doc and link it in this CL? I am not looking for a
comprehensive doc, just something that explains what stale DNS resolver does,
and what options there are, and if there are any gotchas to this system. Not
trying to be a difficult reviewer, I just want to make sure I understand
everything before I sign off. A design doc and tests will help me doing so. They
will make everything much easier to maintain down the road too :)

[Julia Tuttle, 2016-06-13 11:57:13]

On 2016/06/10 19:19:14, xunjieli wrote:
> Can we do the Cronet-specific parts in a separate CL? For Android, we talked
> about the need for C++ unit tests and Java unit tests. I would like to see
these
> tests. The experimental options are fragile, so we need tests in place. For
iOS,
> you need to check with kapishnikov@ or mef@, who understand iOS much better
than
> me. I'd suggest starting with Android first, iOS in a separate CL.
Additionally,
> can we have a short design doc and link it in this CL? I am not looking for a
> comprehensive doc, just something that explains what stale DNS resolver does,
> and what options there are, and if there are any gotchas to this system. Not
> trying to be a difficult reviewer, I just want to make sure I understand
> everything before I sign off. A design doc and tests will help me doing so.
They
> will make everything much easier to maintain down the road too :)

At this point, most of this is "Cronet-specific". Would you like me to split it
into "add StaleHostResolver" and "configure StaleHostResolver using experimental
options"?

[xunjieli, 2016-06-13 12:01:13]

On 2016/06/13 11:57:13, Julia Tuttle wrote:
> On 2016/06/10 19:19:14, xunjieli wrote:
> > Can we do the Cronet-specific parts in a separate CL? For Android, we talked
> > about the need for C++ unit tests and Java unit tests. I would like to see
> these
> > tests. The experimental options are fragile, so we need tests in place. For
> iOS,
> > you need to check with kapishnikov@ or mef@, who understand iOS much better
> than
> > me. I'd suggest starting with Android first, iOS in a separate CL.
> Additionally,
> > can we have a short design doc and link it in this CL? I am not looking for
a
> > comprehensive doc, just something that explains what stale DNS resolver
does,
> > and what options there are, and if there are any gotchas to this system. Not
> > trying to be a difficult reviewer, I just want to make sure I understand
> > everything before I sign off. A design doc and tests will help me doing so.
> They
> > will make everything much easier to maintain down the road too :)
> 
> At this point, most of this is "Cronet-specific". Would you like me to split
it
> into "add StaleHostResolver" and "configure StaleHostResolver using
experimental
> options"?

SGTM. The net/dns/host_resolver.h,cc should go into one CL with its own net unit
tests. The other components/cronet/ files should go into another CL.

[Randy Smith (Not in Mondays), 2016-06-13 12:28:34]

On 2016/06/10 17:31:45, Julia Tuttle wrote:

Quick heads up: I will try to respond with my thoughts on your response
today/tomorrow, but I doubt I'll be able to work on this again until after I'm
back from vacation.  So unless I can stamp or conditional stamp you might want
to look around for a substitute reviewer (or just use for both the core stuff
and Cronet Helen).

[xunjieli, 2016-06-13 12:33:21]

On 2016/06/13 12:28:34, Randy Smith - Not in Fridays wrote:
> On 2016/06/10 17:31:45, Julia Tuttle wrote:
> 
> Quick heads up: I will try to respond with my thoughts on your response
> today/tomorrow, but I doubt I'll be able to work on this again until after I'm
> back from vacation.  So unless I can stamp or conditional stamp you might want
> to look around for a substitute reviewer (or just use for both the core stuff
> and Cronet Helen).

I missed the part where StaleHostResolver is moved to components/cronet/. I
think it makes sense for them to be in the same CL. I will take a look.

[xunjieli, 2016-06-13 19:25:04]

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:15: #define STALE_HISTOGRAM_ENUM(name,
value, max) \
I am not sure if we want these defines. Can we inline them?

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:80: // asynchronously, unless the
requets is canceled (via |Cancel()|).
typo in "request"

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:106: static int
ResolveStale(net::HostResolverImpl* resolver,
Need documentation, specifically, what the return value is.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:140: net::HostResolver::RequestHandle
network_handle_;
need documentation on these member variables.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:321: }  // namespace
Can we have the anonymous namespaces at the top of the file with the other
anonymous namespace?

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:326: :
resolver_(std::move(inner_resolver)), options_(stale_options) {
nit: Can we rename |resolver_| to |inner_resolver_|? I think it is slightly
clearer that way.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:347: pending_requests_.insert(request);
If request->Start() finishes synchronously, can we not insert |request| into
|pending_requests| ? That will remove the need to delete the pointer in
Request::Start().

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:398: pending_requests_.erase(request);
Instead of deleting |request| in OnNetworkRequestComplete, we can delete
|request| here.  It's easier to reason that if StaleHostResolver manages the
lifetime of the |request| entirely. 

Maybe also get rid of OnStaleDelayElapsed and OnNetworkRequestComplete, and bind
a bool to OnRequestComplete callback to specify whether the callback is invoked
with stale results.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
File components/cronet/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.h:81: void OnRequestComplete(Request*
request,
Need documentation.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.h:87: std::set<Request*>
pending_requests_;
need documentation.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/url_...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/url_...
components/cronet/url_request_context_config.cc:239: DCHECK(false) << "AsyncDNS
and StaleDNS experiments require NetLog.";
I think we can make this into a CHECK(net_log) instead of a DCHECK.

[Julia Tuttle, 2016-06-14 23:11:33]

PTAL, xunjieli.

(Gonna start working on tests tomorrow.)

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:15: #define STALE_HISTOGRAM_ENUM(name,
value, max) \
On 2016/06/13 19:25:04, xunjieli wrote:
> I am not sure if we want these defines. Can we inline them?

Done.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:80: // asynchronously, unless the
requets is canceled (via |Cancel()|).
On 2016/06/13 19:25:04, xunjieli wrote:
> typo in "request"

Done.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:106: static int
ResolveStale(net::HostResolverImpl* resolver,
On 2016/06/13 19:25:04, xunjieli wrote:
> Need documentation, specifically, what the return value is.

Done.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:140: net::HostResolver::RequestHandle
network_handle_;
On 2016/06/13 19:25:03, xunjieli wrote:
> need documentation on these member variables.

Done.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:321: }  // namespace
On 2016/06/13 19:25:03, xunjieli wrote:
> Can we have the anonymous namespaces at the top of the file with the other
> anonymous namespace?

Done.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:326: :
resolver_(std::move(inner_resolver)), options_(stale_options) {
On 2016/06/13 19:25:04, xunjieli wrote:
> nit: Can we rename |resolver_| to |inner_resolver_|? I think it is slightly
> clearer that way.

Done.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:347: pending_requests_.insert(request);
On 2016/06/13 19:25:04, xunjieli wrote:
> If request->Start() finishes synchronously, can we not insert |request| into
> |pending_requests| ? That will remove the need to delete the pointer in
> Request::Start().

Done.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:398: pending_requests_.erase(request);
On 2016/06/13 19:25:04, xunjieli wrote:
> Instead of deleting |request| in OnNetworkRequestComplete, we can delete
> |request| here.  It's easier to reason that if StaleHostResolver manages the
> lifetime of the |request| entirely. 
> 
> Maybe also get rid of OnStaleDelayElapsed and OnNetworkRequestComplete, and
bind
> a bool to OnRequestComplete callback to specify whether the callback is
invoked
> with stale results. 

Done.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
File components/cronet/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.h:81: void OnRequestComplete(Request*
request,
On 2016/06/13 19:25:04, xunjieli wrote:
> Need documentation.

Done.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/stal...
components/cronet/stale_host_resolver.h:87: std::set<Request*>
pending_requests_;
On 2016/06/13 19:25:04, xunjieli wrote:
> need documentation.

Done.

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/url_...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1898873006/diff/240001/components/cronet/url_...
components/cronet/url_request_context_config.cc:239: DCHECK(false) << "AsyncDNS
and StaleDNS experiments require NetLog.";
On 2016/06/13 19:25:04, xunjieli wrote:
> I think we can make this into a CHECK(net_log) instead of a DCHECK.

Done.

[xunjieli, 2016-06-16 13:14:02]

I am on triage duty on Wednesday and Thursday, so I won't be able to review this
until Friday. I would like a brief design doc and unit tests. Having a design
doc will be helpful in reviewing this CL, and will make life easier for future
maintainers. In this particular case, since Cronet is exposing as an
experimental option, so there will not be any API or JavaDoc. A design doc will
make consumers informed and help them in setting up the options correctly.

[Bryan McQuade, 2016-06-16 16:37:18]

bmcquade@chromium.org changed reviewers:
+ bmcquade@chromium.org

[Bryan McQuade, 2016-06-16 16:37:19]

i was curious to see how this was taking shape so did a quick pass. hope you
don't mind the drive by review. :)

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:154: int stale_error_;
similar - should we set this to a sane default?

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:219: stale_timer_.Start(FROM_HERE,
stale_delay, stale_callback);
what happens if stale_delay is the default value (zero)? should we
DCHECK(!stale_delay.is_zero()); at the beginning of this function?

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
File components/cronet/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
components/cronet/stale_host_resolver.h:36: bool allow_other_network;
for deterministic behavior in cases where the caller doesn't explicitly specify
this value, can we default this to false?

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
components/cronet/stale_host_resolver.h:40: int max_stale_uses;
same - default to zero?

[Julia Tuttle, 2016-06-20 13:49:57]

Made requested changes.

Wrote a design doc:
https://docs.google.com/document/d/1OaaypSZRRrpIMOLHmMqhS6eciLEAEGuEVKZ-lNdng...

Let me know if you'd like me to mirror it to a chromium.org doc; right now it's
google.com-only.

Still trying to figure out a good way to unit-test StaleHostResolver; since it
explicitly takes a HostResolverImpl, I basically need to test both at once, and
I'm not super keen on just mirroring the test fixture from
host_resolver_impl_unittest.cc, since it's a bit big.

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:134: // Populates |*addresses_| from
|addresses| if and only if rv is OK, clears
I simplified a few of these comments to avoid just repeating the code
line-for-line in English.

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:154: int stale_error_;
On 2016/06/16 16:37:19, Bryan McQuade wrote:
> similar - should we set this to a sane default?

Sure, I'll use net::ERR_DNS_CACHE_MISS as a sentinel for it being unset, and
also reset it to that after returning the stale result.

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:219: stale_timer_.Start(FROM_HERE,
stale_delay, stale_callback);
On 2016/06/16 16:37:19, Bryan McQuade wrote:
> what happens if stale_delay is the default value (zero)? should we
> DCHECK(!stale_delay.is_zero()); at the beginning of this function?

Zero should Just Work(TM) -- the timer will post a non-delayed task to run
OnStaleDelayElapsed, which will return the stale results shortly after Start was
called.

I'd like to support this use case, since it's effectively the DNS version of
stale-while-revalidate.

If you want, I can do the extra work to make this return synchronously, but I
don't think it's worth the effort.

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
File components/cronet/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
components/cronet/stale_host_resolver.h:36: bool allow_other_network;
On 2016/06/16 16:37:19, Bryan McQuade wrote:
> for deterministic behavior in cases where the caller doesn't explicitly
specify
> this value, can we default this to false?

Done.

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
components/cronet/stale_host_resolver.h:40: int max_stale_uses;
On 2016/06/16 16:37:19, Bryan McQuade wrote:
> same - default to zero?

Done.

[Bryan McQuade, 2016-06-20 13:51:47]

Thanks! Overall this change looks very good to me, but I'm not familiar enough
with the code to give an overall review, so will defer to someone else.

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/280001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:219: stale_timer_.Start(FROM_HERE,
stale_delay, stale_callback);
On 2016/06/20 at 13:49:57, Julia Tuttle wrote:
> On 2016/06/16 16:37:19, Bryan McQuade wrote:
> > what happens if stale_delay is the default value (zero)? should we
> > DCHECK(!stale_delay.is_zero()); at the beginning of this function?
> 
> Zero should Just Work(TM) -- the timer will post a non-delayed task to run
OnStaleDelayElapsed, which will return the stale results shortly after Start was
called.
> 
> I'd like to support this use case, since it's effectively the DNS version of
stale-while-revalidate.
> 
> If you want, I can do the extra work to make this return synchronously, but I
don't think it's worth the effort.

Sounds fine as is, thanks!

[xunjieli, 2016-06-20 18:50:20]

The design doc looks very good. It's very concise and clear. I'd add a brief
definition on what "network" in "allow_other_network" is. Also it isn't clear if
"other networks" is different from "previous networks". I tried to comment on
the doc, but it seems that I only have view access.

https://codereview.chromium.org/1898873006/diff/300001/components/cronet/stal...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/300001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:284: void
StaleHostResolver::Request::ReturnResult(
Forgot to set |addresses_|?

https://codereview.chromium.org/1898873006/diff/300001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:291: void
StaleHostResolver::Request::Complete() {
Forgot to set |addresses_|?

https://codereview.chromium.org/1898873006/diff/300001/components/cronet/stal...
File components/cronet/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/300001/components/cronet/stal...
components/cronet/stale_host_resolver.h:97: };
DISALLOW_COPY_AND_ASSIGN?

[Ryan Sleevi, 2016-06-21 01:25:53]

https://codereview.chromium.org/1898873006/diff/300001/net/dns/host_resolver.cc
File net/dns/host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/300001/net/dns/host_resolver....
net/dns/host_resolver.cc:132: }
STYLE: Match declaration and definition order (e.g. between .h and .cc files)

https://codereview.chromium.org/1898873006/diff/300001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/300001/net/dns/host_resolver....
net/dns/host_resolver.h:195: NetLog* net_log);
Strictly speaking, this should be below the *Resolver() variants, particularly
because the documentation is out of sync.

[Julia Tuttle, 2016-07-18 18:43:32]

https://codereview.chromium.org/1898873006/diff/300001/components/cronet/stal...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/300001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:284: void
StaleHostResolver::Request::ReturnResult(
On 2016/06/20 18:50:20, xunjieli wrote:
> Forgot to set |addresses_|?

Oops. This is why I need to write unittests.

https://codereview.chromium.org/1898873006/diff/300001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:291: void
StaleHostResolver::Request::Complete() {
On 2016/06/20 18:50:20, xunjieli wrote:
> Forgot to set |addresses_|?

|Complete()| isn't supposed to set |addresses_| -- it's called even if the
actual request is canceled, since the Request object is now no longer needed.

https://codereview.chromium.org/1898873006/diff/300001/components/cronet/stal...
File components/cronet/stale_host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/300001/components/cronet/stal...
components/cronet/stale_host_resolver.h:97: };
On 2016/06/20 18:50:20, xunjieli wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/1898873006/diff/300001/net/dns/host_resolver.cc
File net/dns/host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/300001/net/dns/host_resolver....
net/dns/host_resolver.cc:132: }
On 2016/06/21 01:25:53, Ryan Sleevi wrote:
> STYLE: Match declaration and definition order (e.g. between .h and .cc files)

Done.

https://codereview.chromium.org/1898873006/diff/300001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/300001/net/dns/host_resolver....
net/dns/host_resolver.h:195: NetLog* net_log);
On 2016/06/21 01:25:53, Ryan Sleevi wrote:
> Strictly speaking, this should be below the *Resolver() variants, particularly
> because the documentation is out of sync.

Done.

[Julia Tuttle, 2016-07-22 18:34:42]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 18:35:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Julia Tuttle, 2016-07-22 18:56:14]

PTAL, xunjieli.

[commit-bot: I haz the power, 2016-07-22 19:49:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 19:49:42]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-07-25 17:18:15]

Looks great! I am ready to sign off after these few comments. Could you also
open up the design doc and link it in the CL description? I don't see anything
confidential there, so I think we can do a public doc.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:184:
resolver_->CancelRequest(network_handle_);
Out of paranoia, could you also do "network_handle_ = nullptr;" here?

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:352: Request* request = new
Request(inner_resolver_.get());
If |request| completes synchronously, will we leak |request|? I don't see the
place it is cleaned up in that sync case.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
File components/cronet/stale_host_resolver_unittest.cc (right):

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:131: void PopulateCache(int
age_sec) {
Could you add a documentation to this method? 
It isn't as generic as it sounds -- it populates the hostcache with a particular
key.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:159: EXPECT_TRUE(entry !=
nullptr);
EXPECT_NE?

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:194: resolve_closure_ =
run_loop.QuitWhenIdleClosure();
Can we use TestCompletionCallback and just wait for OnResolveComplete to be
invoked? Why do we also do this QuitWhenIdleClosure() here in WaitForResolve()?

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:235: base::MessageLoopForIO
message_loop_for_io_;
Could you add a comment to why |message_loop_for_io_| is needed here?

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:236:
scoped_refptr<MockHostResolverProc> mock_proc_;
Include base/memory/ref_counted.h ?

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:240:
std::unique_ptr<StaleHostResolver> stale_resolver_;
A lot of these member variables should be private and not protected.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:252:
TEST_F(StaleHostResolverTest, Null) {}
What dos this test do? It creates the test fixture and tears it down? It seems
that it doesn't do anything meaningful. Could you add a comment?

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:255: CreateResolver();
This test create the resolver, but it doesn't do anything with it. Why do we
need this test case?

https://codereview.chromium.org/1898873006/diff/340001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/340001/net/dns/host_resolver....
net/dns/host_resolver.h:198: static std::unique_ptr<HostResolverImpl>
CreateSystemResolverImpl(
Is this preferred over doing a runtime cast of HostResolver to HostResolverImpl?

[Julia Tuttle, 2016-07-25 19:11:48]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 19:12:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Julia Tuttle, 2016-07-25 19:27:00]

Description was changed from

==========
Cronet: Use stale DNS cache entries experimentally.

Create a Cronet-specific HostResolver that uses stale DNS cache entries
when available after a delay, to reduce DNS latency at the expense of
accuracy.

Configure it in place of (actually wrapped around) the default reoslver
when configured by Cronet experimental options.

TODO:
Tests.

BUG=605138
==========

to

==========
Cronet: Use stale DNS cache entries experimentally.

Create a Cronet-specific HostResolver that uses stale DNS cache entries
when available after a delay, to reduce DNS latency at the expense of
accuracy.

Configure it in place of (actually wrapped around) the default resolver
when configured by Cronet experimental options.

Design doc:
https://docs.google.com/document/d/1qmxDNfNUIK2cQQi9u_-Qv3PKxEbEtBKhEvIOza2s7...

BUG=605138
==========

[Julia Tuttle, 2016-07-25 19:29:16]

PTAL, xunjieli.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:184:
resolver_->CancelRequest(network_handle_);
On 2016/07/25 17:18:14, xunjieli wrote:
> Out of paranoia, could you also do "network_handle_ = nullptr;" here?

Done.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:352: Request* request = new
Request(inner_resolver_.get());
On 2016/07/25 17:18:14, xunjieli wrote:
> If |request| completes synchronously, will we leak |request|? I don't see the
> place it is cleaned up in that sync case.

Done.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
File components/cronet/stale_host_resolver_unittest.cc (right):

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:131: void PopulateCache(int
age_sec) {
On 2016/07/25 17:18:15, xunjieli wrote:
> Could you add a documentation to this method? 
> It isn't as generic as it sounds -- it populates the hostcache with a
particular
> key.

Done.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:159: EXPECT_TRUE(entry !=
nullptr);
On 2016/07/25 17:18:15, xunjieli wrote:
> EXPECT_NE?

EXPECT_TRUE(entry)?

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:194: resolve_closure_ =
run_loop.QuitWhenIdleClosure();
On 2016/07/25 17:18:15, xunjieli wrote:
> Can we use TestCompletionCallback and just wait for OnResolveComplete to be
> invoked? Why do we also do this QuitWhenIdleClosure() here in
WaitForResolve()?

TestCompletionCallback won't give me the timeout, which I want.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:235: base::MessageLoopForIO
message_loop_for_io_;
On 2016/07/25 17:18:14, xunjieli wrote:
> Could you add a comment to why |message_loop_for_io_| is needed here?

Done.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:236:
scoped_refptr<MockHostResolverProc> mock_proc_;
On 2016/07/25 17:18:14, xunjieli wrote:
> Include base/memory/ref_counted.h ?

Done.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:240:
std::unique_ptr<StaleHostResolver> stale_resolver_;
On 2016/07/25 17:18:15, xunjieli wrote:
> A lot of these member variables should be private and not protected.

Done.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:252:
TEST_F(StaleHostResolverTest, Null) {}
On 2016/07/25 17:18:15, xunjieli wrote:
> What dos this test do? It creates the test fixture and tears it down? It seems
> that it doesn't do anything meaningful. Could you add a comment?

This and the next case are just here to make it easier to tell whether a crash
is in the test harness setup and teardown, or the resolver setup and teardown,
or the request itself. I'll add comments to make it clear I'm testing each of
those parts.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:255: CreateResolver();
On 2016/07/25 17:18:15, xunjieli wrote:
> This test create the resolver, but it doesn't do anything with it. Why do we
> need this test case?

See above.

https://codereview.chromium.org/1898873006/diff/340001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/340001/net/dns/host_resolver....
net/dns/host_resolver.h:198: static std::unique_ptr<HostResolverImpl>
CreateSystemResolverImpl(
On 2016/07/25 17:18:15, xunjieli wrote:
> Is this preferred over doing a runtime cast of HostResolver to
HostResolverImpl?

They're both ugly; I think this is slightly less ugly, because it means someone
who wants to change what CreateSystemResolver returns will have to think about
it instead of just breaking things.

[commit-bot: I haz the power, 2016-07-25 20:18:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-25 20:18:42]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-07-25 21:09:15]

Unfortunately, the unit tests won't be run on any CQ bots. Could you kick off a
run on android_cronet_tester trybot? That will let you know if your tests will
pass on Cronet bots once your CL lands.

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
File components/cronet/stale_host_resolver_unittest.cc (right):

https://codereview.chromium.org/1898873006/diff/340001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:194: resolve_closure_ =
run_loop.QuitWhenIdleClosure();
On 2016/07/25 19:29:16, Julia Tuttle wrote:
> On 2016/07/25 17:18:15, xunjieli wrote:
> > Can we use TestCompletionCallback and just wait for OnResolveComplete to be
> > invoked? Why do we also do this QuitWhenIdleClosure() here in
> WaitForResolve()?
> 
> TestCompletionCallback won't give me the timeout, which I want.

I am probably missing something. What is the significance of kWaitTimeoutSec =
1s here? Why do we timeout at 1second?

I think this is probably tied to each test case. Could you make the timeout as
an argument of WaitForResolve(), so it is clearer in the call places on why that
particular timeout is used?

https://codereview.chromium.org/1898873006/diff/340001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/1898873006/diff/340001/net/dns/host_resolver....
net/dns/host_resolver.h:198: static std::unique_ptr<HostResolverImpl>
CreateSystemResolverImpl(
On 2016/07/25 19:29:16, Julia Tuttle wrote:
> On 2016/07/25 17:18:15, xunjieli wrote:
> > Is this preferred over doing a runtime cast of HostResolver to
> HostResolverImpl?
> 
> They're both ugly; I think this is slightly less ugly, because it means
someone
> who wants to change what CreateSystemResolver returns will have to think about
> it instead of just breaking things.

Acknowledged.

https://codereview.chromium.org/1898873006/diff/360001/components/cronet/stal...
File components/cronet/stale_host_resolver_unittest.cc (right):

https://codereview.chromium.org/1898873006/diff/360001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:261: private:
duplicated "private" keyword

https://codereview.chromium.org/1898873006/diff/360001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:495: // N.B. Experimental
config above sets 0ms stale delay.
mind expanding what "N.B."? I am not sure what that means.

https://codereview.chromium.org/1898873006/diff/360001/components/cronet/url_...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1898873006/diff/360001/components/cronet/url_...
components/cronet/url_request_context_config.cc:74: const char
kStaleDnsMaxStaleUses[] = "max_stale_uses";
Can you add a comment here to document that if this number is 0 that means there
is no limit?

[Julia Tuttle, 2016-08-02 16:04:22]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 16:04:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 16:15:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 16:15:24]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Julia Tuttle, 2016-08-02 16:26:08]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 16:26:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 16:37:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 16:37:35]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Julia Tuttle, 2016-08-02 16:48:19]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 16:48:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-02 17:52:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 17:52:21]

Dry run: This issue passed the CQ dry run.

[Julia Tuttle, 2016-08-03 13:34:24]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 13:34:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Julia Tuttle, 2016-08-03 13:45:29]

PTAL, xunjieli.

Made the changes and ran the trybot you suggested; also, clarified the test
timeout.

https://codereview.chromium.org/1898873006/diff/360001/components/cronet/stal...
File components/cronet/stale_host_resolver_unittest.cc (right):

https://codereview.chromium.org/1898873006/diff/360001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:261: private:
On 2016/07/25 21:09:14, xunjieli wrote:
> duplicated "private" keyword

Done.

https://codereview.chromium.org/1898873006/diff/360001/components/cronet/stal...
components/cronet/stale_host_resolver_unittest.cc:495: // N.B. Experimental
config above sets 0ms stale delay.
On 2016/07/25 21:09:14, xunjieli wrote:
> mind expanding what "N.B."? I am not sure what that means.

I changed it to "Note:". (It's an abbreviation of the Latin "nota bene", which
means "note well".)

https://codereview.chromium.org/1898873006/diff/360001/components/cronet/url_...
File components/cronet/url_request_context_config.cc (right):

https://codereview.chromium.org/1898873006/diff/360001/components/cronet/url_...
components/cronet/url_request_context_config.cc:74: const char
kStaleDnsMaxStaleUses[] = "max_stale_uses";
On 2016/07/25 21:09:15, xunjieli wrote:
> Can you add a comment here to document that if this number is 0 that means
there
> is no limit?

Done.

[commit-bot: I haz the power, 2016-08-03 14:44:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-03 14:44:53]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xunjieli, 2016-08-03 16:03:40]

lgtm

[Julia Tuttle, 2016-08-03 17:21:55]

The CQ bit was checked by juliatuttle@chromium.org

[commit-bot: I haz the power, 2016-08-03 17:22:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 17:29:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-03 17:29:51]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Julia Tuttle, 2016-08-03 17:32:22]

PTAL at histograms.xml, asvitkine.

[Bryan McQuade, 2016-08-03 20:01:32]

bmcquade@chromium.org changed reviewers:
- bmcquade@chromium.org

[Julia Tuttle, 2016-08-08 18:41:26]

juliatuttle@chromium.org changed reviewers:
+ mpearson@chromium.org
- asvitkine@chromium.org

[Julia Tuttle, 2016-08-08 18:41:27]

[-asvitkine,+mpearson]

PTAL at histograms.xml, mpearson. Thanks!

[Mark P, 2016-08-08 19:16:21]

https://codereview.chromium.org/1898873006/diff/440001/components/cronet/stal...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/440001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:59:
UMA_HISTOGRAM_MEDIUM_TIMES("DNS.StaleHostResolver.NetworkLate",
Is medium times the right macro here?  Typical range is 10ms - 3 minutes?  Not
knowing anything about when this logic triggers, I'd naively think you'd want
more precision at the lower end (down to 1ms) and not need as long a higher
bound (more on the order of seconds, not minutes).

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:9905: +    was available, and the
network responded before the stale delay, how much
is it possible that the network responded at the same time?  In other words, is
there a hole into which some replies can fall that are neither the earlier nor
the later bucket?

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:9927: +
I don't see anything about errors here.  Is there basically no possibility of an
error in fetching a stale result?

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:71980: +  <int value="5"
label="Canceled; stale result available."/>
I'm confused.  If the request was canceled, how would you know whether a stale
result was available, i.e., distinguish between 4 and 5?

[Julia Tuttle, 2016-08-09 12:45:19]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 12:45:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Julia Tuttle, 2016-08-09 12:50:32]

PTAL, mpearson.

https://codereview.chromium.org/1898873006/diff/440001/components/cronet/stal...
File components/cronet/stale_host_resolver.cc (right):

https://codereview.chromium.org/1898873006/diff/440001/components/cronet/stal...
components/cronet/stale_host_resolver.cc:59:
UMA_HISTOGRAM_MEDIUM_TIMES("DNS.StaleHostResolver.NetworkLate",
On 2016/08/08 19:16:21, Mark P wrote:
> Is medium times the right macro here?  Typical range is 10ms - 3 minutes?  Not
> knowing anything about when this logic triggers, I'd naively think you'd want
> more precision at the lower end (down to 1ms) and not need as long a higher
> bound (more on the order of seconds, not minutes).

Good call. I'll use UMA_HISTOGRAM_LONG_TIMES_100 instead. It's a bit overkill,
but it's the same range that the main DNS code uses.

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:9905: +    was available, and the
network responded before the stale delay, how much
On 2016/08/08 19:16:21, Mark P wrote:
> is it possible that the network responded at the same time?  In other words,
is
> there a hole into which some replies can fall that are neither the earlier nor
> the later bucket?

Yes; I've ensured that's included in NetworkEarly, and tweaked the text to note.

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:9927: +
On 2016/08/08 19:16:21, Mark P wrote:
> I don't see anything about errors here.  Is there basically no possibility of
an
> error in fetching a stale result?

There's never the possibility of getting an error *fetching* it -- it's just
from an in-memory cache.

There *is* the possibility that the stale cached data *was* an error result
(although I think we store those with 0 TTL), or that the network result
eventually resulted in an error, but those aren't included in this histogram.

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:71980: +  <int value="5"
label="Canceled; stale result available."/>
On 2016/08/08 19:16:21, Mark P wrote:
> I'm confused.  If the request was canceled, how would you know whether a stale
> result was available, i.e., distinguish between 4 and 5?

The stale results come from the cache, not from the network. I've clarified by
saying "stale cached result".

[commit-bot: I haz the power, 2016-08-09 14:41:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 14:41:27]

Dry run: This issue passed the CQ dry run.

[Mark P, 2016-08-11 22:58:22]

sorry for the delay

--mark

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:9927: +
On 2016/08/09 12:50:32, Julia Tuttle wrote:
> On 2016/08/08 19:16:21, Mark P wrote:
> > I don't see anything about errors here.  Is there basically no possibility
of
> an
> > error in fetching a stale result?
> 
> There's never the possibility of getting an error *fetching* it -- it's just
> from an in-memory cache.
> 
> There *is* the possibility that the stale cached data *was* an error result
> (although I think we store those with 0 TTL), or that the network result
> eventually resulted in an error, but those aren't included in this histogram.

Considering this, perhaps you want to add the word "successfully" to the above
histogram descriptions, e.g., "network successfully responded"?

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:71980: +  <int value="5"
label="Canceled; stale result available."/>
On 2016/08/09 12:50:32, Julia Tuttle wrote:
> On 2016/08/08 19:16:21, Mark P wrote:
> > I'm confused.  If the request was canceled, how would you know whether a
stale
> > result was available, i.e., distinguish between 4 and 5?
> 
> The stale results come from the cache, not from the network. I've clarified by
> saying "stale cached result".

Your clarified is nice for those histograms.  However, I was asking about the
DNS.StaleHostResolver.RequestOutcome histogram and this associated enum, neither
of which you revised/clarified.

[Julia Tuttle, 2016-08-16 13:44:22]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 13:44:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Julia Tuttle, 2016-08-16 13:53:38]

PTAL, mpearson.

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:9927: +
On 2016/08/11 22:58:22, Mark P wrote:
> On 2016/08/09 12:50:32, Julia Tuttle wrote:
> > On 2016/08/08 19:16:21, Mark P wrote:
> > > I don't see anything about errors here.  Is there basically no possibility
> of
> > an
> > > error in fetching a stale result?
> > 
> > There's never the possibility of getting an error *fetching* it -- it's just
> > from an in-memory cache.
> > 
> > There *is* the possibility that the stale cached data *was* an error result
> > (although I think we store those with 0 TTL), or that the network result
> > eventually resulted in an error, but those aren't included in this
histogram.
> 
> Considering this, perhaps you want to add the word "successfully" to the above
> histogram descriptions, e.g., "network successfully responded"?

I have "both the stale and network results were successful" already. Is that
okay?

https://codereview.chromium.org/1898873006/diff/440001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:71980: +  <int value="5"
label="Canceled; stale result available."/>
On 2016/08/11 22:58:22, Mark P wrote:
> On 2016/08/09 12:50:32, Julia Tuttle wrote:
> > On 2016/08/08 19:16:21, Mark P wrote:
> > > I'm confused.  If the request was canceled, how would you know whether a
> stale
> > > result was available, i.e., distinguish between 4 and 5?
> > 
> > The stale results come from the cache, not from the network. I've clarified
by
> > saying "stale cached result".
> 
> Your clarified is nice for those histograms.  However, I was asking about the
> DNS.StaleHostResolver.RequestOutcome histogram and this associated enum,
neither
> of which you revised/clarified.

Oops! Clarified these in the same way (noted that "stale" was "cached").

[commit-bot: I haz the power, 2016-08-16 14:40:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 14:40:36]

Dry run: This issue passed the CQ dry run.

[Mark P, 2016-08-16 17:59:03]

histograms.xml lgtm with still minor comments below
(I don't want to slow you down too much with back-and-forth, which is why I'm
approving now.)

--mark

https://codereview.chromium.org/1898873006/diff/480001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1898873006/diff/480001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:10144: +    and both the stale and
network results were successful, the difference
On 2016/08/16 13:53:38, Julia Tuttle wrote:
> On 2016/08/11 22:58:22, Mark P wrote:
> > On 2016/08/09 12:50:32, Julia Tuttle wrote:
> > > On 2016/08/08 19:16:21, Mark P wrote:
> > > > I don't see anything about errors here.  Is there basically no
possibility
> > of
> > > an
> > > > error in fetching a stale result?
> > > 
> > > There's never the possibility of getting an error *fetching* it -- it's
just
> > > from an in-memory cache.
> > > 
> > > There *is* the possibility that the stale cached data *was* an error
result
> > > (although I think we store those with 0 TTL), or that the network result
> > > eventually resulted in an error, but those aren't included in this
> histogram.
> > 
> > Considering this, perhaps you want to add the word "successfully" to the
above
> > histogram descriptions, e.g., "network successfully responded"?
> 
> I have "both the stale and network results were successful" already. Is that
> okay?

That's fine here.  I was suggesting that you revise
DNS.StaleHostResolver.NetworkEarly and DNS.StaleHostResolver.NetworkLate from
"and the network responded {before,after}" to "and the network responded
successfully {before,after}", if indeed you only record those histograms on
successful network responses, not errors.

As for the histogram DNS.StaleHostResolver.RequestOutcome, I assume that you
record this regardless of whether the network response succeeded; please correct
me if I'm wrong.

(This is what I meant  by the "above histograms", i.e., not this histogram; this
histogram is fine.)

[Julia Tuttle, 2016-08-22 15:06:07]

On 2016/08/16 17:59:03, Mark P wrote:
> histograms.xml lgtm with still minor comments below
> (I don't want to slow you down too much with back-and-forth, which is why I'm
> approving now.)
> 
> --mark
> 
>
https://codereview.chromium.org/1898873006/diff/480001/tools/metrics/histogra...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1898873006/diff/480001/tools/metrics/histogra...
> tools/metrics/histograms/histograms.xml:10144: +    and both the stale and
> network results were successful, the difference
> On 2016/08/16 13:53:38, Julia Tuttle wrote:
> > On 2016/08/11 22:58:22, Mark P wrote:
> > > On 2016/08/09 12:50:32, Julia Tuttle wrote:
> > > > On 2016/08/08 19:16:21, Mark P wrote:
> > > > > I don't see anything about errors here.  Is there basically no
> possibility
> > > of
> > > > an
> > > > > error in fetching a stale result?
> > > > 
> > > > There's never the possibility of getting an error *fetching* it -- it's
> just
> > > > from an in-memory cache.
> > > > 
> > > > There *is* the possibility that the stale cached data *was* an error
> result
> > > > (although I think we store those with 0 TTL), or that the network result
> > > > eventually resulted in an error, but those aren't included in this
> > histogram.
> > > 
> > > Considering this, perhaps you want to add the word "successfully" to the
> above
> > > histogram descriptions, e.g., "network successfully responded"?
> > 
> > I have "both the stale and network results were successful" already. Is that
> > okay?
> 
> That's fine here.  I was suggesting that you revise
> DNS.StaleHostResolver.NetworkEarly and DNS.StaleHostResolver.NetworkLate from
> "and the network responded {before,after}" to "and the network responded
> successfully {before,after}", if indeed you only record those histograms on
> successful network responses, not errors.
> 
> As for the histogram DNS.StaleHostResolver.RequestOutcome, I assume that you
> record this regardless of whether the network response succeeded; please
correct
> me if I'm wrong.
> 
> (This is what I meant  by the "above histograms", i.e., not this histogram;
this
> histogram is fine.)

Oh! No, those are recorded whenever the network request completes, whether it
succeeds or fails.

[Julia Tuttle, 2016-08-22 15:06:22]

The CQ bit was checked by juliatuttle@chromium.org

[Julia Tuttle, 2016-08-22 15:06:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/1898873006/#ps480001
(title: "Make requested changes.")

[commit-bot: I haz the power, 2016-08-22 15:06:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 17:57:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 17:57:40]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Julia Tuttle, 2016-08-23 20:02:23]

The CQ bit was checked by juliatuttle@chromium.org

[commit-bot: I haz the power, 2016-08-23 20:02:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 20:06:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 20:06:17]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Julia Tuttle, 2016-08-23 20:46:08]

The CQ bit was checked by juliatuttle@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 20:46:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 22:35:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 22:35:11]

Dry run: This issue passed the CQ dry run.

[Julia Tuttle, 2016-08-23 22:42:45]

The CQ bit was checked by juliatuttle@chromium.org

[Julia Tuttle, 2016-08-23 22:42:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org, xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/1898873006/#ps500001
(title: "Resolve merge conflict.")

[commit-bot: I haz the power, 2016-08-23 22:43:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 22:49:37]

Description was changed from

==========
Cronet: Use stale DNS cache entries experimentally.

Create a Cronet-specific HostResolver that uses stale DNS cache entries
when available after a delay, to reduce DNS latency at the expense of
accuracy.

Configure it in place of (actually wrapped around) the default resolver
when configured by Cronet experimental options.

Design doc:
https://docs.google.com/document/d/1qmxDNfNUIK2cQQi9u_-Qv3PKxEbEtBKhEvIOza2s7...

BUG=605138
==========

to

==========
Cronet: Use stale DNS cache entries experimentally.

Create a Cronet-specific HostResolver that uses stale DNS cache entries
when available after a delay, to reduce DNS latency at the expense of
accuracy.

Configure it in place of (actually wrapped around) the default resolver
when configured by Cronet experimental options.

Design doc:
https://docs.google.com/document/d/1qmxDNfNUIK2cQQi9u_-Qv3PKxEbEtBKhEvIOza2s7...

BUG=605138
==========

[commit-bot: I haz the power, 2016-08-23 22:49:39]

Committed patchset #26 (id:500001)

[commit-bot: I haz the power, 2016-08-23 22:50:49]

Description was changed from

==========
Cronet: Use stale DNS cache entries experimentally.

Create a Cronet-specific HostResolver that uses stale DNS cache entries
when available after a delay, to reduce DNS latency at the expense of
accuracy.

Configure it in place of (actually wrapped around) the default resolver
when configured by Cronet experimental options.

Design doc:
https://docs.google.com/document/d/1qmxDNfNUIK2cQQi9u_-Qv3PKxEbEtBKhEvIOza2s7...

BUG=605138
==========

to

==========
Cronet: Use stale DNS cache entries experimentally.

Create a Cronet-specific HostResolver that uses stale DNS cache entries
when available after a delay, to reduce DNS latency at the expense of
accuracy.

Configure it in place of (actually wrapped around) the default resolver
when configured by Cronet experimental options.

Design doc:
https://docs.google.com/document/d/1qmxDNfNUIK2cQQi9u_-Qv3PKxEbEtBKhEvIOza2s7...

BUG=605138

Committed: https://crrev.com/50d9c4b48edf4cb160b426fea7475e689cd1816b
Cr-Commit-Position: refs/heads/master@{#413875}
==========

[commit-bot: I haz the power, 2016-08-23 22:50:51]

Patchset 26 (id:??) landed as
https://crrev.com/50d9c4b48edf4cb160b426fea7475e689cd1816b
Cr-Commit-Position: refs/heads/master@{#413875}

[rohitrao (ping after 24h), 2016-08-24 12:24:44]

rohitrao@chromium.org changed reviewers:
+ rohitrao@chromium.org

[rohitrao (ping after 24h), 2016-08-24 12:24:46]

It looks like cronet_unittests aren't being compiled on the ios bots?  We're
hitting chromium-style errors on the internal bots that should be caught in the
public tree, but aren't:

../../components/cronet/stale_host_resolver_unittest.cc:61:3: error:
[chromium-style] Classes that are ref-counted should have destructors that are
declared protected or private.
  ~MockHostResolverProc() override {}
  ^
../../components/cronet/stale_host_resolver_unittest.cc:57:30: note:
[chromium-style] 'MockHostResolverProc' inherits from 'net::HostResolverProc'
here
class MockHostResolverProc : public net::HostResolverProc {
                             ^
../../net/dns/host_resolver_proc.h:27:7: note: [chromium-style]
'HostResolverProc' inherits from 'base::RefCountedThreadSafe<HostResolverProc>'
here
    : public base::RefCountedThreadSafe<HostResolverProc> {
      ^
../../components/cronet/stale_host_resolver_unittest.cc:81:28: error:
[chromium-style] Overriding method must be marked with 'override' or 'final'.
  ~StaleHostResolverTest() {}
                           ^
                            override
2 errors generated.

[Xi Han, 2016-08-24 14:09:38]

On 2016/08/24 12:24:46, rohitrao wrote:
> It looks like cronet_unittests aren't being compiled on the ios bots?  We're
> hitting chromium-style errors on the internal bots that should be caught in
the
> public tree, but aren't:
> 
> ../../components/cronet/stale_host_resolver_unittest.cc:61:3: error:
> [chromium-style] Classes that are ref-counted should have destructors that are
> declared protected or private.
>   ~MockHostResolverProc() override {}
>   ^
> ../../components/cronet/stale_host_resolver_unittest.cc:57:30: note:
> [chromium-style] 'MockHostResolverProc' inherits from 'net::HostResolverProc'
> here
> class MockHostResolverProc : public net::HostResolverProc {
>                              ^
> ../../net/dns/host_resolver_proc.h:27:7: note: [chromium-style]
> 'HostResolverProc' inherits from
'base::RefCountedThreadSafe<HostResolverProc>'
> here
>     : public base::RefCountedThreadSafe<HostResolverProc> {
>       ^
> ../../components/cronet/stale_host_resolver_unittest.cc:81:28: error:
> [chromium-style] Overriding method must be marked with 'override' or 'final'.
>   ~StaleHostResolverTest() {}
>                            ^
>                             override
> 2 errors generated.

The same compile error occurs in android bot too
(https://uberchromegw.corp.google.com/i/internal.client.clank/builders/clang-b...)

[agrieve, 2016-08-24 16:17:46]

On 2016/08/24 14:09:38, Xi Han wrote:
> On 2016/08/24 12:24:46, rohitrao wrote:
> > It looks like cronet_unittests aren't being compiled on the ios bots?  We're
> > hitting chromium-style errors on the internal bots that should be caught in
> the
> > public tree, but aren't:
> > 
> > ../../components/cronet/stale_host_resolver_unittest.cc:61:3: error:
> > [chromium-style] Classes that are ref-counted should have destructors that
are
> > declared protected or private.
> >   ~MockHostResolverProc() override {}
> >   ^
> > ../../components/cronet/stale_host_resolver_unittest.cc:57:30: note:
> > [chromium-style] 'MockHostResolverProc' inherits from
'net::HostResolverProc'
> > here
> > class MockHostResolverProc : public net::HostResolverProc {
> >                              ^
> > ../../net/dns/host_resolver_proc.h:27:7: note: [chromium-style]
> > 'HostResolverProc' inherits from
> 'base::RefCountedThreadSafe<HostResolverProc>'
> > here
> >     : public base::RefCountedThreadSafe<HostResolverProc> {
> >       ^
> > ../../components/cronet/stale_host_resolver_unittest.cc:81:28: error:
> > [chromium-style] Overriding method must be marked with 'override' or
'final'.
> >   ~StaleHostResolverTest() {}
> >                            ^
> >                             override
> > 2 errors generated.
> 
> The same compile error occurs in android bot too
>
(https://uberchromegw.corp.google.com/i/internal.client.clank/builders/clang-b...)

FYI - I believe the reason this wasn't caught by
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...
is that cronet doesn't build with is_component_build. It's actually a
misconfiguration (I believe) that the downstream clang bot is built in
non-component mode.

[Julia Tuttle, 2016-08-25 13:20:50]

These should be fixed by:

https://codereview.chromium.org/2277453003/ (by bauerb) and 
https://codereview.chromium.org/2274123002/ (by me)

Thanks to everyone who pointed it out.