The http locking should be default in NRWT.
Created attachment 71413 [details] proposed patch
Comment on attachment 71413 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=71413&action=review Should we remove start/stop server calls from run_webkit_tests.py as well? Seems like we should only need to do this in dump_render_tree_thread, right? > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:587 > + def start_servers_to_lock(self): > + self.start_http_server() > + self.start_websocket_server() > + > + def stop_servers_to_lock(self): > + self.stop_http_server() > + self.stop_websocket_server() Can we move the acquire_http_lock and release_http_lock calls into these functions? And maybe rename them to start_servers_with_lock/stop_servers_with_lock?
Comment on attachment 71413 [details] proposed patch we also need to preserve some equivalent level of logging ... we need to be able to tell when the servers are started and stopped, at least at the debug level. It would be nice if we could preserve the print_update() status messages as well, but that's not really compatible with the one-line-status mode of running and starting the server while we're running other tests ...
Created attachment 71535 [details] proposed_patch_v2 I made the changes what Ojan ask for and put some debug message to the start/stop methods.
Comment on attachment 71535 [details] proposed_patch_v2 Code looks fine. I think it would be clearer if there were just two methods: _start_servers_with_lock and _stop_servers_with_lock and those two methods contained all the logic. So, I would merge _stop_http_lock into _stop_servers_with_lock and the logic of lines 401-404 into _start_servers_with_lock. Then the if statement would look like this: if self._current_group == "tests_to_http_lock": self._start_servers_with_lock() elif self._have_http_lock: self._stop_servers_with_lock()
Patch looks good to me as well with Ojan's suggestion rolled in. Thanks for all your work on this!
Modified patch landed in http://trac.webkit.org/changeset/70442
Comment on attachment 71535 [details] proposed_patch_v2 remove r+ from landed patch
Unfortunately it broke Windows bots on http://build.chromium.org/buildbot/waterfall.fyi/console Traceback (most recent call last): File "c:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\new-run-webkit-tests", line 38, in <module> sys.exit(run_webkit_tests.main()) File "c:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\run_webkit_tests.py", line 1714, in main return run(port_obj, options, args) File "c:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\run_webkit_tests.py", line 1408, in run num_unexpected_results = test_runner.run(result_summary) File "c:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\run_webkit_tests.py", line 766, in run self._run_tests(self._test_files_list, result_summary)) File "c:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\run_webkit_tests.py", line 646, in _run_tests result_summary) File "c:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\layout_package\dump_render_tree_thread.py", line 324, in _covered_run self._run(test_runner=None, result_summary=None) File "c:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\layout_package\dump_render_tree_thread.py", line 401, in _run self._start_servers_with_lock() File "c:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\layout_package\dump_render_tree_thread.py", line 554, in _start_servers_with_lock self._port.acquire_http_lock() File "c:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\port\base.py", line 557, in acquire_http_lock self._http_lock.wait_for_httpd_lock() File "c:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\port\http_lock.py", line 123, in wait_for_httpd_lock self._create_lock_file() File "c:\b\slave\webkit-rel-webkit-org\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\port\http_lock.py", line 108, in _create_lock_file os.O_CREAT | os.O_NONBLOCK | os.O_EXCL) AttributeError: 'module' object has no attribute 'O_NONBLOCK'
r70442 was rolled out by http://trac.webkit.org/changeset/70458. Gábor, could you file a new bug against http_lock.py, which blocks this bug?
(In reply to comment #10) > r70442 was rolled out by http://trac.webkit.org/changeset/70458. > > Gábor, could you file a new bug against http_lock.py, which blocks this bug? HTTP locking for Windows is fixed in https://bugs.webkit.org/show_bug.cgi?id=48321 I'm going to re-land r70442 and watch Windows bots.
(In reply to comment #11) > I'm going to re-land r70442 and watch Windows bots. Landed in http://trac.webkit.org/changeset/70674
(In reply to comment #12) > (In reply to comment #11) > > I'm going to re-land r70442 and watch Windows bots. > > Landed in http://trac.webkit.org/changeset/70674 Still broke the Chromium build. This time, it hung indefinitely: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win/builds/372/steps/webkit_tests/logs/stdio
Reverted r70674 for reason: Broke Chromium Windows build. Committed r70683: <http://trac.webkit.org/changeset/70683>
(In reply to comment #14) > Reverted r70674 for reason: > > Broke Chromium Windows build. > > Committed r70683: <http://trac.webkit.org/changeset/70683> Sorry for broken bot and thanks for roll-out. :(( We are going to fix it tomorrow.
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > I'm going to re-land r70442 and watch Windows bots. > > > > Landed in http://trac.webkit.org/changeset/70674 > > Still broke the Chromium build. This time, it hung indefinitely: > > http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win/builds/372/steps/webkit_tests/logs/stdio That's strange, maybe there is a stuck lock file in the Windows temp directory. We tried it on Windows and it's worked for us, so can you check whether there is any WebKitHttpd.lock or WebKit.lock file in the temp directory. Anyway I will file a bug to clear the lock file if it's not valid on Windows platform as well.
Depending bug fixed: [NRWT] Clear invalid http locks on Windows platform as well - https://bugs.webkit.org/show_bug.cgi?id=48515 Let's try to enable http locking again: http://trac.webkit.org/changeset/71340
(In reply to comment #17) > Depending bug fixed: [NRWT] Clear invalid http locks on Windows platform as well - https://bugs.webkit.org/show_bug.cgi?id=48515 > > Let's try to enable http locking again: http://trac.webkit.org/changeset/71340 It broke Chromium Windows bot again: http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win/builds/777/steps/webkit_tests/logs/stdio Rollout patch landed in http://trac.webkit.org/changeset/71342
Depending bug fixed: [NRWT] If the http lock fails we shouldn't do any locking - https://bugs.webkit.org/show_bug.cgi?id=49164 http locking is enabled by http://trac.webkit.org/changeset/71527
The last blocker https://bugs.webkit.org/show_bug.cgi?id=49187 was fixed by http://trac.webkit.org/changeset/72194 , so we can close this bug.