[net/dns] Perform A/AAAA queries for AF_UNSPEC resolutions in parallel.

[net/dns] Perform A/AAAA queries for AF_UNSPEC resolutions in parallel. 

The second DnsTransaction is scheduled as a second job on the resolver's
PrioritizedDispatcher, at the beginning of its priority queue.  The two
DnsTransactions run independently of each other, although if one of them
finishes with an error, the other one will be scrapped immediately, and
the Job will fall back to ProcTask. 

BUG=174992
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218616

[mmenke, 2013-07-24 18:59:45]

WDYT?  This is "inspired" by your CL, though I do some things a little
differently.

No rush to review this, as nothing really depends on it.

One source of weirdness is that reprioritizing a request with one job running is
a little weird.  Since we'll start the second request when the first request
completes, I think the way we do this falls under "good enough".

This could pretty easily be modified to always take a second slot in the
dispatcher when the DNS task starts, regardless of whether there's an available
slot or now.  That would be a bit simpler, but may not lend itself as well to
rate limiting DNS transactions at the dispatcher layer (We could also rate limit
them at a lower point, of course.  Regarldess of what we try, getting a method
that rate limits both DnsTransactions and ProcTasks is a bit tricky).

https://codereview.chromium.org/19498003/diff/122001/net/dns/host_resolver_im...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/19498003/diff/122001/net/dns/host_resolver_im...
net/dns/host_resolver_impl.cc:1250: handle =
resolver_->dispatcher_.AddAtHead(this, priority());
There doesn't seem to be a reasonable way to test this, since when the first job
completes, we start the job immediately, without waiting on the queue.

[szym, 2013-08-06 20:35:31]

Thanks for picking up the slack and sorry for taking forever to review this.

Overall LG, but I had some tough time with FinishOrCancelSecondDispatcherJob.

https://codereview.chromium.org/19498003/diff/33010/net/base/prioritized_disp...
File net/base/prioritized_dispatcher.h (right):

https://codereview.chromium.org/19498003/diff/33010/net/base/prioritized_disp...
net/base/prioritized_dispatcher.h:81: Handle AddAtHead(Job* job, Priority
priority);
Needs a short comment.

