Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8633004:
Stop WebSocker server processes certainly (Closed)
|
Created:
9 years, 1 month ago by Takashi Toyoshima Modified:
9 years, 1 month ago CC:
chromium-reviews, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionKill orphaned WebSocker server processes before launching new one in POSIX,
or use JobObject to kill child servers on quitting in Win32.
Sometime, unexpected existing WebSocket servers cause continuous test failures
because they prevent to launch new WebSocket server using the same port.
This change kills orphaned WebSocket servers before launching new one in POSIX,
or use JobObject to register the server processes must be killed after parent process quit.
This mechanism works fine in net/test/test_server_{posix|win}.cc for HTTP servers.
BUG=91058
TEST=third_party/WebKit/Tools/Script/new-run-webkit-websocketserver --server=start; ui_tests --gtest_filter="*WorkerWebSocket*" --gtest_also_run_disabled_tests (ui_tests must run by killing existing WebSocket server)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111473
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Add Win32 impl. #
Total comments: 6
Patch Set 4 : revised #
Total comments: 14
Patch Set 5 : revise #Patch Set 6 : again #
Total comments: 4
Patch Set 7 : fix and use SetJobObjectAsKillOnJobClose() #Patch Set 8 : rebase #Messages
Total messages: 20 (0 generated)
Hi chrome/test/OWNERS guys, I'd be happy if one of you could review this change. This change is required to re-enable our disabled WebSocket tests. And also it needed for launching WebSocket Pepper API for M17. Thanks,
How about Windows?
> How about Windows? If the host can hold its pid file, new-run-webkit-websocketserver can kill the previous orphaned servers. I think it's enough for Windows. On the other hands, we store its pid file under /tmp/ in posix environment. It might disappear on unexpected system reboot or so. I think this is the posix environment only issue because Windows never clean up its temporary folder at its reboot sequence. In the case of net/test/test_server*.c, only posix version supply killing orphaned servers procedure.
On 2011/11/22 06:43:09, toyoshim wrote: > If the host can hold its pid file, new-run-webkit-websocketserver can kill the > previous > orphaned servers. I think it's enough for Windows. If I recall correctly, this was also happened on Windows too, even after pidfile fix was applied in WebKit.
websocket.pid file is generated in a temporary directory which is removed every time the server is stopped (see ScopedTempDir). So this is not a POSIX-only problem.
OK, make sense. I must consider on win32.
I look into querying command lines on win32. It's not impossible but a little tricky. I guess there is no usual way. FYI: http://msdn.microsoft.com/en-us/magazine/bb985842.aspx I think there are two way to resolve this issue. One is that we use some fixed directory to hold its pid file. The other way, we can use pipe to notice parents states to child. When parent process die, the pipe disappears and child could detect parent's death via errors or EOF on pipe. But currently, new-run-webkit-websocketserver daemonize pywebsocket. So we must change pywebsocket, new-run-webkit-websocketserver, and so on. I think this issue happens in very rare case. So, we can just enable our tests with this change. And after that, we can improve them. Disabling all worker tests on WebSocket for long time must be bad from the viewpoint of test coverage. Or we have a choice to enable them in only non-win32 ports.
Have you tried a different approach first: starting the server in a process group / job object and then killing it on destruction, after stopping gracefully? If in doubt, see how net/test/test_server works, I don't remember similar problems with it. I _might_ have commented on this WebSocket code before.
Thanks, Paweł! The way you suggest looks very nice. I'll apply it to my change for win32. I'll upload new change soon. Thanks!
I revised my change to support Win32 JobObject. Could you look into it?
That's good direction. Some implementation comments. Bonus points for moving the code out of ui_test_utils (up to you, feel free to do in a separate CL or not at all). http://codereview.chromium.org/8633004/diff/2003/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.cc (right): http://codereview.chromium.org/8633004/diff/2003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:9: #if defined(OS_WIN) nit: I think we include C headers before C++ ones. http://codereview.chromium.org/8633004/diff/2003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:777: if (!base::KillProcesses("python", -1, &filter)) I think you could use process groups on POSIX in a way similar to the job object on Windows: options.new_process_group = true; And on stop: base::KillProcessGroup(process_handle); http://codereview.chromium.org/8633004/diff/2003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:790: if (0 == SetInformationJobObject(job_handle_.Get(), I think this is the third occurrence of this code - could you extract it to a common function?
http://codereview.chromium.org/8633004/diff/2003/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.cc (right): http://codereview.chromium.org/8633004/diff/2003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:9: #if defined(OS_WIN) On 2011/11/22 16:38:37, Paweł Hajdan Jr. wrote: > nit: I think we include C headers before C++ ones. Oh, sorry and thanks. http://codereview.chromium.org/8633004/diff/2003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:777: if (!base::KillProcesses("python", -1, &filter)) On 2011/11/22 16:38:37, Paweł Hajdan Jr. wrote: > I think you could use process groups on POSIX in a way similar to the job object > on Windows: > > options.new_process_group = true; > > And on stop: > > base::KillProcessGroup(process_handle); Done. http://codereview.chromium.org/8633004/diff/2003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:790: if (0 == SetInformationJobObject(job_handle_.Get(), OK, I found the same code sequence in five source code as follows. src/net/test/test_server_win.cc, src/chrome/test/ui/ui_test_suite.cc, src/chrome/common/chrome_content_client.cc, src/chrome/test/base/layout_test_http_server.cc, and here. I'll extract it to a common function. (But, sorry I'll do it tomorrow)
I make a separate change because this refactoring require more CC members. http://codereview.chromium.org/8667006/ So, Could you allow this change land?
Mostly lgtm except for process_handle_ issue. http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.cc (right): http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:740: OrphanedWebSocketServerFilter(std::string script_name, std::string port) Arguments should be typed as const std::string&, because std::string argument yields a copy. http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:754: // Check if an argument sequence like "--port 8880" exist. nit: exist -> exists. http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:780: process_handle_(base::kNullProcessHandle) { Don't you have to guard process_handle_ with #if defined(OS_POSIX)? http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:809: if (!base::KillProcesses("python", -1, &filter)) nit: Add curly braces if the content of a block spans two or more lines. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:824: if (0 == SetInformationJobObject(job_handle_.Get(), nit: Prefer (x == 0) over (0 == x). Maybe it's better to take the result of SetInformationJobObject() to a named variable. See http://dev.chromium.org/developers/coding-style#TOC-Code-formatting http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:836: if (!base::LaunchProcess(*cmd_line.get(), options, &process_handle_)) { Same here for #if defined(OS_POSIX) for process_handle_. http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:893: base::KillProcessGroup(process_handle_); Ditto.
Thank you for review, and sorry for too many careless misses. I upload revised one for the next review. Also, I push it to trybot. http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... File chrome/test/base/ui_test_utils.cc (right): http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:740: OrphanedWebSocketServerFilter(std::string script_name, std::string port) On 2011/11/23 06:06:09, Yuta Kitamura wrote: > Arguments should be typed as const std::string&, because std::string argument > yields a copy. Done. http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:754: // Check if an argument sequence like "--port 8880" exist. On 2011/11/23 06:06:09, Yuta Kitamura wrote: > nit: exist -> exists. Done. http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:780: process_handle_(base::kNullProcessHandle) { Oh, right! Nice catch. http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:809: if (!base::KillProcesses("python", -1, &filter)) On 2011/11/23 06:06:09, Yuta Kitamura wrote: > nit: Add curly braces if the content of a block spans two or more lines. > > See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals Done. http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:824: if (0 == SetInformationJobObject(job_handle_.Get(), For now, other four place use this exactly same idiom. I have another change to extract these common code into a function. I'll fix it in the change. You can follow the change in http://codereview.chromium.org/8667006/ http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:836: if (!base::LaunchProcess(*cmd_line.get(), options, &process_handle_)) { On 2011/11/23 06:06:09, Yuta Kitamura wrote: > Same here for #if defined(OS_POSIX) for process_handle_. Done. http://codereview.chromium.org/8633004/diff/6003/chrome/test/base/ui_test_uti... chrome/test/base/ui_test_utils.cc:893: base::KillProcessGroup(process_handle_); On 2011/11/23 06:06:09, Yuta Kitamura wrote: > Ditto. Done.
OK, I'll land 8667006 first, then land this using 8667006.
LGTM with comments. http://codereview.chromium.org/8633004/diff/15001/chrome/test/base/ui_test_ut... File chrome/test/base/ui_test_utils.cc (right): http://codereview.chromium.org/8633004/diff/15001/chrome/test/base/ui_test_ut... chrome/test/base/ui_test_utils.cc:738: class OrphanedWebSocketServerFilter : public base::ProcessFilter { Could you remove it? It shouldn't be necessary now. http://codereview.chromium.org/8633004/diff/15001/chrome/test/base/ui_test_ut... chrome/test/base/ui_test_utils.cc:898: #if defined(PS_POSIX) Ooops, this should be OS_POSIX.
http://codereview.chromium.org/8633004/diff/15001/chrome/test/base/ui_test_ut... File chrome/test/base/ui_test_utils.cc (right): http://codereview.chromium.org/8633004/diff/15001/chrome/test/base/ui_test_ut... chrome/test/base/ui_test_utils.cc:738: class OrphanedWebSocketServerFilter : public base::ProcessFilter { OK, I remove OrphanedWebSocketServerFilter related code totally. http://codereview.chromium.org/8633004/diff/15001/chrome/test/base/ui_test_ut... chrome/test/base/ui_test_utils.cc:898: #if defined(PS_POSIX) On 2011/11/23 17:50:43, Paweł Hajdan Jr. wrote: > Ooops, this should be OS_POSIX. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/8633004/14005
Change committed as 111473 |
