Bug 48053 - Make http locking default in NRWT
Summary: Make http locking default in NRWT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 48248 48321 48515 49164 49187
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-21 02:42 PDT by Gabor Rapcsanyi
Modified: 2010-11-17 07:58 PST (History)
7 users (show)

See Also:


Attachments
proposed patch (5.32 KB, patch)
2010-10-21 03:10 PDT, Gabor Rapcsanyi
ojan: review-
Details | Formatted Diff | Diff
proposed_patch_v2 (5.25 KB, patch)
2010-10-22 01:21 PDT, Gabor Rapcsanyi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabor Rapcsanyi 2010-10-21 02:42:21 PDT
The http locking should be default in NRWT.
Comment 1 Gabor Rapcsanyi 2010-10-21 03:10:46 PDT
Created attachment 71413 [details]
proposed patch
Comment 2 Ojan Vafai 2010-10-21 09:01:44 PDT
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 3 Dirk Pranke 2010-10-21 18:34:07 PDT
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 ...
Comment 4 Gabor Rapcsanyi 2010-10-22 01:21:19 PDT
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 5 Ojan Vafai 2010-10-22 08:13:04 PDT
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()
Comment 6 Dirk Pranke 2010-10-22 12:04:48 PDT
Patch looks good to me as well with Ojan's suggestion rolled in. Thanks for all your work on this!
Comment 7 Csaba Osztrogonác 2010-10-25 00:57:45 PDT
Modified patch landed in http://trac.webkit.org/changeset/70442
Comment 8 Csaba Osztrogonác 2010-10-25 00:58:08 PDT
Comment on attachment 71535 [details]
proposed_patch_v2

remove r+ from landed patch
Comment 9 Csaba Osztrogonác 2010-10-25 09:11:30 PDT
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'
Comment 10 Csaba Osztrogonác 2010-10-25 09:14:53 PDT
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?
Comment 11 Csaba Osztrogonác 2010-10-27 10:54:29 PDT
(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.
Comment 12 Csaba Osztrogonác 2010-10-27 11:03:46 PDT
(In reply to comment #11)
> I'm going to re-land r70442 and watch Windows bots.

Landed in http://trac.webkit.org/changeset/70674
Comment 13 Dimitri Glazkov (Google) 2010-10-27 11:57:51 PDT
(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
Comment 14 Dimitri Glazkov (Google) 2010-10-27 11:58:30 PDT
Reverted r70674 for reason:

Broke Chromium Windows build.

Committed r70683: <http://trac.webkit.org/changeset/70683>
Comment 15 Csaba Osztrogonác 2010-10-27 12:22:52 PDT
(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.
Comment 16 Gabor Rapcsanyi 2010-10-28 05:37:33 PDT
(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.
Comment 17 Csaba Osztrogonác 2010-11-04 09:48:51 PDT
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
Comment 18 Csaba Osztrogonác 2010-11-04 10:16:29 PDT
(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
Comment 19 Csaba Osztrogonác 2010-11-08 09:18:14 PST
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
Comment 20 Csaba Osztrogonác 2010-11-17 07:58:45 PST
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.