Bug 47072 – Implement http locking in NRWT

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Gabor Rapcsanyi]

NRWT doesn't have http lock option. It should be implemented like in ORWT.

[Gabor Rapcsanyi]

Created attachment 69607 [details]
proposed patch

[Tony Chang]

Comment on attachment 69607 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69607&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:308
> +        return max(self._stop_time - self._start_time - \
> +                   self._http_lock_wait_time(), 0.0)

Nit: I don't think you need the \ here.  It should be implicit since you're in ().

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:350
> +        self._canceled = True

Instead of setting _canceled directly, can you just call WatchableThread.cancel(self)?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:362
> +        if self._http_lock_wait_end == 0:
> +            return time.time() - self._http_lock_wait_begin
> +        return self._http_lock_wait_end - self._http_lock_wait_begin

If wait_for_httpd has not been set, won't we return time.time()?  That is, the default value for _http_lock_wait_end and _http_lock_wait_begin are 0.  This means that get_total_time() will return self._stop_time - self._start_time - time.time(), which seems incorrect.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:402
> +                if self._options.wait_for_httpd and \
> +                  self._current_dir == "tests_to_http_lock" and \
> +                  not self._http_server_running:

Nit: () around conditions is preferred to using \.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:34
> +import os
> +import sys
> +import time
> +import fcntl
> +import signal
> +import glob

