Bug 50712 – make starting the websocket server more reliable on windows

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

make starting the websocket server more reliable on windows

[Tony Chang]

Created attachment 75958 [details]
Patch

[Tony Chang]

On the chromium win dbg(2) bot, we're seeing:
  File "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\layout_package\dump_render_tree_thread.py", line 414, in _run
    self._start_servers_with_lock()
  File "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\layout_package\dump_render_tree_thread.py", line 550, in _start_servers_with_lock
    self._port.start_websocket_server()
  File "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\port\base.py", line 548, in start_websocket_server
    self._websocket_server.start()
  File "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\port\websocket_server.py", line 153, in start
    self.remove_log_files(self._output_dir, log_prefix)
  File "e:\b\build\slave\Webkit_Win__dbg__2_\build\src\third_party\WebKit\WebKitTools\Scripts\webkitpy\layout_tests\port\http_server_base.py", line 81, in remove_log_files
    os.remove(full_path)
WindowsError: [Error 32] The process cannot access the file because it is being used by another process: 'e:\\b\\build\\slave\\Webkit_Win__dbg__2_\\build\\src\\webkit\\Debug\\..\\../../layout-test-results\\pywebsocket.ws.log-08Dec2010-020727-out.txt'
No handlers could be found for logger "webkitpy.layout_tests.layout_package.dump_render_tree_thread"

[Eric Seidel (no email)]

I think yuzo filed a separate bug about this.

[Eric Seidel (no email)]

Why is the retry needed?  What's the race here?

[Tony Chang]

(In reply to comment #4)
> Why is the retry needed?  What's the race here?

I think on Windows, it's common for the OS to hold on to files after making system calls to delete/close the file.  We do a similar thing for chromium unit tests:
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/base/test/test_file_util_win.cc&q=diefiledie&exact_package=chromium&l=22

[Tony Chang]

Oh, Yuta filed the bug.  It's https://bugs.webkit.org/show_bug.cgi?id=50670

[Tony Chang]

Created attachment 75972 [details]
Patch

[Tony Chang]

(In reply to comment #7)
> Created an attachment (id=75972) [details]
> Patch

Per discussion on IRC, I did the following:
- Moved the method into filesystem.py
- Added links to other known cases of the problem
- Raise an error if the retries all fail

[Eric Seidel (no email)]

Comment on attachment 75972 [details]
Patch

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

> WebKitTools/Scripts/webkitpy/common/system/filesystem.py:115
> +        try:
> +            WindowsError
> +        except NameError:
> +            WindowsError = FileSystem._WindowsError

Does this work on non-windows python?  Should we even be executing this code for non-windows users?

> WebKitTools/Scripts/webkitpy/common/system/filesystem.py:126
> +        raise OSError("failed to delete %s" % path)

Seems we should just re-raise the original WindowsError in that case, no?

> WebKitTools/Scripts/webkitpy/common/system/filesystem_unittest.py:156
> +        FileSystemTest._remove_failures = 2

Can this just be a local?

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_server_base.py:83
> +                filesystem.remove(full_path)

This can't work?  remove is an instance method.

[Tony Chang]

Created attachment 75982 [details]
Patch

[Tony Chang]

(In reply to comment #9)
> Does this work on non-windows python?  Should we even be executing this code for non-windows users?

Yes, it tested it on Linux.  I don't know.  I could either check platforms and return early or make the code handle non-Windows platforms.  Let me know which way you prefer (I thought this way was cleaner).

> > WebKitTools/Scripts/webkitpy/common/system/filesystem.py:126
> > +        raise OSError("failed to delete %s" % path)
> 
> Seems we should just re-raise the original WindowsError in that case, no?

Fixed.

> > WebKitTools/Scripts/webkitpy/common/system/filesystem_unittest.py:156
> > +        FileSystemTest._remove_failures = 2
> 
> Can this just be a local?

Unfortunately, no. See http://www.python.org/dev/peps/pep-3104/.  If you want, I can do the Namespace trick shown in the example instead.

> > WebKitTools/Scripts/webkitpy/layout_tests/port/http_server_base.py:83
> > +                filesystem.remove(full_path)
> 
> This can't work?  remove is an instance method.

Sorry, fixed.

[Eric Seidel (no email)]

Comment on attachment 75982 [details]
Patch

Seems OK.  Thanks

[Tony Chang]

*** Bug 50670 has been marked as a duplicate of this bug. ***

[Tony Chang]

Committed r73563: <http://trac.webkit.org/changeset/73563>

[WebKit Review Bot]

http://trac.webkit.org/changeset/73563 might have broken GTK Linux 32-bit Release and GTK Linux 64-bit Debug
The following tests are not passing:
fast/css/input-search-padding.html
fast/css/text-input-with-webkit-border-radius.html
fast/forms/box-shadow-override.html
fast/forms/control-restrict-line-height.html
fast/forms/input-appearance-height.html
fast/forms/placeholder-pseudo-style.html
fast/forms/placeholder-set-value.html
fast/forms/search-cancel-button-style-sharing.html
fast/forms/search-placeholder-value-changed.html
fast/forms/search-rtl.html
fast/forms/search-styled.html
fast/forms/search-transformed.html
fast/forms/search-vertical-alignment.html
fast/forms/search-zoomed.html
fast/forms/searchfield-heights.html