[net/dns] Suppress multiple DnsConfig withdrawals after timeout.

[net/dns] Suppress multiple DnsConfig withdrawals after timeout.

BUG=132110
TEST=net_unittests --gtest_filter=DnsConfigServiceTest.Timeout


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

[szym, 2012-06-11 19:47:50]

Introduces |last_sent_empty_| to complement |need_update_|. Both variables could
also be replaced by |DnsConfig last_sent_config_|. However, since DnsHosts could
potentially take up significant memory, it seems a good idea to avoid keeping
the extra copy in memory.

[mmenke, 2012-06-11 22:46:25]

LGTM

http://codereview.chromium.org/10537106/diff/1/net/dns/dns_config_service.cc
File net/dns/dns_config_service.cc (right):

http://codereview.chromium.org/10537106/diff/1/net/dns/dns_config_service.cc#...
net/dns/dns_config_service.cc:151: return;  // No need to withdraw again.
optional:  May also want to DHCECK that the timer isn't running before
returning.

[mmenke, 2012-06-11 22:48:25]

Is this expected to fix the entire problem?  We only get a a bunch of redundant
events on Mac when we don't have a valid config?  Or is this just aimed at
fixing half the problem?

[szym, 2012-06-12 18:08:39]

On 2012/06/11 22:48:25, Matt Menke wrote:
> Is this expected to fix the entire problem?  We only get a a bunch of
redundant
> events on Mac when we don't have a valid config?  Or is this just aimed at
> fixing half the problem?

If we already have config then the inequality checks ensure that we don't send
an update that does not change anything. This fix is for the half of the problem
which was not yet addressed: if we have withdrawn config, we should not withdraw
it again.

Now, there's still a potential issue: if reading HOSTS takes > 100ms (unlikely)
and we receive spurious signals from FilePathWatcher (not unlikely), then we
will withdraw the config just to add it again (which will abort HostResolverImpl
Jobs twice). I'm planning to solve this by adding a stat-check and avoid reading
HOSTS if no attributes have changed. I plan to add this to DnsHostsReader which
is shared across platforms.

[commit-bot: I haz the power, 2012-06-12 18:09:24]

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

[mmenke, 2012-06-12 18:21:01]

On 2012/06/12 18:08:39, szym wrote:
> On 2012/06/11 22:48:25, Matt Menke wrote:
> > Is this expected to fix the entire problem?  We only get a a bunch of
> redundant
> > events on Mac when we don't have a valid config?  Or is this just aimed at
> > fixing half the problem?
> 
> If we already have config then the inequality checks ensure that we don't send
> an update that does not change anything. This fix is for the half of the
problem
> which was not yet addressed: if we have withdrawn config, we should not
withdraw
> it again.
> 
> Now, there's still a potential issue: if reading HOSTS takes > 100ms
(unlikely)
> and we receive spurious signals from FilePathWatcher (not unlikely), then we
> will withdraw the config just to add it again (which will abort
HostResolverImpl
> Jobs twice). I'm planning to solve this by adding a stat-check and avoid
reading
> HOSTS if no attributes have changed. I plan to add this to DnsHostsReader
which
> is shared across platforms.

How often do we get spurious changes?  If it's on the order of seconds, it still
seems like a lot of extra work.  If it's on the order of minutes, I'm not too
concerned.

[szym, 2012-06-12 18:26:22]

On 2012/06/12 18:21:01, Matt Menke wrote:
> How often do we get spurious changes?  If it's on the order of seconds, it
still
> seems like a lot of extra work.  If it's on the order of minutes, I'm not too
> concerned.

http://crbug.com/130610 tracks the issue on OSX. I observed that there are
periods when it signals every 5-15 seconds. I'm not concerned about the work,
but about the aborted HostResolver Requests which do not automatically get
reattempted.

[mmenke, 2012-06-12 18:40:04]

On 2012/06/12 18:26:22, szym wrote:
> On 2012/06/12 18:21:01, Matt Menke wrote:
> > How often do we get spurious changes?  If it's on the order of seconds, it
> still
> > seems like a lot of extra work.  If it's on the order of minutes, I'm not
too
> > concerned.
> 
> http://crbug.com/130610 tracks the issue on OSX. I observed that there are
> periods when it signals every 5-15 seconds. I'm not concerned about the work,
> but about the aborted HostResolver Requests which do not automatically get
> reattempted.

The extra events are obviously the bigger issue, but a lot of closely spaced
disk reads can also cause jank.  Also a tiny chance of not getting the new
configuration before we clear the old one on each of the attempts.

[commit-bot: I haz the power, 2012-06-12 19:50:38]

Change committed as 141715