Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(532)

Issue 145873004: Give localserver time to shut down (Closed)

Created:
6 years, 10 months ago by sigbjorn
Modified:
6 years, 9 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, Johnny(Jianning) Ding, Daniel Bratell
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Fixup! Give Windows plenty of time to shut down, don't call KillProcess #

Total comments: 7

Patch Set 3 : Comment cleanup #

Patch Set 4 : Use TerminateJobObject. Restore KillProcess for Windows #

Total comments: 7

Patch Set 5 : Original code, using action_timeout #

Total comments: 1

Patch Set 6 : Special Windows code for shutting down localserver #

Patch Set 7 : Proper windows timeout #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -0 lines) Patch
M net/test/spawned_test_server/local_test_server.cc View 1 2 3 4 5 6 2 chunks +5 lines, -0 lines 0 comments Download

Messages

Total messages: 59 (0 generated)
sigbjorn
Please forward if I got the wrong reviewers (though a one line change should be ...
6 years, 10 months ago (2014-02-11 17:30:19 UTC) #1
M-A Ruel
On 2014/02/11 17:30:19, sigbjorn wrote: > Please forward if I got the wrong reviewers (though ...
6 years, 10 months ago (2014-02-11 18:03:49 UTC) #2
Paweł Hajdan Jr.
Well, what's wrong with killing the process if it doesn't shut down in time?
6 years, 10 months ago (2014-02-12 18:45:51 UTC) #3
sigbjorn
On 2014/02/12 18:45:51, Paweł Hajdan Jr. wrote: > Well, what's wrong with killing the process ...
6 years, 10 months ago (2014-02-13 09:01:01 UTC) #4
M-A Ruel
On 2014/02/13 09:01:01, sigbjorn wrote: > On 2014/02/12 18:45:51, Paweł Hajdan Jr. wrote: > > ...
6 years, 10 months ago (2014-02-13 13:39:50 UTC) #5
sigbjorn
On 2014/02/13 13:39:50, M-A Ruel wrote: > I'm actually fine to put an infinite time ...
6 years, 10 months ago (2014-02-14 08:24:29 UTC) #6
M-A Ruel
https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_server/local_test_server.cc File net/test/spawned_test_server/local_test_server.cc (right): https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_server/local_test_server.cc#newcode130 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 ...
6 years, 10 months ago (2014-02-14 13:54:35 UTC) #7
sigbjorn
https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_server/local_test_server.cc File net/test/spawned_test_server/local_test_server.cc (right): https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_server/local_test_server.cc#newcode130 net/test/spawned_test_server/local_test_server.cc:130: // Wait for the process to terminate. > processes ...
6 years, 10 months ago (2014-02-14 16:05:50 UTC) #8
M-A Ruel
https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_server/local_test_server.cc File net/test/spawned_test_server/local_test_server.cc (right): https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_server/local_test_server.cc#newcode130 net/test/spawned_test_server/local_test_server.cc:130: // Wait for the process to terminate. On 2014/02/14 ...
6 years, 10 months ago (2014-02-14 20:14:37 UTC) #9
Paweł Hajdan Jr.
Please make sure KillProcess is also called on Windows. Job objects should be a secondary ...
6 years, 10 months ago (2014-02-14 20:14:52 UTC) #10
sigbjorn
KillProcess restored to work also on Windows. https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_server/local_test_server.cc File net/test/spawned_test_server/local_test_server.cc (right): https://codereview.chromium.org/145873004/diff/70001/net/test/spawned_test_server/local_test_server.cc#newcode130 net/test/spawned_test_server/local_test_server.cc:130: // Wait ...
6 years, 10 months ago (2014-02-17 12:50:39 UTC) #11
Paweł Hajdan Jr.
https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc File net/test/spawned_test_server/local_test_server.cc (right): https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc#newcode129 net/test/spawned_test_server/local_test_server.cc:129: bool ret = TerminateJobObject(job_handle_, 0); Can this actually fail? ...
6 years, 10 months ago (2014-02-18 19:29:54 UTC) #12
M-A Ruel
https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc File net/test/spawned_test_server/local_test_server.cc (right): https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc#newcode129 net/test/spawned_test_server/local_test_server.cc:129: bool ret = TerminateJobObject(job_handle_, 0); On 2014/02/18 19:29:54, Paweł ...
6 years, 10 months ago (2014-02-18 19:39:32 UTC) #13
sigbjorn
https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc File net/test/spawned_test_server/local_test_server.cc (right): https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc#newcode129 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 ...
6 years, 10 months ago (2014-02-19 09:29:12 UTC) #14
Paweł Hajdan Jr.
https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc File net/test/spawned_test_server/local_test_server.cc (right): https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc#newcode129 net/test/spawned_test_server/local_test_server.cc:129: bool ret = TerminateJobObject(job_handle_, 0); On 2014/02/19 09:29:13, sigbjorn ...
6 years, 10 months ago (2014-02-19 19:20:48 UTC) #15
M-A Ruel
On 2014/02/19 19:20:48, Paweł Hajdan Jr. wrote: > https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc > File net/test/spawned_test_server/local_test_server.cc (right): > > ...
6 years, 10 months ago (2014-02-19 19:25:32 UTC) #16
sigbjorn
https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc File net/test/spawned_test_server/local_test_server.cc (right): https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc#newcode129 net/test/spawned_test_server/local_test_server.cc:129: bool ret = TerminateJobObject(job_handle_, 0); On 2014/02/19 19:20:49, Paweł ...
6 years, 10 months ago (2014-02-20 09:25:56 UTC) #17
Paweł Hajdan Jr.
https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc File net/test/spawned_test_server/local_test_server.cc (right): https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc#newcode129 net/test/spawned_test_server/local_test_server.cc:129: bool ret = TerminateJobObject(job_handle_, 0); On 2014/02/20 09:25:56, sigbjorn ...
6 years, 10 months ago (2014-02-20 19:06:19 UTC) #18
sigbjorn
https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc File net/test/spawned_test_server/local_test_server.cc (right): https://codereview.chromium.org/145873004/diff/200001/net/test/spawned_test_server/local_test_server.cc#newcode129 net/test/spawned_test_server/local_test_server.cc:129: bool ret = TerminateJobObject(job_handle_, 0); On 2014/02/20 19:06:20, Paweł ...
6 years, 10 months ago (2014-02-21 13:02:10 UTC) #19
Paweł Hajdan Jr.
Patch Set 5 LGTM++ Please let me take another look if any changes are made ...
6 years, 10 months ago (2014-02-21 18:56:39 UTC) #20
sigbjorn
The CQ bit was checked by sigbjorn@opera.com
6 years, 10 months ago (2014-02-24 10:30:01 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/350001
6 years, 10 months ago (2014-02-24 10:30:09 UTC) #22
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-24 13:14:13 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel
6 years, 10 months ago (2014-02-24 13:14:13 UTC) #24
sigbjorn
The CQ bit was checked by sigbjorn@opera.com
6 years, 10 months ago (2014-02-24 14:20:44 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/350001
6 years, 10 months ago (2014-02-24 14:20:55 UTC) #26
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-24 14:21:05 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel, mac_chromium_rel
6 years, 10 months ago (2014-02-24 14:21:05 UTC) #28
sigbjorn
On 2014/02/24 13:14:13, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
6 years, 10 months ago (2014-02-24 14:21:11 UTC) #29
sigbjorn
We have 4 stable failures in tests on linux_chromium_rel. They all time out in WaitForSingleProcess, ...
6 years, 10 months ago (2014-02-26 15:26:08 UTC) #30
sigbjorn
On 2014/02/26 15:26:08, sigbjorn wrote: > Would one of you be able > to help ...
6 years, 9 months ago (2014-02-28 10:54:46 UTC) #31
sigbjorn
On 2014/02/28 10:54:46, sigbjorn wrote: > On 2014/02/26 15:26:08, sigbjorn wrote: > > Would one ...
6 years, 9 months ago (2014-03-12 10:37:16 UTC) #32
Paweł Hajdan Jr.
On 2014/03/12 10:37:16, sigbjorn wrote: > Paweł and/or M-A Ruel, any thoughts? There are no ...
6 years, 9 months ago (2014-03-12 16:03:53 UTC) #33
sigbjorn
The CQ bit was checked by sigbjorn@opera.com
6 years, 9 months ago (2014-03-13 08:02:58 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/350001
6 years, 9 months ago (2014-03-13 08:03:11 UTC) #35
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-13 10:23:47 UTC) #36
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
6 years, 9 months ago (2014-03-13 10:23:48 UTC) #37
sigbjorn
On 2014/03/12 16:03:53, Paweł Hajdan Jr. wrote: > On 2014/03/12 10:37:16, sigbjorn wrote: > > ...
6 years, 9 months ago (2014-03-13 10:27:24 UTC) #38
sigbjorn
Paweł and/or M-A Ruel, any thoughts before the logs disappear again? Are there any keywords ...
6 years, 9 months ago (2014-03-18 14:25:41 UTC) #39
M-A Ruel
https://codereview.chromium.org/145873004/diff/350001/net/test/spawned_test_server/local_test_server.cc File net/test/spawned_test_server/local_test_server.cc (right): https://codereview.chromium.org/145873004/diff/350001/net/test/spawned_test_server/local_test_server.cc#newcode129 net/test/spawned_test_server/local_test_server.cc:129: // This kills all the processes in the job ...
6 years, 9 months ago (2014-03-18 19:36:03 UTC) #40
sigbjorn
The CQ bit was checked by sigbjorn@opera.com
6 years, 9 months ago (2014-03-19 10:29:58 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/690001
6 years, 9 months ago (2014-03-19 10:30:19 UTC) #42
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-19 11:17:36 UTC) #43
commit-bot: I haz the power
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&number=284290
6 years, 9 months ago (2014-03-19 11:17:37 UTC) #44
sigbjorn
The CQ bit was checked by sigbjorn@opera.com
6 years, 9 months ago (2014-03-19 11:57:07 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/710001
6 years, 9 months ago (2014-03-19 11:57:38 UTC) #46
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-19 13:38:25 UTC) #47
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=284310
6 years, 9 months ago (2014-03-19 13:38:26 UTC) #48
sigbjorn
The CQ bit was checked by sigbjorn@opera.com
6 years, 9 months ago (2014-03-21 14:44:26 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/710001
6 years, 9 months ago (2014-03-21 14:44:40 UTC) #50
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-21 16:46:25 UTC) #51
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=285981
6 years, 9 months ago (2014-03-21 16:46:26 UTC) #52
sigbjorn
The CQ bit was checked by sigbjorn@opera.com
6 years, 9 months ago (2014-03-27 14:33:47 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/710001
6 years, 9 months ago (2014-03-27 14:34:17 UTC) #54
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-27 15:21:55 UTC) #55
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=57857
6 years, 9 months ago (2014-03-27 15:21:56 UTC) #56
sigbjorn
The CQ bit was checked by sigbjorn@opera.com
6 years, 9 months ago (2014-03-28 12:04:24 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjorn@opera.com/145873004/710001
6 years, 9 months ago (2014-03-28 12:12:15 UTC) #58
commit-bot: I haz the power
6 years, 9 months ago (2014-03-28 18:17:44 UTC) #59
Message was sent while issue was closed.
Change committed as 260201

Powered by Google App Engine
This is Rietveld 408576698