https://codereview.chromium.org/19498003/diff/33010/net/dns/dns_test_util.cc
File net/dns/dns_test_util.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/dns_test_util.cc#...
net/dns/dns_test_util.cc:64: if (!delayed_) {
nit:
if (delayed_) return;

https://codereview.chromium.org/19498003/diff/33010/net/dns/dns_test_util.cc#...
net/dns/dns_test_util.cc:151: class MockAddressSorter : public AddressSorter {
It's strange to see it between MockTransaction and MockTransacitonFactory.
Suggest either leaving it where it is or moving it to the top of the file.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1096: if (needs_two_transactions() &&
num_completed_transactions_ == 1) {
At this point we could also set key_.hostname = transaction->GetHostname() so
that if the second transaction is still queued, it does not need to perform
suffix search.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1165: // Number of transactions that have started,
but not yet completed.
The name of the variable is in direct conflict with the comment above it. I
don't think this really needs a comment.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1219: KillDnsTask();
I suggest leave it as is. Job is destroyed while running only if the whole
resolver is destroyed as well, so all we want to do is log.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1254: // and the second attempt does not complete
synchronously.
"attempt" is ambiguous. Suggest:
"The dispatcher could have started |this| in Add, which could have called
Schedule again. In that case |handle| will be null, but |handle_| will already
have been set by the other Schedule."

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1386: void FinishOrCancelSecondDispatcherJob() {
I've spent a lot of time trying to make it more obvious what this function does.
The key problem seems to be that it is supposed to solve three separate issues,
but the code ends up the same in both cases.

Case 1: The Job ends while running, so clean it up.
 Subcase 1: occupying two job slots. Signal OnJobFinished for the extra slot.
 Subcase 2: queued for another job slot. Cancel the queue handle.
OnJobFinished for the remaining slot is called in CompleteRequests.

Case 2: Work in the first job slot has finished. Leverage it for the other job
slot.
 Subcase 1: other job slot already running. Signal OnJobFinished for the
completed slot.
 Subcase 2: other job still in queue. Cancel its queue handle and take over the
job slot we already have.

Case 3: DnsTask failed. Fallback to ProcTask.
 Similar subcases to case 1, except that ProcTask takes over the remaining slot.


How about:
// Reduce the number of job slots occupied and queued in the dispatcher
// to one. If there is work still queued for the second job slot, let it
// take over this one.
ReduceToOneJobSlot() {
  DCHECK(num_occupied_job_slots_ >= 1);
  if (is_queued()) {
    dispatcher_.Cancel(handle_);
    handle_.Reset();
  } else if (num_occupied_job_slots_ > 1) {
    dispatcher_.OnJobFinished();
    --num_occupied_job_slots_;
  }
  DCHECK(num_occupied_job_slots_ == 1);
}

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1531: // Schedule a second transaciton, if needed.
nit: transaction

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1611: // If the second job is still queued, start
it now.
"Second job" is a bit confusing. Suggest further explanation:
We already have a job slot at the dispatcher, so reuse it now instead of waiting
in the queue for the second slot.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1614: FinishOrCancelSecondDispatcherJob();
If we already started both jobs, then we are actually finishing the _first_
slot. Renaming the function will help.

We StartSecondTransaction before canceling the second dispatcher job from the
queue. I understand this is because after canceling, we can't check is_queued(),
but it's counter-intuitive.

It'd be more obvious if it was:

DCHECK_EQ(dns_task_->needs_another_transaction(), is_queued());
ReduceToOneJobSlot();
if (dns_task_->needs_another_transaction())
  dns_task_->StartSecondTransaction();

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_impl.h
File net/dns/host_resolver_impl.h (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.h:227: // HostResolverImpl::Job moccupy multiple
PrioritizedDispatcher job slots.
moccupy => could occupy
Probably OK to skip HostResolverImpl::

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1327: MockDnsClient* dns_client_;
  // Owned by |resolver_|.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1675: TEST_F(HostResolverImplDnsTest,
AsyncResolverCancelWithOneTransactionActive) {
No need for AsyncResolver prefix to these tests. All HostResolverImplDnsTest
exercise DnsTask.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1676: set_fallback_to_proctask(false);
I don't think these tests need set_fallback_to_proctask(false);

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1682: requests_[0]->Cancel();
When the test depends on TearDown, suggest comment at end of test:
// Check result in TearDown.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1740: 
Add a test which ends in resolver_.Reset() without canceling the requests.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1743: set_fallback_to_proctask(false);
OK. These three last tests do need fallback disabled.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1806: EXPECT_EQ(ERR_IO_PENDING,
CreateRequest("ok", 80, MEDIUM,
MEDIUM is the default priority in CreateRequest.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1808: EXPECT_EQ(ERR_IO_PENDING,
CreateRequest("4slow_ok", 80)->Resolve());
Maybe make the priority explicit here.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1809: // An IPv4 request should have been
started pending for each job.
Actually, the first Resolve() call will cause the first Job to launch both of
its transactions. The two job slots for the second Resolve() will be queued
despite its higher priority.

To exercise the intended scenario use CreateSerialResolver() here and just
interleave them as follows:

req0->Resolve at MEDIUM, it takes the job slot, queues another at MEDIUM
req1->Resolve at HIGH, it queues two at HIGH

RunUntilIdle
req0's first job completes, req1's first job is started
req1's first job completes, req1's second job is started
req1's first job completes, req0's second job is started
req0's second job completes

Up to you if you want to sprinkle in some delayed transactions.

[mmenke, 2013-08-19 17:31:11]

https://codereview.chromium.org/19498003/diff/33010/net/base/prioritized_disp...
File net/base/prioritized_dispatcher.h (right):

https://codereview.chromium.org/19498003/diff/33010/net/base/prioritized_disp...
net/base/prioritized_dispatcher.h:81: Handle AddAtHead(Job* job, Priority
priority);
On 2013/08/06 20:35:31, szym wrote:
> Needs a short comment.

Done.

https://codereview.chromium.org/19498003/diff/33010/net/dns/dns_test_util.cc
File net/dns/dns_test_util.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/dns_test_util.cc#...
net/dns/dns_test_util.cc:64: if (!delayed_) {
On 2013/08/06 20:35:31, szym wrote:
> nit:
> if (delayed_) return;

Done.

https://codereview.chromium.org/19498003/diff/33010/net/dns/dns_test_util.cc#...
net/dns/dns_test_util.cc:151: class MockAddressSorter : public AddressSorter {
On 2013/08/06 20:35:31, szym wrote:
> It's strange to see it between MockTransaction and MockTransacitonFactory.
> Suggest either leaving it where it is or moving it to the top of the file.

Moved to top of file, since I prefer having it in an anonymous namespace.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1096: if (needs_two_transactions() &&
num_completed_transactions_ == 1) {
On 2013/08/06 20:35:31, szym wrote:
> At this point we could also set key_.hostname = transaction->GetHostname() so
> that if the second transaction is still queued, it does not need to perform
> suffix search.

Done.  Wonder how difficult it would be to make a unit test for this...

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1165: // Number of transactions that have started,
but not yet completed.
On 2013/08/06 20:35:31, szym wrote:
> The name of the variable is in direct conflict with the comment above it. I
> don't think this really needs a comment.

Done.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1219: KillDnsTask();
On 2013/08/06 20:35:31, szym wrote:
> I suggest leave it as is. Job is destroyed while running only if the whole
> resolver is destroyed as well, so all we want to do is log.

I'm not comfortable relying on that being true - it's the kind of thing that's
hard to test for.  Could very easily see that change, and then permanently
losing dispatcher slots as a result.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1254: // and the second attempt does not complete
synchronously.
On 2013/08/06 20:35:31, szym wrote:
> "attempt" is ambiguous. Suggest:
> "The dispatcher could have started |this| in Add, which could have called
> Schedule again. In that case |handle| will be null, but |handle_| will already
> have been set by the other Schedule."

Done (With some slight rewording).

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1386: void FinishOrCancelSecondDispatcherJob() {
On 2013/08/06 20:35:31, szym wrote:
> I've spent a lot of time trying to make it more obvious what this function
does.
> The key problem seems to be that it is supposed to solve three separate
issues,
> but the code ends up the same in both cases.
> 
> Case 1: The Job ends while running, so clean it up.
>  Subcase 1: occupying two job slots. Signal OnJobFinished for the extra slot.
>  Subcase 2: queued for another job slot. Cancel the queue handle.
> OnJobFinished for the remaining slot is called in CompleteRequests.
> 
> Case 2: Work in the first job slot has finished. Leverage it for the other job
> slot.
>  Subcase 1: other job slot already running. Signal OnJobFinished for the
> completed slot.
>  Subcase 2: other job still in queue. Cancel its queue handle and take over
the
> job slot we already have.
> 
> Case 3: DnsTask failed. Fallback to ProcTask.
>  Similar subcases to case 1, except that ProcTask takes over the remaining
slot.
> 
> 
> How about:
> // Reduce the number of job slots occupied and queued in the dispatcher
> // to one. If there is work still queued for the second job slot, let it
> // take over this one.
> ReduceToOneJobSlot() {
>   DCHECK(num_occupied_job_slots_ >= 1);
>   if (is_queued()) {
>     dispatcher_.Cancel(handle_);
>     handle_.Reset();
>   } else if (num_occupied_job_slots_ > 1) {
>     dispatcher_.OnJobFinished();
>     --num_occupied_job_slots_;
>   }
>   DCHECK(num_occupied_job_slots_ == 1);
> }

Done, though I changed the comment (What "let" means in the second sentence
seems rather ambiguous), and kept the DCHECK_*s instead of DCHECK.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1531: // Schedule a second transaciton, if needed.
On 2013/08/06 20:35:31, szym wrote:
> nit: transaction

Done.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1611: // If the second job is still queued, start
it now.
On 2013/08/06 20:35:31, szym wrote:
> "Second job" is a bit confusing. Suggest further explanation:
> We already have a job slot at the dispatcher, so reuse it now instead of
waiting
> in the queue for the second slot.

Done (Comment slightly modified).

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1614: FinishOrCancelSecondDispatcherJob();
On 2013/08/06 20:35:31, szym wrote:
> If we already started both jobs, then we are actually finishing the _first_
> slot. Renaming the function will help.
> 
> We StartSecondTransaction before canceling the second dispatcher job from the
> queue. I understand this is because after canceling, we can't check
is_queued(),
> but it's counter-intuitive.
> 
> It'd be more obvious if it was:
> 
> DCHECK_EQ(dns_task_->needs_another_transaction(), is_queued());
> ReduceToOneJobSlot();
> if (dns_task_->needs_another_transaction())
>   dns_task_->StartSecondTransaction();
> 

Done.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_impl.h
File net/dns/host_resolver_impl.h (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.h:227: // HostResolverImpl::Job moccupy multiple
PrioritizedDispatcher job slots.
On 2013/08/06 20:35:31, szym wrote:
> moccupy => could occupy

Done.

> Probably OK to skip HostResolverImpl::

Since Job is overloaded (PrioritiedDispatcher has them and HostResolverImpl has
them), I prefer to keep the "HostResolverImpl::" prefix, to reduce ambiguity.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1327: MockDnsClient* dns_client_;
On 2013/08/06 20:35:31, szym wrote:
>   // Owned by |resolver_|.

Done.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1675: TEST_F(HostResolverImplDnsTest,
AsyncResolverCancelWithOneTransactionActive) {
On 2013/08/06 20:35:31, szym wrote:
> No need for AsyncResolver prefix to these tests. All HostResolverImplDnsTest
> exercise DnsTask.

Done (Though I think it's very easy to miss the "DNS" in
"HostResolverImplDnsTest").

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1676: set_fallback_to_proctask(false);
On 2013/08/06 20:35:31, szym wrote:
> I don't think these tests need set_fallback_to_proctask(false);

Done.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1682: requests_[0]->Cancel();
On 2013/08/06 20:35:31, szym wrote:
> When the test depends on TearDown, suggest comment at end of test:
> // Check result in TearDown.

Done.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1740: 
On 2013/08/06 20:35:31, szym wrote:
> Add a test which ends in resolver_.Reset() without canceling the requests.

Done (Added two, actually).

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1743: set_fallback_to_proctask(false);
On 2013/08/06 20:35:31, szym wrote:
> OK. These three last tests do need fallback disabled.

Yea, I wrote them first, and then just copied + pasted for the others, without
thinking about it.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1806: EXPECT_EQ(ERR_IO_PENDING,
CreateRequest("ok", 80, MEDIUM,
On 2013/08/06 20:35:31, szym wrote:
> MEDIUM is the default priority in CreateRequest.

There's no overload that takes an address family, but not a priority.  I use an
IPv4 request so it only occupies one slot.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1808: EXPECT_EQ(ERR_IO_PENDING,
CreateRequest("4slow_ok", 80)->Resolve());
On 2013/08/06 20:35:31, szym wrote:
> Maybe make the priority explicit here.

Done.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1809: // An IPv4 request should have been
started pending for each job.
On 2013/08/06 20:35:31, szym wrote:
> Actually, the first Resolve() call will cause the first Job to launch both of
> its transactions. The two job slots for the second Resolve() will be queued
> despite its higher priority.

The first resolve only has one transaction (It's IPv4 for the purpose).  The
jobs are supposed to have the same priority.  The purpose of the test is to have
a Job's second transaction start when the transaction of some other job
completes while the first transaction is still running.  A serial resolver can't
do that.

> To exercise the intended scenario use CreateSerialResolver() here and just
> interleave them as follows:
> 
> req0->Resolve at MEDIUM, it takes the job slot, queues another at MEDIUM
> req1->Resolve at HIGH, it queues two at HIGH
> 
> RunUntilIdle
> req0's first job completes, req1's first job is started
> req1's first job completes, req1's second job is started
> req1's first job completes, req0's second job is started
> req0's second job completes
> 
> Up to you if you want to sprinkle in some delayed transactions.

Your suggestion doesn't actually work - when req0's first job completes, req0's
second job will start, if it hasn't already, since I bypass the dispatcher's
queue in that case.

[mmenke, 2013-08-19 17:32:04]

Also, thanks for the great feedback - very useful comments.

[mmenke, 2013-08-19 18:12:08]

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1809: // An IPv4 request should have been
started pending for each job.
To give a better explanation of how this works:

Request 0 starts its only transaction on creation.
Request 1 starts its A transaction on creation.
Request 0 completes, request 1 starts its AAAA transaction (In the RunUntilIdle
call).
Request 1's AAAA transaction completes (Also in the RunUntilIdle call)
Request 1's A transaction completes (In the CompleteDelayedTransactions call).

[szym, 2013-08-19 18:47:20]

LGTM with nits, and a couple suggestions.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1219: KillDnsTask();
On 2013/08/19 17:31:11, mmenke wrote:
> On 2013/08/06 20:35:31, szym wrote:
> > I suggest leave it as is. Job is destroyed while running only if the whole
> > resolver is destroyed as well, so all we want to do is log.
> 
> I'm not comfortable relying on that being true - it's the kind of thing that's
> hard to test for.  Could very easily see that change, and then permanently
> losing dispatcher slots as a result.

I understand the sentiment. My major concern is any extra work during
destruction:
~HostResolverImpl
STLDeleteValues
~Job
Job::KillDnsTask
PrioritizedDispatcher::OnJobComplete
Job::Start
// spawns new WorkerPool tasks or sends new UDP packets

Would it make sense to add PrioritizedDispatcher::Flush() or ::Clear() which
means "remove all Jobs from all queues"?

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1806: EXPECT_EQ(ERR_IO_PENDING,
CreateRequest("ok", 80, MEDIUM,
On 2013/08/19 17:31:11, mmenke wrote:
> On 2013/08/06 20:35:31, szym wrote:
> > MEDIUM is the default priority in CreateRequest.
> 
> There's no overload that takes an address family, but not a priority.  I use
an
> IPv4 request so it only occupies one slot.

I totally missed that this request is IPV4 only. Sorry.

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1809: // An IPv4 request should have been
started pending for each job.
On 2013/08/19 17:31:11, mmenke wrote:
> On 2013/08/06 20:35:31, szym wrote:
> > Actually, the first Resolve() call will cause the first Job to launch both
of
> > its transactions. The two job slots for the second Resolve() will be queued
> > despite its higher priority.
> 
> The first resolve only has one transaction (It's IPv4 for the purpose).  The
> jobs are supposed to have the same priority.  The purpose of the test is to
have
> a Job's second transaction start when the transaction of some other job
> completes while the first transaction is still running.  A serial resolver
can't
> do that.

I understand now. It all makes sense once I noticed that req0 is single family.

> 
> > To exercise the intended scenario use CreateSerialResolver() here and just
> > interleave them as follows:
> > 
> > req0->Resolve at MEDIUM, it takes the job slot, queues another at MEDIUM
> > req1->Resolve at HIGH, it queues two at HIGH
> > 
> > RunUntilIdle
> > req0's first job completes, req1's first job is started
> > req1's first job completes, req1's second job is started
> > req1's first job completes, req0's second job is started
> > req0's second job completes
> > 
> > Up to you if you want to sprinkle in some delayed transactions.
> 
> Your suggestion doesn't actually work - when req0's first job completes,
req0's
> second job will start, if it hasn't already, since I bypass the dispatcher's
> queue in that case.

Right.

https://codereview.chromium.org/19498003/diff/158001/net/dns/host_resolver_im...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/19498003/diff/158001/net/dns/host_resolver_im...
net/dns/host_resolver_impl.cc:1391: // queued job. Otherwise, the second Job was
been started by the
nit: was -> has

https://codereview.chromium.org/19498003/diff/158001/net/dns/host_resolver_im...
net/dns/host_resolver_impl.cc:1392: // PriorityizedDispatcher, so signals it is
complete.
nit: Prioritized

https://codereview.chromium.org/19498003/diff/158001/net/dns/host_resolver_im...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/19498003/diff/158001/net/dns/host_resolver_im...
net/dns/host_resolver_impl_unittest.cc:1734: }
To improve coverage, I suggest a test with more than one running and queued
Jobs.

[mmenke, 2013-08-19 19:31:33]

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1219: KillDnsTask();
On 2013/08/19 18:47:21, szym wrote:
> On 2013/08/19 17:31:11, mmenke wrote:
> > On 2013/08/06 20:35:31, szym wrote:
> > > I suggest leave it as is. Job is destroyed while running only if the whole
> > > resolver is destroyed as well, so all we want to do is log.
> > 
> > I'm not comfortable relying on that being true - it's the kind of thing
that's
> > hard to test for.  Could very easily see that change, and then permanently
> > losing dispatcher slots as a result.
> 
> I understand the sentiment. My major concern is any extra work during
> destruction:
> ~HostResolverImpl
> STLDeleteValues
> ~Job
> Job::KillDnsTask
> PrioritizedDispatcher::OnJobComplete
> Job::Start
> // spawns new WorkerPool tasks or sends new UDP packets
> 
> Would it make sense to add PrioritizedDispatcher::Flush() or ::Clear() which
> means "remove all Jobs from all queues"?

I don't think performance is a huge concern, since it happens at most 8 times
(We only cancel/complete the second request)....  However, I agree that the
re-entrancy you're pointing out is a greater concern than my worry about
regressions.

I don't think adding either of the new functions you suggest to the
PrioritizedDispatcher really answers my concern, which is after some future
refactor, we end up here when not deleting a Job, so permanently lose a job
slot.  We could add some sort of Pause() or PrepareToDie() function that stops
it from starting new jobs, but I think either would be more effort than it's
worth.

I've gone with your original suggestion, with a comment about it.  Open to other
ideas.

https://codereview.chromium.org/19498003/diff/158001/net/dns/host_resolver_im...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/19498003/diff/158001/net/dns/host_resolver_im...
net/dns/host_resolver_impl.cc:1391: // queued job. Otherwise, the second Job was
been started by the
On 2013/08/19 18:47:21, szym wrote:
> nit: was -> has
Done

https://codereview.chromium.org/19498003/diff/158001/net/dns/host_resolver_im...
net/dns/host_resolver_impl.cc:1392: // PriorityizedDispatcher, so signals it is
complete.
On 2013/08/19 18:47:21, szym wrote:
> nit: Prioritized

Done

https://codereview.chromium.org/19498003/diff/158001/net/dns/host_resolver_im...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/19498003/diff/158001/net/dns/host_resolver_im...
net/dns/host_resolver_impl_unittest.cc:1734: }
On 2013/08/19 18:47:21, szym wrote:
> To improve coverage, I suggest a test with more than one running and queued
> Jobs.

Done.

[szym, 2013-08-19 19:46:45]

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1219: KillDnsTask();
On 2013/08/19 19:31:33, mmenke wrote:
> On 2013/08/19 18:47:21, szym wrote:
> > On 2013/08/19 17:31:11, mmenke wrote:
> > > On 2013/08/06 20:35:31, szym wrote:
> > > > I suggest leave it as is. Job is destroyed while running only if the
whole
> > > > resolver is destroyed as well, so all we want to do is log.
> > > 
> > > I'm not comfortable relying on that being true - it's the kind of thing
> that's
> > > hard to test for.  Could very easily see that change, and then permanently
> > > losing dispatcher slots as a result.
> > 
> > I understand the sentiment. My major concern is any extra work during
> > destruction:
> > ~HostResolverImpl
> > STLDeleteValues
> > ~Job
> > Job::KillDnsTask
> > PrioritizedDispatcher::OnJobComplete
> > Job::Start
> > // spawns new WorkerPool tasks or sends new UDP packets
> > 
> > Would it make sense to add PrioritizedDispatcher::Flush() or ::Clear() which
> > means "remove all Jobs from all queues"?
> 
> I don't think performance is a huge concern, since it happens at most 8 times
> (We only cancel/complete the second request)....  However, I agree that the
> re-entrancy you're pointing out is a greater concern than my worry about
> regressions.
> 
> I don't think adding either of the new functions you suggest to the
> PrioritizedDispatcher really answers my concern, which is after some future
> refactor, we end up here when not deleting a Job, so permanently lose a job
> slot.  We could add some sort of Pause() or PrepareToDie() function that stops
> it from starting new jobs, but I think either would be more effort than it's
> worth.
> 
> I've gone with your original suggestion, with a comment about it.  Open to
other
> ideas.

I wasn't clear. I am suggesting going with your code, but in:
~HostResolverImpl(){
  dispatcher_.Flush();  // Empty the queues so that no further jobs get started.
  STLDeleteValues(jobs_);  // It's ok to KillDnsTask here, because OnJobComplete
is almost a no-op.
}

[mmenke, 2013-08-19 19:57:40]

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1219: KillDnsTask();
On 2013/08/19 19:46:45, szym wrote:
> On 2013/08/19 19:31:33, mmenke wrote:
> > On 2013/08/19 18:47:21, szym wrote:
> > > On 2013/08/19 17:31:11, mmenke wrote:
> > > > On 2013/08/06 20:35:31, szym wrote:
> > > > > I suggest leave it as is. Job is destroyed while running only if the
> whole
> > > > > resolver is destroyed as well, so all we want to do is log.
> > > > 
> > > > I'm not comfortable relying on that being true - it's the kind of thing
> > that's
> > > > hard to test for.  Could very easily see that change, and then
permanently
> > > > losing dispatcher slots as a result.
> > > 
> > > I understand the sentiment. My major concern is any extra work during
> > > destruction:
> > > ~HostResolverImpl
> > > STLDeleteValues
> > > ~Job
> > > Job::KillDnsTask
> > > PrioritizedDispatcher::OnJobComplete
> > > Job::Start
> > > // spawns new WorkerPool tasks or sends new UDP packets
> > > 
> > > Would it make sense to add PrioritizedDispatcher::Flush() or ::Clear()
which
> > > means "remove all Jobs from all queues"?
> > 
> > I don't think performance is a huge concern, since it happens at most 8
times
> > (We only cancel/complete the second request)....  However, I agree that the
> > re-entrancy you're pointing out is a greater concern than my worry about
> > regressions.
> > 
> > I don't think adding either of the new functions you suggest to the
> > PrioritizedDispatcher really answers my concern, which is after some future
> > refactor, we end up here when not deleting a Job, so permanently lose a job
> > slot.  We could add some sort of Pause() or PrepareToDie() function that
stops
> > it from starting new jobs, but I think either would be more effort than it's
> > worth.
> > 
> > I've gone with your original suggestion, with a comment about it.  Open to
> other
> > ideas.
> 
> I wasn't clear. I am suggesting going with your code, but in:
> ~HostResolverImpl(){
>   dispatcher_.Flush();  // Empty the queues so that no further jobs get
started.
>   STLDeleteValues(jobs_);  // It's ok to KillDnsTask here, because
OnJobComplete
> is almost a no-op.
> }

That causes problems in the other case...  When ReduceToOneJobSlot() calls
resolver_->dispatcher_.Cancel(handle_), then handle won't have been queued, and
we'd get a DCHECK in PriorityQueue::Erase.  We could remove those DCHECKs,
though I rather like them.

[szym, 2013-08-19 20:00:40]

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1219: KillDnsTask();
On 2013/08/19 19:57:40, mmenke wrote:
> On 2013/08/19 19:46:45, szym wrote:
> > On 2013/08/19 19:31:33, mmenke wrote:
> > > On 2013/08/19 18:47:21, szym wrote:
> > > > On 2013/08/19 17:31:11, mmenke wrote:
> > > > > On 2013/08/06 20:35:31, szym wrote:
> > > > > > I suggest leave it as is. Job is destroyed while running only if the
> > whole
> > > > > > resolver is destroyed as well, so all we want to do is log.
> > > > > 
> > > > > I'm not comfortable relying on that being true - it's the kind of
thing
> > > that's
> > > > > hard to test for.  Could very easily see that change, and then
> permanently
> > > > > losing dispatcher slots as a result.
> > > > 
> > > > I understand the sentiment. My major concern is any extra work during
> > > > destruction:
> > > > ~HostResolverImpl
> > > > STLDeleteValues
> > > > ~Job
> > > > Job::KillDnsTask
> > > > PrioritizedDispatcher::OnJobComplete
> > > > Job::Start
> > > > // spawns new WorkerPool tasks or sends new UDP packets
> > > > 
> > > > Would it make sense to add PrioritizedDispatcher::Flush() or ::Clear()
> which
> > > > means "remove all Jobs from all queues"?
> > > 
> > > I don't think performance is a huge concern, since it happens at most 8
> times
> > > (We only cancel/complete the second request)....  However, I agree that
the
> > > re-entrancy you're pointing out is a greater concern than my worry about
> > > regressions.
> > > 
> > > I don't think adding either of the new functions you suggest to the
> > > PrioritizedDispatcher really answers my concern, which is after some
future
> > > refactor, we end up here when not deleting a Job, so permanently lose a
job
> > > slot.  We could add some sort of Pause() or PrepareToDie() function that
> stops
> > > it from starting new jobs, but I think either would be more effort than
it's
> > > worth.
> > > 
> > > I've gone with your original suggestion, with a comment about it.  Open to
> > other
> > > ideas.
> > 
> > I wasn't clear. I am suggesting going with your code, but in:
> > ~HostResolverImpl(){
> >   dispatcher_.Flush();  // Empty the queues so that no further jobs get
> started.
> >   STLDeleteValues(jobs_);  // It's ok to KillDnsTask here, because
> OnJobComplete
> > is almost a no-op.
> > }
> 
> That causes problems in the other case...  When ReduceToOneJobSlot() calls
> resolver_->dispatcher_.Cancel(handle_), then handle won't have been queued,
and
> we'd get a DCHECK in PriorityQueue::Erase.  We could remove those DCHECKs,
> though I rather like them.

Well, then how about PrioritizedDispatcher::Disable() which tells it to stop
dispatching jobs? The simplest way to implement this is to reduce the number of
allowed job slots to 0.

[mmenke, 2013-08-19 20:41:12]

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/19498003/diff/33010/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:1219: KillDnsTask();
On 2013/08/19 20:00:41, szym wrote:
> On 2013/08/19 19:57:40, mmenke wrote:
> > On 2013/08/19 19:46:45, szym wrote:
> > > On 2013/08/19 19:31:33, mmenke wrote:
> > > > On 2013/08/19 18:47:21, szym wrote:
> > > > > On 2013/08/19 17:31:11, mmenke wrote:
> > > > > > On 2013/08/06 20:35:31, szym wrote:
> > > > > > > I suggest leave it as is. Job is destroyed while running only if
the
> > > whole
> > > > > > > resolver is destroyed as well, so all we want to do is log.
> > > > > > 
> > > > > > I'm not comfortable relying on that being true - it's the kind of
> thing
> > > > that's
> > > > > > hard to test for.  Could very easily see that change, and then
> > permanently
> > > > > > losing dispatcher slots as a result.
> > > > > 
> > > > > I understand the sentiment. My major concern is any extra work during
> > > > > destruction:
> > > > > ~HostResolverImpl
> > > > > STLDeleteValues
> > > > > ~Job
> > > > > Job::KillDnsTask
> > > > > PrioritizedDispatcher::OnJobComplete
> > > > > Job::Start
> > > > > // spawns new WorkerPool tasks or sends new UDP packets
> > > > > 
> > > > > Would it make sense to add PrioritizedDispatcher::Flush() or ::Clear()
> > which
> > > > > means "remove all Jobs from all queues"?
> > > > 
> > > > I don't think performance is a huge concern, since it happens at most 8
> > times
> > > > (We only cancel/complete the second request)....  However, I agree that
> the
> > > > re-entrancy you're pointing out is a greater concern than my worry about
> > > > regressions.
> > > > 
> > > > I don't think adding either of the new functions you suggest to the
> > > > PrioritizedDispatcher really answers my concern, which is after some
> future
> > > > refactor, we end up here when not deleting a Job, so permanently lose a
> job
> > > > slot.  We could add some sort of Pause() or PrepareToDie() function that
> > stops
> > > > it from starting new jobs, but I think either would be more effort than
> it's
> > > > worth.
> > > > 
> > > > I've gone with your original suggestion, with a comment about it.  Open
to
> > > other
> > > > ideas.
> > > 
> > > I wasn't clear. I am suggesting going with your code, but in:
> > > ~HostResolverImpl(){
> > >   dispatcher_.Flush();  // Empty the queues so that no further jobs get
> > started.
> > >   STLDeleteValues(jobs_);  // It's ok to KillDnsTask here, because
> > OnJobComplete
> > > is almost a no-op.
> > > }
> > 
> > That causes problems in the other case...  When ReduceToOneJobSlot() calls
> > resolver_->dispatcher_.Cancel(handle_), then handle won't have been queued,
> and
> > we'd get a DCHECK in PriorityQueue::Erase.  We could remove those DCHECKs,
> > though I rather like them.
> 
> Well, then how about PrioritizedDispatcher::Disable() which tells it to stop
> dispatching jobs? The simplest way to implement this is to reduce the number
of
> allowed job slots to 0.

Done.  That was what I meant by my PrepareToDie() suggestion above.  More
complicated than I like, but it works, and it's not too bad.

https://codereview.chromium.org/19498003/diff/188002/net/base/prioritized_dis...
File net/base/prioritized_dispatcher_unittest.cc (right):

https://codereview.chromium.org/19498003/diff/188002/net/base/prioritized_dis...
net/base/prioritized_dispatcher_unittest.cc:311: ASSERT_TRUE(job_e->running());
Looks like we weren't testing this code path before.

[szym, 2013-08-19 20:49:34]

https://codereview.chromium.org/19498003/diff/188002/net/base/prioritized_dis...
File net/base/prioritized_dispatcher.cc (right):

https://codereview.chromium.org/19498003/diff/188002/net/base/prioritized_dis...
net/base/prioritized_dispatcher.cc:109: if (disabled_ || num_running_jobs_ >=
max_running_jobs_[job_priority])
If instead of |disabled_|, you set max_running_jobs_ to 0, you wouldn't need an
extra condition in all these codepaths.

While it does make it possible for there to be more outstanding jobs than the
max_running_jobs_, I don't think should stop us (and in the future we might
bechanging the limit dynamically).

[mmenke, 2013-08-19 21:09:58]

https://codereview.chromium.org/19498003/diff/188002/net/base/prioritized_dis...
File net/base/prioritized_dispatcher.cc (right):

https://codereview.chromium.org/19498003/diff/188002/net/base/prioritized_dis...
net/base/prioritized_dispatcher.cc:109: if (disabled_ || num_running_jobs_ >=
max_running_jobs_[job_priority])
On 2013/08/19 20:49:34, szym wrote:
> If instead of |disabled_|, you set max_running_jobs_ to 0, you wouldn't need
an
> extra condition in all these codepaths.
> 
> While it does make it possible for there to be more outstanding jobs than the
> max_running_jobs_, I don't think should stop us (and in the future we might
> bechanging the limit dynamically).

Not sure why this didn't occur to me.  It's pretty easy to make SetLimits start
new jobs if needed, but I've chosen not to do so, simply because it seems beyond
the scope of this CL and we currently don't need it.  If you prefer that I add
support for that, however, happy to do so.

[szym, 2013-08-20 16:24:57]

lgtm

https://codereview.chromium.org/19498003/diff/201002/net/base/prioritized_dis...
File net/base/prioritized_dispatcher.cc (right):

https://codereview.chromium.org/19498003/diff/201002/net/base/prioritized_dis...
net/base/prioritized_dispatcher.cc:108: void
PrioritizedDispatcher::SetLimits(const Limits& limits) {
suggest adding
DCHECK_EQ(queue_.num_priorities(), limits.reserved_slots.size());

https://codereview.chromium.org/19498003/diff/201002/net/base/priority_queue.h
File net/base/priority_queue.h (right):

https://codereview.chromium.org/19498003/diff/201002/net/base/priority_queue....
net/base/priority_queue.h:235: size_t NumPriorities() const { return
lists_.size(); }
nit: num_priorities

[mmenke, 2013-08-20 16:33:13]

Thanks for the feedback!

https://codereview.chromium.org/19498003/diff/201002/net/base/prioritized_dis...
File net/base/prioritized_dispatcher.cc (right):

https://codereview.chromium.org/19498003/diff/201002/net/base/prioritized_dis...
net/base/prioritized_dispatcher.cc:108: void
PrioritizedDispatcher::SetLimits(const Limits& limits) {
On 2013/08/20 16:24:57, szym wrote:
> suggest adding
> DCHECK_EQ(queue_.num_priorities(), limits.reserved_slots.size());

SGTM.  Done.

https://codereview.chromium.org/19498003/diff/201002/net/base/priority_queue.h
File net/base/priority_queue.h (right):

https://codereview.chromium.org/19498003/diff/201002/net/base/priority_queue....
net/base/priority_queue.h:235: size_t NumPriorities() const { return
lists_.size(); }
On 2013/08/20 16:24:57, szym wrote:
> nit: num_priorities

Done.  I would have considered size() complicated enough to be worth the caps,
but looks like it's more often lowercase, in similar situations.

[commit-bot: I haz the power, 2013-08-20 16:35:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/19498003/234001

[szym, 2013-08-20 20:11:48]

https://codereview.chromium.org/19498003/diff/234001/net/dns/host_resolver_im...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/19498003/diff/234001/net/dns/host_resolver_im...
net/dns/host_resolver_impl.cc:1888: job->Schedule(false);
Looks like you'll need to resync this to http://crrev.com/218525

[mmenke, 2013-08-20 20:17:14]

On 2013/08/20 20:11:48, szym wrote:
>
https://codereview.chromium.org/19498003/diff/234001/net/dns/host_resolver_im...
> File net/dns/host_resolver_impl.cc (right):
> 
>
https://codereview.chromium.org/19498003/diff/234001/net/dns/host_resolver_im...
> net/dns/host_resolver_impl.cc:1888: job->Schedule(false);
> Looks like you'll need to resync this to http://crrev.com/218525

Yea, looks like I will.  Thanks!

[commit-bot: I haz the power, 2013-08-20 20:55:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/19498003/249001

[commit-bot: I haz the power, 2013-08-21 03:24:54]

Change committed as 218616

[szym, 2013-08-22 15:50:07]

Let me know if you want to revert and fix. Otherwise, I'll just fix without
reverting.

https://chromiumcodereview.appspot.com/19498003/diff/249001/net/dns/host_reso...
File net/dns/host_resolver_impl.cc (left):

https://chromiumcodereview.appspot.com/19498003/diff/249001/net/dns/host_reso...
net/dns/host_resolver_impl.cc:1075: OnFailure(ERR_NAME_NOT_RESOLVED,
DnsResponse::DNS_PARSE_OK);
Oh oh... Looks like we missed handling of this case leading to
http://crbug.com/277625
I thought we had a test covering this case.

How about:
proc_->AddRuleForAllFamilies("6ok_fallback", "192.168.0.1");
CreateRequest("6ok_fallback", 80, MEDIUM, ADDRESS_FAMILY_IPV4)->Resolve();
EXPECT_EQ(OK, requests_[0]->WaitForResult());
EXPECT_TRUE(requests_[0]->HasOneAddress("192.168.0.1", 80));

Could also consider adding a MockDnsRule for "empty" which just returns an empty
response.

https://chromiumcodereview.appspot.com/19498003/diff/249001/net/dns/host_reso...
File net/dns/host_resolver_impl.cc (right):

https://chromiumcodereview.appspot.com/19498003/diff/249001/net/dns/host_reso...
net/dns/host_resolver_impl.cc:1110: 
if (addr_list.empty()) {
  OnFailure(ERR_NAME_NOT_RESOLVED, DnsResponse::DNS_PARSE_OK);
  return;
}

[mmenke1, 2013-08-22 15:53:46]

On 2013/08/22 15:50:07, szym wrote:
> Let me know if you want to revert and fix. Otherwise, I'll just fix without
> reverting.

I'll revert and fix.

>
https://chromiumcodereview.appspot.com/19498003/diff/249001/net/dns/host_reso...
> File net/dns/host_resolver_impl.cc (left):
> 
>
https://chromiumcodereview.appspot.com/19498003/diff/249001/net/dns/host_reso...
> net/dns/host_resolver_impl.cc:1075: OnFailure(ERR_NAME_NOT_RESOLVED,
> DnsResponse::DNS_PARSE_OK);
> Oh oh... Looks like we missed handling of this case leading to
> http://crbug.com/277625
> I thought we had a test covering this case.
> 
> How about:
> proc_->AddRuleForAllFamilies("6ok_fallback", "192.168.0.1");
> CreateRequest("6ok_fallback", 80, MEDIUM, ADDRESS_FAMILY_IPV4)->Resolve();
> EXPECT_EQ(OK, requests_[0]->WaitForResult());
> EXPECT_TRUE(requests_[0]->HasOneAddress("192.168.0.1", 80));
> 
> Could also consider adding a MockDnsRule for "empty" which just returns an
empty
> response.
> 
>
https://chromiumcodereview.appspot.com/19498003/diff/249001/net/dns/host_reso...
> File net/dns/host_resolver_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/19498003/diff/249001/net/dns/host_reso...
> net/dns/host_resolver_impl.cc:1110: 
> if (addr_list.empty()) {
>   OnFailure(ERR_NAME_NOT_RESOLVED, DnsResponse::DNS_PARSE_OK);
>   return;
> }