Give localserver time to shut down

Give localserver time to shut down

On machines with slow I/O, the localserver might not shut down immediately,
wait for it if necessary.

BUG=

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

[sigbjorn, 2014-02-11 17:30:19]

Please forward if I got the wrong reviewers (though a one line change should be
quick to review)

[M-A Ruel, 2014-02-11 18:03:49]

On 2014/02/11 17:30:19, sigbjorn wrote:
> Please forward if I got the wrong reviewers (though a one line change should
be
> quick to review)

You didn't set any reviewer. You'll want to use one that is in the closest
OWNERS file.

[Paweł Hajdan Jr., 2014-02-12 18:45:51]

Well, what's wrong with killing the process if it doesn't shut down in time?

[sigbjorn, 2014-02-13 09:01:01]

On 2014/02/12 18:45:51, Paweł Hajdan Jr. wrote:
> Well, what's wrong with killing the process if it doesn't shut down in time?

"In time" was previously 0 seconds, so would fail frequently on slow machines.
For suitable definitions of "in time", I agree with you, which is what this
patch does.

KillProcess also attemtps to wait, so might as well wait in an more appropriate
place instead. If job_handle_.Close() and WaitForSingleProcess both failed,
KillProcess also tends to fail (I have not investigated why), which returns
false from the function, messing with tests which use the function.

[M-A Ruel, 2014-02-13 13:39:50]

On 2014/02/13 09:01:01, sigbjorn wrote:
> On 2014/02/12 18:45:51, Paweł Hajdan Jr. wrote:
> > Well, what's wrong with killing the process if it doesn't shut down in time?
> 
> "In time" was previously 0 seconds, so would fail frequently on slow machines.
> For suitable definitions of "in time", I agree with you, which is what this
> patch does.
> 
> KillProcess also attemtps to wait, so might as well wait in an more
appropriate
> place instead. If job_handle_.Close() and WaitForSingleProcess both failed,
> KillProcess also tends to fail (I have not investigated why), which returns
> false from the function, messing with tests which use the function.

To be clear, job_handle_.Close() should never fail. All the process in the job
objects are killed. The problem here from what I understand is that
WaitForSingleProcess doesn't wait at all, and since the process are killed
asynchronously, the the call to KillProcess may still execute.
Then the call to KillProcessw will likely fail because the process was killed at
that point.

I'm actually fine to put an infinite time out on Windows, but only on Windows
and remove the KillProcess call on Windows.

[sigbjorn, 2014-02-14 08:24:29]

On 2014/02/13 13:39:50, M-A Ruel wrote:
> I'm actually fine to put an infinite time out on Windows, but only on Windows
> and remove the KillProcess call on Windows.

Infinite sounds like a long time, if it doesn't shut down timely, the function
should still return an error. Increased to 60 seconds, and no calling of
KillProcess on Windows.

[M-A Ruel, 2014-02-14 13:54:35]

https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_se...
File net/test/spawned_test_server/local_test_server.cc (right):

https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_se...
net/test/spawned_test_server/local_test_server.cc:130: // Wait for the process
to terminate.
processes

http://msdn.microsoft.com/library/windows/desktop/ms684161.aspx
" The state of a job object is set to signaled when all of its processes are
terminated because the specified end-of-job time limit has been exceeded. "

So do not wait for process_handle_, wait for job_handle_ to become signaled.

https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_se...
net/test/spawned_test_server/local_test_server.cc:136: ret =
base::KillProcess(process_handle_, 1, true);
That doesn't compile.

[sigbjorn, 2014-02-14 16:05:50]

https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_se...
File net/test/spawned_test_server/local_test_server.cc (right):

https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_se...
net/test/spawned_test_server/local_test_server.cc:130: // Wait for the process
to terminate.
> processes
Fixed

> So do not wait for process_handle_, wait for job_handle_ to become signaled.
How do you propose to do this? job_handle is explicitly nulled in Close()

https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_se...
net/test/spawned_test_server/local_test_server.cc:136: ret =
base::KillProcess(process_handle_, 1, true);
On 2014/02/14 13:54:35, M-A Ruel wrote:
> That doesn't compile.

Which compiler and how?

[M-A Ruel, 2014-02-14 20:14:37]

https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_se...
File net/test/spawned_test_server/local_test_server.cc (right):

https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_se...
net/test/spawned_test_server/local_test_server.cc:130: // Wait for the process
to terminate.
On 2014/02/14 16:05:51, sigbjorn wrote:
> > So do not wait for process_handle_, wait for job_handle_ to become signaled.
> How do you propose to do this? job_handle is explicitly nulled in Close()
> 

With TerminateJobObject(), since you obviously can't wait on a closed handle.
http://msdn.microsoft.com/library/windows/desktop/ms686709.aspx

