[webkit-reviews] review denied: [Bug 27490] Web Sockets Test Infrastructure Part 1/3: Server : [Attachment 40705] add mod_pywebsocket to test Web Sockets
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Oct 14 22:47:52 PDT 2009
David Levin <levin at chromium.org> has denied Yuzo Fujishima <yuzo at google.com>'s
request for review:
Bug 27490: Web Sockets Test Infrastructure Part 1/3: Server
https://bugs.webkit.org/show_bug.cgi?id=27490
Attachment 40705: add mod_pywebsocket to test Web Sockets
https://bugs.webkit.org/attachment.cgi?id=40705&action=review
------- Additional Comments from David Levin <levin at chromium.org>
General notes:
Ideally the copyright would be of this format:
Copyright (C) 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
Copyright (C) 2009 Somebody <email>
Also make sure that variable name and ideally file names too use full words
(not abbreviations).
I think web_socket_shake_hands is misnamed. It isn't a handler that is suppose
to do any handshaking. It seems it is only there to allow handlers to perform
custom checks before the handshake is sent from the server. I haven't thought
a lot about a better name... Here's some brief name brainstorming:
web_socket_handshake_check, web_socket_allow_handshake, etc.
Here's a quote from the websocket spec:
The first field consists of three tokens separated by space
characters (byte 0x20). The middle token is the path being opened.
If the server supports multiple paths, then the server should echo
the value of this field in the initial handshake, as part of the URL
given on the |WebSocket-Location| line (after the appropriate scheme
and host).
If the first field does not have three tokens, the server should
abort the connection as it probably represents an errorneous client.
Where is this done?
> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> +2009-10-01 Yuzo Fujishima <yuzo at google.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Add mod_pywebsocket to test Web Sockets.
> + http://code.google.com/p/pywebsocket/
> + https://bugs.webkit.org/show_bug.cgi?id=27490
> +
> + * pywebsocket/COPYING: Added.
Usually you'd say something about the files where they are listed. This
changelog is a bit sparse.
It would be nice if it referenced the protocol spec since that is what is being
implemented: http://tools.ietf.org/html/draft-hixie-thewebsocketprotocol
> diff --git a/WebKitTools/pywebsocket/COPYING
b/WebKitTools/pywebsocket/COPYING
> +++ b/WebKitTools/pywebsocket/COPYING
Is it really needed to check in this file?
> diff --git a/WebKitTools/pywebsocket/README b/WebKitTools/pywebsocket/README
> new file mode 100644
> index 0000000..1f9f05f
> --- /dev/null
> +++ b/WebKitTools/pywebsocket/README
> @@ -0,0 +1,6 @@
> +Install this package by:
> +$ python setup.py build
> +$ sudo python setup.py install
> +
> +Then read document by:
> +$ pydoc mod_pywebsocket
Does this need to be installed by every webkit developer before running layout
tests?
There needs to be another way to handle this (and then delete this file :) ).
> diff --git a/WebKitTools/pywebsocket/example/echo_client.py
b/WebKitTools/pywebsocket/example/echo_client.py
> +class EchoClient(object):
> + def _handshake(self):
...
> + for expected_char in (
> + 'HTTP/1.1 101 Web Socket Protocol Handshake\r\n'
> + 'Upgrade: WebSocket\r\n'
> + 'Connection: Upgrade\r\n'):
Why not put this in a string constant?
Also you repeat several strings here and in the "self._socket.send" above. If
these will always be the same, it may be better to have only one instance of
them for maintainence reasons.
> + def _skip_headers(self):
> + terminator = '\r\n\r\n'
> + pos = 0
> + while pos < len(terminator):
> + received = self._socket.recv(1)[0]
> + if received == terminator[pos]:
> + pos += 1
> + else:
> + pos = 0
This isn't correct. It should be
if received == terminator[pos]:
pos += 1
elif received == terminator[0]:
pos = 1
else:
pos = 0
> + def _format_host_header(self):
> + host = 'Host: ' + self._options.server_host
> + if ((not self._options.use_tls and
> + self._options.server_port != _DEFAULT_PORT) or
> + (self._options.use_tls and
> + self._options.server_port != _DEFAULT_SECURE_PORT)):
Please put the "and" "or" at the beginning of lines instead of the end
following the WebKit style guide of putting &&/|| at the beginning of lines
when there are continuations.
Also, this would be easier to read if there weren't as many wrapped lines. I
would suggest not warping after the "and"s. WebKit doesn't have an 80 column
limit.
> + (options, _) = parser.parse_args()
"_" is a valid variable name but non-descript and looks like a magic variable
from perl ($_).
Why not just put "arguments" or "unused"?
> + # Default port number depends on whether TLS is used.
> + if options.server_port == _UNDEFINED_PORT:
> + if options.use_tls:
> + options.server_port = _DEFAULT_SECURE_PORT
> + else:
> + options.server_port = _DEFAULT_PORT
It would be nice to make this a function "def get_default_port(use_tls):" which
would be used here and in the "if" in _format_host_header.
> +# vi:sts=4 sw=4 et
No other files in WebKit have this "vi" formatting note. Please get rid of it
everywhere.
> diff --git a/WebKitTools/pywebsocket/example/echo_wsh.py
b/WebKitTools/pywebsocket/example/echo_wsh.py
> +from mod_pywebsocket import msgutil
> +
> +
> +def web_socket_shake_hands(request):
> + pass # always accept
Since python leaves it unspecified, follow the typical WebKit style and only
put one space before end of line comments.
Also, whole sentences that start with a capital and end with a period are
preferred.
> diff --git a/WebKitTools/pywebsocket/mod_pywebsocket/dispatch.py
b/WebKitTools/pywebsocket/mod_pywebsocket/dispatch.py
> +import os
> +import re
> +
Why is there a blank line here?
> +_SHAKE_HANDS_HANDLER_NAME = 'web_socket_shake_hands'
I can't find any references to "shaking hands" when talking about a "handshake"
in a protocol
I'd just call it 'web_socket_do_handshake' or 'web_socket_perform_handshake'
> +_TRANSFER_DATA_HANDLER_NAME = 'web_socket_transfer_data'
> +def _source(source_str):
> + """Source a handler definition string."""
> +
> + global_dic = {}
Use full words "global_dic"
> + try:
> + exec source_str in global_dic
> + except Exception:
> + raise DispatchError('Error in sourcing handler:' +
> + util.get_stack_trace())
No need to line wrap this.
> +def _extract_handler(dic, name):
Use full words "dic"
> +class Dispatcher(object):
> + def shake_hands(self, request):
> + """Hook into Web Socket handshake.
> +
> + Select a handler based on request.uri and call its
> + web_socket_shake_hands function.
> +
> + Args:
> + request: mod_python request.
> + """
> +
> + shake_hands_, _ = self._handler(request)
"_" unfortunate variable name.
> + try:
> + shake_hands_(request)
> + except Exception:
> + raise DispatchError('shake_hands() raised exception: ' +
"shake_hands" isn't the current api name. Why not just use
_SHAKE_HANDS_HANDLER_NAME here?
> + def transfer_data(self, request):
...
> + _, transfer_data_ = self._handler(request)
"_" unfortunate variable name.
> + except Exception:
> + raise DispatchError('transfer_data() raised exception: ' +
Why not use _TRANSFER_DATA_HANDLER_NAME for the function name?
> + util.get_stack_trace())
> diff --git a/WebKitTools/pywebsocket/mod_pywebsocket/handshake.py
b/WebKitTools/pywebsocket/mod_pywebsocket/handshake.py
> +class Handshaker(object):
> + def _set_resource(self):
> + self._request.ws_resource = self._request.uri
Avoid abbreviations "ws_resource"
> + def _set_location(self):
> + location_parts = []
> + if self._request.is_https():
> + location_parts.append(_WEB_SOCKET_SECURE_SCHEME)
> + else:
> + location_parts.append(_WEB_SOCKET_SCHEME)
> + location_parts.append('://')
> + host, port = self._parse_host_header()
> + conn_port = self._request.connection.local_addr[1]
Avoid abbreviations "conn_port"
> + if port != conn_port:
> + raise HandshakeError('Header/connection port mismatch: %d/%d' %
> + (port, conn_port))
> + location_parts.append(host)
> + if ((not self._request.is_https() and
> + port != _DEFAULT_WEB_SOCKET_PORT) or
> + (self._request.is_https() and
> + port != _DEFAULT_WEB_SOCKET_SECURE_PORT)):
Ideally there would be a method to return the default port based on is_https
(perhaps re-use "def get_default_port(use_tls):").
> + def _parse_host_header(self):
> + fields = self._request.headers_in['Host'].split(':', 1)
> + if len(fields) == 1:
> + port = _DEFAULT_WEB_SOCKET_PORT
> + if self._request.is_https():
> + port = _DEFAULT_WEB_SOCKET_SECURE_PORT
Use function to return the port (as above).
> + def _check_header_lines(self):
...
> + raise HandshakeError('Header %s not defined' % key)
How about
raise HandshakeError('Header %s is not defined.' % key)
?
> diff --git a/WebKitTools/pywebsocket/mod_pywebsocket/headerparserhandler.py
b/WebKitTools/pywebsocket/mod_pywebsocket/headerparserhandler.py
> +def headerparserhandler(request):
...
> + request.log_error('mod_pywebsocket: resource:%r' %
request.ws_resource,
Did you want a space before the %r?
> diff --git a/WebKitTools/pywebsocket/mod_pywebsocket/msgutil.py
b/WebKitTools/pywebsocket/mod_pywebsocket/msgutil.py
> +import Queue
> +import StringIO
> +import traceback
> +import threading
out of order (threading before traceback).
> +
> +def _receive_bytes(request, length):
> + bytes = ''
> + while length > 0:
> + new_bytes = request.connection.read(length)
> + bytes += new_bytes
> + length -= len(new_bytes)
> + return bytes
Why not use array concatenation followed by a ''.join() like you've done
elsewhere (to avoid slow string concatenation)?
> +def _read_until(request, delim_char):
> + bytes = ''
> + while True:
> + ch = request.connection.read(1)
> + if ch == delim_char:
> + break
> + bytes += ch
> + return bytes
Why not use array concatenation followed by a ''.join() like you've done
elsewhere (to avoid slow string concatenation)?
> +class MessageReceiver(threading.Thread):
> + def __init__(self, request, onmessage=None):
> + """Construct an instance.
> +
> + Args:
> + request: mod_python request.
> + onmessage: a function to be called when a message is received.
> + Can be None.
s/Can/May/
It may be valuable to mention two things:
1. The onmessage will be called on another thread.
2. If you provide an on message handler, then the received functions are not
useful.
I find #2 strange. It seems like it would be better if there were another class
that used this one to provive receive/receive_nowait, and then this class would
require onmessageto be set.
> + def run(self):
> + while True:
> + message = receive_message(self._request)
> + if self._onmessage:
> + self._onmessage(message)
> + else:
> + self._queue.put(message)
How does this exit? (Does receive_message throw an exception and then the
thread exits?)
> +class MessageSender(threading.Thread):
> + def send(self, message):
> + """Send a message, blocking."""
> +
> + send_message(self._request, message)
So the ordering of this with respect to other messages in the queue is
undefined?
If seems like it should come after all messages that are already in the queue.
I guess this could be explained with some comment in the code about why this
functionality is the way it is.
> diff --git a/WebKitTools/pywebsocket/mod_pywebsocket/standalone.py
b/WebKitTools/pywebsocket/mod_pywebsocket/standalone.py
> +"""Standalone Web Socket server.
> +
> +Use this server to run mod_pywebsocket without Apache HTTP Server.
> +"""
Is this going to be used? It would be nice to have only one way to run these in
apache or not in apache.
> diff --git a/WebKitTools/pywebsocket/setup.py
b/WebKitTools/pywebsocket/setup.py
We need to get rid of set-up. There needs to be a checked in solution that runs
without it.
> diff --git a/WebKitTools/pywebsocket/test/mock.py
b/WebKitTools/pywebsocket/test/mock.py
> +class MockConn(_MockConnBase):
...
> +
> + def readline(self):
> + """Override mod_python.apache.mp_conn.readline."""
> +
> + if self._read_pos >= len(self._read_data):
> + return ''
> + end_index = self._read_data.find('\n', self._read_pos) + 1
> + if end_index == 0:
avoid comparisons to 0.
if not end_index:
> + end_index = len(self._read_data)
> + return self._read_up_to(end_index)
> +class MockBlockingConn(_MockConnBase):
...
> + def readline(self):
...
> + while True:
> + ch = self._queue.get()
If you believe that this variable is worth more than one character, use whole
words. 'c' may be sufficient here.
> + def read(self, length):
> + """Override mod_python.apache.mp_conn.read."""
> +
> + data = ''
> + for _ in range(length):
"_" looks like perl. Why not just use i?
> +class MockTable(dict):
> + """Mock table.
> +
> + This mimicks mod_python mp_table. Note that only the methods used by
s/mimicks/mimics/
> +class MockRequest(object):
> + def __init__(self, uri=None, headers_in={}, connection=None,
> + is_https=False):
> + self.uri = uri
> + self.connection = connection
> + self.headers_in = MockTable(headers_in)
> + self.is_https_ = is_https
Why does is_https_ have a trailing underscore? Did you mean to use a leading
underscore?
> diff --git a/WebKitTools/pywebsocket/test/test_dispatch.py
b/WebKitTools/pywebsocket/test/test_dispatch.py
> +class DispatcherTest(unittest.TestCase):
> + def test_shake_hand(self):
> + dispatcher = dispatch.Dispatcher(_TEST_HANDLERS_DIR)
> + request = mock.MockRequest()
> + request.ws_resource = '/a'
> + request.ws_origin = 'http://example.com'
> + dispatcher.shake_hands(request) # Must not raise exception.
> +
> + request.ws_origin = 'http://bad.example.com'
> + self.assertRaises(dispatch.DispatchError,
> + dispatcher.shake_hands, request)
> +
> + def test_transfer_data(self):
> + dispatcher = dispatch.Dispatcher(_TEST_HANDLERS_DIR)
> + request = mock.MockRequest(connection=mock.MockConn(''))
> + request.ws_resource = '/a'
> + request.ws_protocol = 'p1'
> +
> + dispatcher.transfer_data(request)
> + self.assertEqual('a_wsh.py is called for /a, p1',
> + request.connection.written_data())
> +
> + request = mock.MockRequest(connection=mock.MockConn(''))
> + request.ws_resource = '/sub/e'
> + request.ws_protocol = None
> + dispatcher.transfer_data(request)
> + self.assertEqual('sub/e_wsh.py is called for /sub/e, None',
> + request.connection.written_data())
> +
> + def test_transfer_data_no_handler(self):
> + dispatcher = dispatch.Dispatcher(_TEST_HANDLERS_DIR)
> + for resource in ['/b', '/sub/c', '/sub/d', '/does/not/exist']:
> + request = mock.MockRequest(connection=mock.MockConn(''))
> + request.ws_resource = resource
> + request.ws_protocol = 'p2'
> + try:
> + dispatcher.transfer_data(request)
> + self.fail()
> + except dispatch.DispatchError, e:
> + self.failUnless(str(e).find('No handler') != -1)
> + except Exception:
> + self.fail()
> +
> + def test_transfer_data_handler_exception(self):
> + dispatcher = dispatch.Dispatcher(_TEST_HANDLERS_DIR)
> + request = mock.MockRequest(connection=mock.MockConn(''))
> + request.ws_resource = '/sub/f'
> + request.ws_protocol = 'p3'
> + try:
> + dispatcher.transfer_data(request)
> + self.fail()
> + except dispatch.DispatchError, e:
> + self.failUnless(str(e).find('Intentional') != -1)
> + except Exception:
> + self.fail()
> diff --git a/WebKitTools/pywebsocket/test/test_handshake.py
b/WebKitTools/pywebsocket/test/test_handshake.py
> +_GOOD_REQUEST = (
> + 80,
> + '/demo',
> + {
> + 'Upgrade':'WebSocket',
> + 'Connection':'Upgrade',
> + 'Host':'example.com',
> + 'Origin':'http://example.com',
> + 'WebSocket-Protocol':'sample',
> + }
> +)
> +_GOOD_RESPONSE_SECURE_NONDEF = (
Avoid abbreviations "_GOOD_RESPONSE_SECURE_NONDEF"
> +def _create_request(request_def):
> + conn = mock.MockConn('')
Use whole words "conn"
> diff --git a/WebKitTools/pywebsocket/test/test_util.py
b/WebKitTools/pywebsocket/test/test_util.py
> +class UtilTest(unittest.TestCase):
> + def test_get_stack_trace(self):
> + self.assertEqual('None\n', util.get_stack_trace())
> + try:
> + a = 1 / 0 # Intentionally raise exception
Single space before # and make the comment end with a period.
> diff --git a/WebKitTools/pywebsocket/test/testdata/handlers/a_wsh.py
b/WebKitTools/pywebsocket/test/testdata/handlers/a_wsh.py
Something more descriptive than "a" (ditto for "b_wsh.py" .. "g_wsh.py) would
be nice. Name them according to what they do.
Also "wsh" is an abbreviation. (I think it stands for web socket handler...) It
would be nice to make that more descriptive as well.
> diff --git a/WebKitTools/pywebsocket/test/testdata/handlers/sub/c_wsh.py
b/WebKitTools/pywebsocket/test/testdata/handlers/sub/c_wsh.py
Why is this in a "sub" but a and b aren't?
> diff --git a/WebKitTools/pywebsocket/test/testdata/handlers/sub/d.py
b/WebKitTools/pywebsocket/test/testdata/handlers/sub/d.py
Why doesn't d.py have the _wsh.py postfix like the others? I guess this is
about testing the handler.
> diff --git a/WebKitTools/pywebsocket/test/testdata/handlers/sub/e_wsh.py
b/WebKitTools/pywebsocket/test/testdata/handlers/sub/e_wsh.py
> +def web_socket_shake_hands(request):
> + return True
Why return True here? In other places this just does "pass".
More information about the webkit-reviews
mailing list