Bug 48053 – Make http locking default in NRWT

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Gabor Rapcsanyi]

The http locking should be default in NRWT.

[Gabor Rapcsanyi]

Created attachment 71413 [details]
proposed patch

[Ojan Vafai]

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?

[Dirk Pranke]

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 ...

[Gabor Rapcsanyi]

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.

[Ojan Vafai]

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()

[Dirk Pranke]

Patch looks good to me as well with Ojan's suggestion rolled in. Thanks for all your work on this!

[Csaba Osztrogonác]

Modified patch landed in http://trac.webkit.org/changeset/70442

[Csaba Osztrogonác]

Comment on attachment 71535 [details]
proposed_patch_v2

remove r+ from landed patch

[Csaba Osztrogonác]

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'

[Csaba Osztrogonác]

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?

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

(In reply to comment #11)
> I'm going to re-land r70442 and watch Windows bots.

Landed in http://trac.webkit.org/changeset/70674

[Dimitri Glazkov (Google)]

(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

[Dimitri Glazkov (Google)]

Reverted r70674 for reason:

Broke Chromium Windows build.

Committed r70683: <http://trac.webkit.org/changeset/70683>

[Csaba Osztrogonác]

(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.

[Gabor Rapcsanyi]

(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.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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.