https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_se...
net/test/spawned_test_server/local_test_server.cc:136: ret =
base::KillProcess(process_handle_, 1, true);
On 2014/02/14 16:05:51, sigbjorn wrote:
> On 2014/02/14 13:54:35, M-A Ruel wrote:
> > That doesn't compile.
> 
> Which compiler and how?

Oh sorry, I missed the fact that you had removed { too.

[Paweł Hajdan Jr., 2014-02-14 20:14:52]

Please make sure KillProcess is also called on Windows.

Job objects should be a secondary safety net, not the primary mechanism we rely
on for process termination.

"not lgtm" in the current shape

[sigbjorn, 2014-02-17 12:50:39]

KillProcess restored to work also on Windows.

https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_se...
File net/test/spawned_test_server/local_test_server.cc (right):

https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_se...
net/test/spawned_test_server/local_test_server.cc:130: // Wait for the process
to terminate.
On 2014/02/14 20:14:37, M-A Ruel wrote:
> On 2014/02/14 16:05:51, sigbjorn wrote:
> > > So do not wait for process_handle_, wait for job_handle_ to become
signaled.
> > How do you propose to do this? job_handle is explicitly nulled in Close()
> > 
> 
> With TerminateJobObject(), since you obviously can't wait on a closed handle.
> http://msdn.microsoft.com/library/windows/desktop/ms686709.aspx

Done.

[Paweł Hajdan Jr., 2014-02-18 19:29:54]

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
File net/test/spawned_test_server/local_test_server.cc (right):

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
net/test/spawned_test_server/local_test_server.cc:129: bool ret =
TerminateJobObject(job_handle_, 0);
Can this actually fail? If so, when and why?

How is that different from just closing the job handle with
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE?

[M-A Ruel, 2014-02-18 19:39:32]

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
File net/test/spawned_test_server/local_test_server.cc (right):

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
net/test/spawned_test_server/local_test_server.cc:129: bool ret =
TerminateJobObject(job_handle_, 0);
On 2014/02/18 19:29:54, Paweł Hajdan Jr. wrote:
> Can this actually fail? If so, when and why?
> 
> How is that different from just closing the job handle with
> JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE?

Sigbjorn found out it is actually killing contained processes asynchronously.

[sigbjorn, 2014-02-19 09:29:12]

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
File net/test/spawned_test_server/local_test_server.cc (right):

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
net/test/spawned_test_server/local_test_server.cc:129: bool ret =
TerminateJobObject(job_handle_, 0);
On 2014/02/18 19:39:33, M-A Ruel wrote:
> On 2014/02/18 19:29:54, Paweł Hajdan Jr. wrote:
> > Can this actually fail? If so, when and why?
> > 
> > How is that different from just closing the job handle with
> > JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE?
> 
> Sigbjorn found out it is actually killing contained processes asynchronously.

Our tests would have random failures due to this function returning False. We
only saw the failures on our virtual test slaves with virtual disks (thus very
slow I/O). Increasing the timeout in WaitForSingleProcess from 0 to 10 seconds
eliminated the problem.

That was using job_handle_.Close() though, I have no idea if TerminateJobObject
behaves the same way.

[Paweł Hajdan Jr., 2014-02-19 19:20:48]

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
File net/test/spawned_test_server/local_test_server.cc (right):

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
net/test/spawned_test_server/local_test_server.cc:129: bool ret =
TerminateJobObject(job_handle_, 0);
On 2014/02/19 09:29:13, sigbjorn wrote:
> Our tests would have random failures due to this function returning False. We
> only saw the failures on our virtual test slaves with virtual disks (thus very
> slow I/O). Increasing the timeout in WaitForSingleProcess from 0 to 10 seconds
> eliminated the problem.
> 
> That was using job_handle_.Close() though, I have no idea if
TerminateJobObject
> behaves the same way.

Could you try this instead?

1. Close the job handle (like it was before).
2. Give the process more time to shut down (all platforms, and use the timeouts
from base/test/test_timeouts).
3. Always kill the process if wait fails.

[M-A Ruel, 2014-02-19 19:25:32]

On 2014/02/19 19:20:48, Paweł Hajdan Jr. wrote:
>
https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
> File net/test/spawned_test_server/local_test_server.cc (right):
> 
>
https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
> net/test/spawned_test_server/local_test_server.cc:129: bool ret =
> TerminateJobObject(job_handle_, 0);
> On 2014/02/19 09:29:13, sigbjorn wrote:
> > Our tests would have random failures due to this function returning False.
We
> > only saw the failures on our virtual test slaves with virtual disks (thus
very
> > slow I/O). Increasing the timeout in WaitForSingleProcess from 0 to 10
seconds
> > eliminated the problem.
> > 
> > That was using job_handle_.Close() though, I have no idea if
> TerminateJobObject
> > behaves the same way.
> 
> Could you try this instead?
> 
> 1. Close the job handle (like it was before).
> 2. Give the process more time to shut down (all platforms, and use the
timeouts
> from base/test/test_timeouts).
> 3. Always kill the process if wait fails.

I explicitly said to the the reverse!

Call TerminateJobObject()
Wait for the job object
Close it.

[sigbjorn, 2014-02-20 09:25:56]

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
File net/test/spawned_test_server/local_test_server.cc (right):

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
net/test/spawned_test_server/local_test_server.cc:129: bool ret =
TerminateJobObject(job_handle_, 0);
On 2014/02/19 19:20:49, Paweł Hajdan Jr. wrote:
> On 2014/02/19 09:29:13, sigbjorn wrote:
> > Our tests would have random failures due to this function returning False.
We
> > only saw the failures on our virtual test slaves with virtual disks (thus
very
> > slow I/O). Increasing the timeout in WaitForSingleProcess from 0 to 10
seconds
> > eliminated the problem.
> > 
> > That was using job_handle_.Close() though, I have no idea if
> TerminateJobObject
> > behaves the same way.
> 
> Could you try this instead?
> 
> 1. Close the job handle (like it was before).
> 2. Give the process more time to shut down (all platforms, and use the
timeouts
> from base/test/test_timeouts).
> 3. Always kill the process if wait fails.
You mean like patch set 1 did? Except change the hardcoded 10 to a proper
timeout?

I have no strong opinion on how this should be implemented, and will follow
recommendations, but right now we seem to be going in circles. If you and maruel
could agree on a preferred method first, then I can upload a patch which does
that.

[Paweł Hajdan Jr., 2014-02-20 19:06:19]

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
File net/test/spawned_test_server/local_test_server.cc (right):

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
net/test/spawned_test_server/local_test_server.cc:129: bool ret =
TerminateJobObject(job_handle_, 0);
On 2014/02/20 09:25:56, sigbjorn wrote:
> You mean like patch set 1 did? Except change the hardcoded 10 to a proper
> timeout?

Sounds reasonable.

> I have no strong opinion on how this should be implemented, and will follow
> recommendations, but right now we seem to be going in circles. If you and
maruel
> could agree on a preferred method first, then I can upload a patch which does
> that.

I think my comments are pretty consistent: mostly objecting to lack of
KillProcess.

[sigbjorn, 2014-02-21 13:02:10]

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
File net/test/spawned_test_server/local_test_server.cc (right):

https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_s...
net/test/spawned_test_server/local_test_server.cc:129: bool ret =
TerminateJobObject(job_handle_, 0);
On 2014/02/20 19:06:20, Paweł Hajdan Jr. wrote:
> On 2014/02/20 09:25:56, sigbjorn wrote:
> > You mean like patch set 1 did? Except change the hardcoded 10 to a proper
> > timeout?
> 
> Sounds reasonable.
Now in patch set 5

[Paweł Hajdan Jr., 2014-02-21 18:56:39]

Patch Set 5 LGTM++

Please let me take another look if any changes are made to it.

[sigbjorn, 2014-02-24 10:30:01]

The CQ bit was checked by sigbjorn@opera.com

[commit-bot: I haz the power, 2014-02-24 10:30:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/350001

[commit-bot: I haz the power, 2014-02-24 13:14:13]

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

[commit-bot: I haz the power, 2014-02-24 13:14:13]

Try jobs failed on following builders: linux_chromium_rel

[sigbjorn, 2014-02-24 14:20:44]

The CQ bit was checked by sigbjorn@opera.com

[commit-bot: I haz the power, 2014-02-24 14:20:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/350001

[commit-bot: I haz the power, 2014-02-24 14:21:05]

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

[commit-bot: I haz the power, 2014-02-24 14:21:05]

Try jobs failed on following builders: linux_chromium_rel, mac_chromium_rel

[sigbjorn, 2014-02-24 14:21:11]

On 2014/02/24 13:14:13, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders: linux_chromium_rel

I know to little about trybot to understand what is going on, trying again to
see if this was random failures.

[sigbjorn, 2014-02-26 15:26:08]

We have 4 stable failures in tests on linux_chromium_rel. They all time out in
WaitForSingleProcess, suggesting local_test_server does not shut down cleanly.
Either the tests are already very close to their timeout value, or trybot runs
in debug mode (where action_timeout_ms_ is modified). The problem is likely
elsewhere, uncovered by this change, but not introduced by it.

However, I know nothing about trybot, little about local_test_server, I can't
reproduce on Windows, and I don't have a Linux setup. Would one of you be able
to help me out by taking a look at these failures?

[sigbjorn, 2014-02-28 10:54:46]

On 2014/02/26 15:26:08, sigbjorn wrote:
> Would one of you be able
> to help me out by taking a look at these failures?

Alternatively I can apply the timeout only on Windows, as initially suggested in
comment #5?

[sigbjorn, 2014-03-12 10:37:16]

On 2014/02/28 10:54:46, sigbjorn wrote:
> On 2014/02/26 15:26:08, sigbjorn wrote:
> > Would one of you be able
> > to help me out by taking a look at these failures?
> 
> Alternatively I can apply the timeout only on Windows, as initially suggested
in
> comment #5?

Paweł and/or M-A Ruel, any thoughts?

[Paweł Hajdan Jr., 2014-03-12 16:03:53]

On 2014/03/12 10:37:16, sigbjorn wrote:
> Paweł and/or M-A Ruel, any thoughts?

There are no logs now.

I'm opposed to only applying the timeout on Windows.

Note that even if the timeout is "too short", the process should still get
killed, and if kill is successful the method should return true.

It's hard to say something more without the logs. Sorry for not looking at this
earlier.

[sigbjorn, 2014-03-13 08:02:58]

The CQ bit was checked by sigbjorn@opera.com

[commit-bot: I haz the power, 2014-03-13 08:03:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/350001

[commit-bot: I haz the power, 2014-03-13 10:23:47]

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

[commit-bot: I haz the power, 2014-03-13 10:23:48]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[sigbjorn, 2014-03-13 10:27:24]

On 2014/03/12 16:03:53, Paweł Hajdan Jr. wrote:
> On 2014/03/12 10:37:16, sigbjorn wrote:
> > Paweł and/or M-A Ruel, any thoughts?
> 
> There are no logs now.
> 
> I'm opposed to only applying the timeout on Windows.
> 
> Note that even if the timeout is "too short", the process should still get
> killed, and if kill is successful the method should return true.
> 
> It's hard to say something more without the logs. Sorry for not looking at
this
> earlier.

New logs on
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...
and
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...

Search for WaitForSingleProcess

[sigbjorn, 2014-03-18 14:25:41]

Paweł and/or M-A Ruel, any thoughts before the logs disappear again? Are there
any keywords I should use to ensure your attention in mail?

[M-A Ruel, 2014-03-18 19:36:03]

https://codereview.chromium.org/145873004/diff/350001/net/test/spawned_test_s...
File net/test/spawned_test_server/local_test_server.cc (right):

https://codereview.chromium.org/145873004/diff/350001/net/test/spawned_test_s...
net/test/spawned_test_server/local_test_server.cc:129: // This kills all the
processes in the job object.
Remove the change to lines 133-136 and replace lines 128-131 with the following:

#if defined(OS_WIN)
  // This kills all the processes in the job object.
  ::TerminateJobObject(job_handle_.Get(), 1);
  // This is done asynchronously so wait a bit for the processes to be killed.
  ::WaitForSingleObject(job_handle_.Get(), TestTimeouts::action_timeout());
  job_handle_.Close();
#endif

AFAIK, that's the only correct way to do this, independent of what Pawel said.
Reference:
http://msdn.microsoft.com/library/windows/desktop/ms684161.aspx

[sigbjorn, 2014-03-19 10:29:58]

The CQ bit was checked by sigbjorn@opera.com

[commit-bot: I haz the power, 2014-03-19 10:30:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/690001

[commit-bot: I haz the power, 2014-03-19 11:17:36]

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

[commit-bot: I haz the power, 2014-03-19 11:17:37]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sigbjorn, 2014-03-19 11:57:07]

The CQ bit was checked by sigbjorn@opera.com

[commit-bot: I haz the power, 2014-03-19 11:57:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/710001

[commit-bot: I haz the power, 2014-03-19 13:38:25]

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

[commit-bot: I haz the power, 2014-03-19 13:38:26]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sigbjorn, 2014-03-21 14:44:26]

The CQ bit was checked by sigbjorn@opera.com

[commit-bot: I haz the power, 2014-03-21 14:44:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/710001

[commit-bot: I haz the power, 2014-03-21 16:46:25]

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

[commit-bot: I haz the power, 2014-03-21 16:46:26]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sigbjorn, 2014-03-27 14:33:47]

The CQ bit was checked by sigbjorn@opera.com

[commit-bot: I haz the power, 2014-03-27 14:34:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/710001

[commit-bot: I haz the power, 2014-03-27 15:21:55]

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

[commit-bot: I haz the power, 2014-03-27 15:21:56]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[sigbjorn, 2014-03-28 12:04:24]

The CQ bit was checked by sigbjorn@opera.com

[commit-bot: I haz the power, 2014-03-28 12:12:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/710001

[commit-bot: I haz the power, 2014-03-28 18:17:44]

Change committed as 260201