Bug 47072 - Implement http locking in NRWT
Summary: Implement http locking 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:
Blocks:
 
Reported: 2010-10-04 01:55 PDT by Gabor Rapcsanyi
Modified: 2010-10-12 10:13 PDT (History)
6 users (show)

See Also:


Attachments
proposed patch (13.12 KB, patch)
2010-10-04 02:07 PDT, Gabor Rapcsanyi
tony: review-
Details | Formatted Diff | Diff
proposed_patch_v2 (12.79 KB, patch)
2010-10-04 14:18 PDT, Gabor Rapcsanyi
tony: review-
Details | Formatted Diff | Diff
proposed_patch_v3 (19.66 KB, patch)
2010-10-11 05:02 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-04 01:55:47 PDT
NRWT doesn't have http lock option. It should be implemented like in ORWT.
Comment 1 Gabor Rapcsanyi 2010-10-04 02:07:07 PDT
Created attachment 69607 [details]
proposed patch
Comment 2 Tony Chang 2010-10-04 10:09:00 PDT
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.
Comment 3 Gabor Rapcsanyi 2010-10-04 14:18:44 PDT
Created attachment 69679 [details]
proposed_patch_v2
Comment 4 Gabor Rapcsanyi 2010-10-04 14:19:43 PDT
(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.
Comment 5 Dirk Pranke 2010-10-04 14:27:08 PDT
(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.
Comment 6 Dirk Pranke 2010-10-04 14:40:50 PDT
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.
Comment 7 Tony Chang 2010-10-04 15:27:56 PDT
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.
Comment 8 Gabor Rapcsanyi 2010-10-11 05:02:34 PDT
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.
Comment 9 Tony Chang 2010-10-11 14:49:48 PDT
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))
Comment 10 Dirk Pranke 2010-10-11 15:09:35 PDT
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.
Comment 11 Tony Chang 2010-10-11 15:50:15 PDT
(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.
Comment 12 Dirk Pranke 2010-10-11 16:12:44 PDT
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).
Comment 13 Eric Seidel (no email) 2010-10-11 21:20:46 PDT
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. :)
Comment 14 Gabor Rapcsanyi 2010-10-12 07:16:37 PDT
I've made the changes what you suggested.
The patch has landed at https://trac.webkit.org/changeset/69578
Comment 15 Gabor Rapcsanyi 2010-10-12 07:23:25 PDT
Comment on attachment 70429 [details]
proposed_patch_v3

Modified patch landed.
Comment 16 Gabor Rapcsanyi 2010-10-12 07:46:04 PDT
(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.
Comment 17 Tony Chang 2010-10-12 09:27:00 PDT
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!
Comment 18 Tony Chang 2010-10-12 10:13:12 PDT
(In reply to comment #17)
> 
> We're going to revert, sorry!

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