Nit: please sort these alphabetically

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:42
> +        self._lock_path = "/tmp"
> +        self._lock_file_prefix = "WebKitHttpd.lock."
> +        self._lock_file_path_prefix = os.path.join(self._lock_path,

Would it be better to use tempfile.gettempdir() instead of /tmp?  This would allows people to override the location by setting TEMPDIR.  On the other hand, apache uses /tmp/WebKit for some fimes.  If we don't use tempfile, we should at put the files in /tmp/WebKit/.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:45
> +        self._my_lock_file_name = ""

Nit: Drop the "my" in the variable name.  Maybe process_lock_file_name would be better?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:53
> +        if not os.path.exists(lock_file_name):
> +            return -1

Do we need this check?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:86
> +            if not current_pid or \
> +              not (sys.platform in ('darwin', 'linux2') and \
> +              self._check_pid(int(current_pid))):

Nit: use () for line continuation instead of \.  Also, you can do "sys.platform not in ('darwin', 'linux2')", which is a little easier to read.

[Gabor Rapcsanyi]

Created attachment 69679 [details]
proposed_patch_v2

[Gabor Rapcsanyi]

(In reply to comment #2)
> (From update of attachment 69607 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69607&action=review
>
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:308
> > +        return max(self._stop_time - self._start_time - \
> > +                   self._http_lock_wait_time(), 0.0)
>
> Nit: I don't think you need the \ here.  It should be implicit since you're in ().
>

Fixed.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:350
> > +        self._canceled = True
>
> Instead of setting _canceled directly, can you just call WatchableThread.cancel(self)?
>

Fixed.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:362
> > +        if self._http_lock_wait_end == 0:
> > +            return time.time() - self._http_lock_wait_begin
> > +        return self._http_lock_wait_end - self._http_lock_wait_begin
>
> If wait_for_httpd has not been set, won't we return time.time()?  That is, the default value for _http_lock_wait_end and _http_lock_wait_begin are 0.  This means that get_total_time() will return self._stop_time - self._start_time - time.time(), which seems incorrect.
>

You're right, I just missed that case.
Fixed.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:402
> > +                if self._options.wait_for_httpd and \
> > +                  self._current_dir == "tests_to_http_lock" and \
> > +                  not self._http_server_running:
>
> Nit: () around conditions is preferred to using \.
>

Fixed.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:34
> > +import os
> > +import sys
> > +import time
> > +import fcntl
> > +import signal
> > +import glob
>
> Nit: please sort these alphabetically
>

FIxed.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:42
> > +        self._lock_path = "/tmp"
> > +        self._lock_file_prefix = "WebKitHttpd.lock."
> > +        self._lock_file_path_prefix = os.path.join(self._lock_path,
>
> Would it be better to use tempfile.gettempdir() instead of /tmp?  This would allows people to override the location by setting TEMPDIR.  On the other hand, apache uses /tmp/WebKit for some fimes.  If we don't use tempfile, we should at put the files in /tmp/WebKit/.
>

Fixed to tempfile.gettempdir(). ORWT is use /tmp and I want NRWT to be able to work together with ORWT.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:45
> > +        self._my_lock_file_name = ""
>
> Nit: Drop the "my" in the variable name.  Maybe process_lock_file_name would be better?
>

Renamed.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:53
> > +        if not os.path.exists(lock_file_name):
> > +            return -1
>
> Do we need this check?
>

Not now, It just stayed in the code.
Fixed.

> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:86
> > +            if not current_pid or \
> > +              not (sys.platform in ('darwin', 'linux2') and \
> > +              self._check_pid(int(current_pid))):
>
> Nit: use () for line continuation instead of \.  Also, you can do "sys.platform not in ('darwin', 'linux2')", which is a little easier to read.

Now it's more readable.

Furthermore, I bring _clean_up_http_lock() in dump_render_tree_thread.py earlier because it will run another tests not just https.
So after the http tests it will clean up the lock.

[Dirk Pranke]

(In reply to comment #1)
> Created an attachment (id=69607) [details]
> proposed patch

the HttpLock code looks generally pretty good, but doesn't actually have anything to do with HTTP; it's a generic file locking mechanism.  Can we rename the file? 

More importantly, the code is conceptually a platform-specific (or port-specific) abstraction. It would be nice to move this file into the port/ directory and hang it off of the port object (like we do for the http_server code). That way ports don't need to implement this if it isn't necessary, or can implement something else as appropriate.

Lastly, to address Tony's suggestion about tempfile directory locations, I'd suggest taking the directory as an argument in the constructor, and that way the port files can pass in the directory they'd like to use.

[Dirk Pranke]

Comment on attachment 69679 [details]
proposed_patch_v2

I realize that (a) this code is largely trying to mirror the algorithm in ORWT and (b) it's not usually webkit style to comment the code very heavily, but it would be good to have some comments here documenting the algorithm you're trying to follow. Also, this code needs unit tests.

[Tony Chang]

Comment on attachment 69679 [details]
proposed_patch_v2

View in context: https://bugs.webkit.org/attachment.cgi?id=69679&action=review

r- mainly because there are no tests

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1609
> +        optparse.make_option("--no-wait-for-httpd", action="store_false",
> +            dest="wait_for_httpd", help="Do not use http locks."),

I realize you're just trying to match ORWT, but do you think we could get by without --no-wait-for-httpd ?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:413
> +                    elif self._http_server_running:
> +                        self._clean_up_http_lock()

Why was this added?  Doesn't this say other threads should stop the http server?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:560
> +    def _clean_up_http_lock(self):

Do we need to call _clean_up_http_lock when the "test_to_http_lock" are done running?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:38
> +class HttpLock(object):
> +

I agree with Dirk that this file needs unittests and should include a class docstring.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:98
> +                self._process_lock_file_name = self._lock_file_path_prefix + \
> +                                          str(self._next_lock_number())

Nit: () instead of \.  Also the indention looks a bit off.

[Gabor Rapcsanyi]

Created attachment 70429 [details]
proposed_patch_v3

I think the HttpLock name is better than a general name like ThreadLock, TestLock, ServerLock or etc. because we use this lock primarily for http tests locking. But if you have a better suggestion I can use that.
I put the HttpLock class to the port directory and now the drt_thread is reach it from the port object.
Also I put some comments into HttpLock and add a unittest for it.

[Tony Chang]

Comment on attachment 70429 [details]
proposed_patch_v3

View in context: https://bugs.webkit.org/attachment.cgi?id=70429&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:48
>  import apache_http_server
>  import test_files
> +import http_lock
>  import http_server
>  import websocket_server

Nit: sort these alphabetically

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:113
> +                self._process_lock_file_name = (self._lock_file_path_prefix +
> +                                          str(self._next_lock_number()))

Nit: The indention looks weird here.  Either line it up with the ( in the previous line or just indent 4 spaces.

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:53
> +        if first != second:
> +            self.clean_all_lockfile()

This seems ok, although you could also use try/finally in each test.  I don't really care either way.

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:83
> +        lock_file_list = [
> +            self.lock_file_path_prefix + "00",
> +            self.lock_file_path_prefix + "9",
> +            self.lock_file_path_prefix + "001",
> +            self.lock_file_path_prefix + "021",
> +        ]

Nit: A tuple is a bit faster than using a list.  It also prevents unintended modification.

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:85
> +        expected_number_list = ["0", "9", "1", "21"]

Nit: tuple, and why not drop the "" and just make these ints (so you and avoid the cast below).

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:89
> +        for i in range(len(lock_file_list)):
> +            self.assertEqual(self.http_lock_obj._extract_lock_number(lock_file_list[i]),
> +                             int(expected_number_list[i]))

Nit: You could re-write this as:
for lock_file, expected in zip(lock_file_list, expected_number_list):
    self.assertEqual(self.http_lock_obj._extract_lock_number(lock_file), int(expected_number_list))

[Dirk Pranke]

Can we get rid of --wait-for-httpd and just always wait for the lock? Only risk is if something goes south and the lock files aren't cleaned up, but I think we can deal with that if it actually starts to happen.

[Tony Chang]

(In reply to comment #10)
> Can we get rid of --wait-for-httpd and just always wait for the lock? Only risk is if something goes south and the lock files aren't cleaned up, but I think we can deal with that if it actually starts to happen.

This sounds fine to me.  It could be a follow up patch.

It would be nice if hitting ctrl+c in the middle of http tests running would still cause the lock file to be cleaned up.  The only remaining case would be if you lost power while running http tests.  You could handle this by verifying that the pid in the file matches with an existing process, but it doesn't seem to be that big of a deal.

[Dirk Pranke]

Comment on attachment 70429 [details]
proposed_patch_v3

View in context: https://bugs.webkit.org/attachment.cgi?id=70429&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:407
> +

Please add the print_update("Starting ...") messages here that we have in run_webkit_tests. Frankly, it would be good to combine these two blocks of code to avoid repetition (same for the the _stop_http_lock() code).

[Eric Seidel (no email)]

I support always locking.

Although ideally we'll change DRT to re-write the port numbers and then we won't need any of this.  It shouldn't be that hard since the WebKit API allows the loading delegate to intercept every network request and mutate it (at least on mac), we just need to do it one of these days. :)

[Gabor Rapcsanyi]

I've made the changes what you suggested.
The patch has landed at https://trac.webkit.org/changeset/69578

[Gabor Rapcsanyi]

Comment on attachment 70429 [details]
proposed_patch_v3

Modified patch landed.

[Gabor Rapcsanyi]

(In reply to comment #12)
> (From update of attachment 70429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70429&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:407
> > +
> 
> Please add the print_update("Starting ...") messages here that we have in run_webkit_tests. Frankly, it would be good to combine these two blocks of code to avoid repetition (same for the the _stop_http_lock() code).

I cannot use the printer object in dump_render_tree_thread.py.
In another bug I planning to make two method in base.py to start and stop the http and websocket servers together.

[Tony Chang]

This failed on Windows.  Here's the stack (http://build.webkit.org/builders/Chromium%20Win%20Release%20(Tests)/builds/3999/steps/layout-test/logs/stdio):


Traceback (most recent call last):
  File "./WebKitTools/Scripts/new-run-webkit-tests", line 38, in <module>
    sys.exit(run_webkit_tests.main())
  File "D:\google-windows-2\chromium-win-release-tests\build\WebKitTools\Scripts\webkitpy\layout_tests\run_webkit_tests.py", line 1730, in main
    port_obj = port.get(options.platform, options)
  File "D:\google-windows-2\chromium-win-release-tests\build\WebKitTools\Scripts\webkitpy\layout_tests\port\factory.py", line 49, in get
    return _get_kwargs(**kwargs)
  File "D:\google-windows-2\chromium-win-release-tests\build\WebKitTools\Scripts\webkitpy\layout_tests\port\factory.py", line 101, in _get_kwargs
    import chromium_win
  File "D:\google-windows-2\chromium-win-release-tests\build\WebKitTools\Scripts\webkitpy\layout_tests\port\chromium_win.py", line 36, in <module>
    import chromium
  File "D:\google-windows-2\chromium-win-release-tests\build\WebKitTools\Scripts\webkitpy\layout_tests\port\chromium.py", line 46, in <module>
    import base
  File "D:\google-windows-2\chromium-win-release-tests\build\WebKitTools\Scripts\webkitpy\layout_tests\port\base.py", line 45, in <module>
    import http_lock
  File "D:\google-windows-2\chromium-win-release-tests\build\WebKitTools\Scripts\webkitpy\layout_tests\port\http_lock.py", line 31, in <module>
    import fcntl
ImportError: No module named fcntl



We're going to revert, sorry!

[Tony Chang]

(In reply to comment #17)
> 
> We're going to revert, sorry!

Nevermind, fixed it in http://trac.webkit.org/changeset/69585 .