Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor to use standard SocketServer RequestHandler design #72

Merged
merged 43 commits into from
Nov 28, 2013

Conversation

astrand
Copy link

@astrand astrand commented Mar 14, 2013

See #71.

Peter Åstrand (astrand) added 4 commits March 14, 2013 15:23
Rename self.client to self.request, since this is what standard
SocketServer request handlers are using.
Move around functions and methods, so that connection-related and
server-related stuff are close together.

This patch just moves things around - it does not change anything at
all. This can be verified with:

git diff websocket.py | grep ^- | cut -c 2- | sort > removed
git diff websocket.py | grep ^+ | cut -c 2- | sort > added
diff -u removed added
* Move traffic_legend.

* Since websocket.WebSocketServer.socket is static, don't call it with
  self.socket.
refactoring. Basically, we are dividing WebSocketServer into two
classes: One request handler following the SocketServer Requesthandler
API, and one optional server engine. The standard Python SocketServer
engine can also be used.

websocketproxy.py has been updated to match the API change. I've also
added a new option --libserver in order to use the Python built in
server instead.

I've done a lot of testing with the new code. This includes: verbose,
daemon, run-once, timeout, idle-timeout, ssl, web, libserver. I've
tested both Python 2 and 3. I've also tested websocket.py in another
external service.

Code details follows:

* The new request handler class is called WebSocketRequestHandler,
  inheriting SimpleHTTPRequestHandler.

* The service engine is called WebSocketServer, just like before.

* do_websocket_handshake: Using send_header() etc, instead of manually
  sending HTTP response.

* A new method called handle_websocket() upgrades the connection to
  WebSocket, if requested. Otherwise, it returns False. A typical
  application use is:

    def do_GET(self):
        if not self.handle_websocket():
	   # handle normal requests

* new_client has been renamed to new_websocket_client, in order to
  have a better name in the SocketServer/HTTPServer request handler
  hierarchy.

* Note that in the request handler, configuration variables must be
  provided by the "server" object, ie self.server.target_host.
@kanaka
Copy link
Member

kanaka commented Mar 14, 2013

Looks like it's taking shape. Here is my initial feedback:

The WebSocketServer class in websocket.py should be usable without websocketproxy. For example, see tests/echo.py and tests/load.py (I just pushed a fix to make them work with the current structure).

At some point I may split out websocket.py into a separate project, especially now that you are cleaning up the code to be more consistent with existing python Socket/HTTP servers/handlers. In addition, websocketproxy really shouldn't have any knowledge of HTTP serving. So the code in LibProxyServer should really live in WebSocketServer. What might make sense is to have WebSocketServer inherit from SocketServer.ForkingMixIn and BaseHTTPServer.HTTPServer but only initialize BaseHTTPServer if the --libserver option is specified.

Related to that, I think the RequestHandler argument should be optional and if none is provided then the default handler is used. I.e. standalone consumers of WebSocketServer like echo.py and load.py shouldn't need to define their own if they don't need to.

Thank you for doing all the testing. The files tests/echo.py and tests/load.py are good standalone servers that make use of websocket.py and are quick tests (just fire up the html page by the same name, e.g. tests/echo.html). I also wanted to mention that OpenStack incorporates websockify (and noVNC) and their code-base is actually what I'm most concerned about breaking with this change. For example, they currently use the WebSocketProxy class. I think CustomProxyServer should be continued to be named WebSocketProxy class (especially since LibProxyServer code should really be incorporated into WebSocketServer).

I don't know if there is any possibility you could test the nova-novncproxy consumer of websockify within openstack, but if you could that would give me a lot more confidence about the changes. I've had some luck using devstack as a simple setup of OpenStack before and I believe it pulls directly from source repos for most components.

Thanks for your work on this!

Peter Åstrand (astrand) added 3 commits March 18, 2013 12:04
TypeError: exceptions must be old-style classes or derived from
BaseException, not str

Thus, we are not allowed to raise a string. Raise Exception instead.
Also, call the server instance "server", not "httpd", even when using
LibProxyServer.
@astrand
Copy link
Author

astrand commented Mar 18, 2013

Looks like it's taking shape. Here is my initial feedback:

The WebSocketServer class in websocket.py should be usable without websocketproxy. For example, see tests/echo.py and tests/load.py (I just pushed a fix to make them work with the current structure).

Where can I find this fix/push? I cannot see it. Yes, the API has changed slightly, so tests needs to be updated. Thanks for pointing this out. I didn't find any dependency on websocketproxy though?

At some point I may split out websocket.py into a separate project, especially now that you are cleaning up the code to be more consistent with existing python Socket/HTTP servers/handlers.

Ok, but for me, the websockify project is a home good enough for websocket.py.

In addition, websocketproxy really shouldn't have any knowledge of HTTP serving. So the code in LibProxyServer should really live in WebSocketServer. What might make sense is to have WebSocketServer inherit from SocketServer.ForkingMixIn and BaseHTTPServer.HTTPServer but only initialize BaseHTTPServer if the --libserver option is specified.

I agree that websocketproxy shouldn't have any knowledge of HTTP. However, the using application must create a server subclass in order to set application-specific options. So the entire LibProxyServer cannot be moved to websocket.py. But perhaps some parts of it.

Letting WebSocketServer inherit from ie BaseHTTPServer is interesting. I will investigate this approach.

Related to that, I think the RequestHandler argument should be optional and if none is provided then the default handler is used. I.e. standalone consumers of WebSocketServer like echo.py and load.py shouldn't need to define their own if they don't need to.

I don't see how that would work? There's no default handler. Besides, the standard SocketServer API requires a request handler parameter.

Thank you for doing all the testing. The files tests/echo.py and tests/load.py are good standalone servers that make use of websocket.py and are quick tests (just fire up the html page by the same name, e.g. tests/echo.html).

Will test.

I also wanted to mention that OpenStack incorporates websockify (and noVNC) and their code-base is actually what I'm most concerned about breaking with this change. For example, they currently use the WebSocketProxy class. I think CustomProxyServer should be continued to be named WebSocketProxy class (especially since LibProxyServer code should really be incorporated into WebSocketServer).

I've renamed CustomProxyServer to WebSocketProxy now.

I don't know if there is any possibility you could test the nova-novncproxy consumer of websockify within openstack, but if you could that would give me a lot more confidence about the changes. I've had some luck using devstack as a simple setup of OpenStack before and I believe it pulls directly from source repos for most components.

I'll see what I can do. I've checked https://github.com/kanaka/noVNC/blob/master/utils/nova-novncproxy now, and one thing that definitely needs to be updated is renaming new_client to new_websock_client.

In general, it might be difficult to keep full backwards compatibility with such a big change. In any case, as the example with new_client above shows, it will be a tradeoff between short term compatibility and long term "nicer names". What do you think is most important? Either way is fine with me.

Peter Åstrand (astrand) added 8 commits March 18, 2013 14:50
>commit b9e1295
>    Prepare for solving novnc#71:
>
>    Rename self.client to self.request, since this is what standard
>    SocketServer request handlers are using.
websocket.py:

* With echo.py, which doesn't need any server configuration, we can
  just switch over our application class to inherit from
  WebSocketRequestHandler instead of WebSocketServer. Also, need to
  use the new method name new_websocket_client.

* With load.py, since we have the "delay" configuration, we need both
  a server class and a request handler.

Note that for both tests, I've removed the raising of
self.EClose(closed). This is incorrect. First of all, it's described
as "An exception before the WebSocket connection was established", so
not suitable for our case. Second, it will cause send_close to be
called twice. Finally, self.EClose is now in the WebSocketServer
class, and not a member of the request handler.
requesthandler. Removed comments about above/below which does not make
sense any longer. No functional changes.
from WebSocketProxy without having to specify handler class.
@astrand
Copy link
Author

astrand commented Mar 20, 2013

The WebSocketServer class in websocket.py should be usable without websocketproxy.
For example, see tests/echo.py and tests/load.py (I just pushed a fix to make them work with
the current structure).

Where can I find this fix/push? I cannot see it. Yes, the API has changed slightly, so tests needs to be updated.

I understand what you mean now. I've fixed the tests now.

In addition, websocketproxy really shouldn't have any knowledge of HTTP serving. So the code in
LibProxyServer should really live in WebSocketServer. What might make sense is to have WebSocketServer
inherit from SocketServer.ForkingMixIn and BaseHTTPServer.HTTPServer but only initialize
BaseHTTPServer if the --libserver option is specified.

I agree that websocketproxy shouldn't have any knowledge of HTTP. However, the using application must create a server subclass in order to set application-specific options. So the entire LibProxyServer cannot be moved to websocket.py. But perhaps some parts of it.

Letting WebSocketServer inherit from ie BaseHTTPServer is interesting. I will investigate this approach.

Investigation done. This is indeed possible, and websocketproxy gets smaller. However, after thinking about it, I think this is a bad idea. From an application point of view, it doesn't make much sense to support two server engines at the same time. You should decide on either the standard SocketServer engine, or the custom one in websocket.py. The API is different; there's not much in common except that they both support the same request handler API. So trying to merge them into one is a bad idea, it will just be confusing. It wil also mean that we will have 3 different use cases: 1) WebSocketServer(libserver=False), 2) WebSocketServer(libserver=True), and 3) an application specific server derived from SocketServer (this is what we are doing). This means more work with testing etc. I prefer to have only two cases: 1) WebSocketServer, and 3) standard SocketServer.

websocketproxy is slightly longer now, supporting both cases above, since I wanted to demonstrate this. This part is optional; I can remove or move it if you want.

Wrt websocketproxy having knowledge of BaseHTTPServer.HTTPServer when using "libserver": I don't think there's anything wrong with that. It's possible to use SocketServer.TCPServer instead, but BaseHTTPServer.HTTPServer is nicer, since it sets allow_reuse_address = 1.

I also wanted to mention that OpenStack incorporates websockify (and noVNC) and their code-base is
actually what I'm most concerned about breaking with this change. For example, they currently use the
WebSocketProxy class. I think CustomProxyServer should be continued to be named WebSocketProxy class
(especially since LibProxyServer code should really be incorporated into WebSocketServer).

Thought more of this now. I think that for WebSocketProxy, we do not need to follow the SocketServer API. Thus, it is possible to let WebSocketProxy have ProxyRequestHandler as a default handler.

I don't know if there is any possibility you could test the nova-novncproxy consumer of websockify within
openstack, but if you could that would give me a lot more confidence about the changes. I've had some luck
using devstack as a simple setup of OpenStack before and I believe it pulls directly from source repos for most >components.

Will check.

@astrand
Copy link
Author

astrand commented Mar 20, 2013

I don't know if there is any possibility you could test the nova-novncproxy consumer of websockify within
openstack, but if you could that would give me a lot more confidence about the changes. I've had some luck
using devstack as a simple setup of OpenStack before and I believe it pulls directly from source repos for most >components.

Will check.

Checked now. OpenStack is available on CentOS 6, which I'm using, so I could install OpenStack easily. I haven't been able to do a full test, but could at least check that the proxy starts and enters the handler correctly.

It still seems impossible to provide full backwards API compatibility: An app such as nova-novncproxy must subclass the request handler, but also start the server. The necessary changes are minimal, though:

-class NovaWebSocketProxy(wsproxy.WebSocketProxy):
-    def __init__(self, *args, **kwargs):
-        wsproxy.WebSocketProxy.__init__(self, *args, **kwargs)

-    def new_client(self):
+class NovaWebSocketHandler(wsproxy.ProxyRequestHandler):
+    def new_websocket_client(self):
...
-    server = NovaWebSocketProxy(listen_host=CONF.novncproxy_host,
+    server = wsproxy.WebSocketProxy(NovaWebSocketHandler, listen_host=CONF.novncproxy_host,

My suggestion is that we should go with this approach.

@astrand
Copy link
Author

astrand commented Mar 20, 2013

I've done a lot of tests with the latest code, for example:

echo: works
load: works
verbose: works
record: works
run-once: works
timeout: works
idle-timeout: works
ssl: works
web: works
libserver: works
python3: works

It would be great if you could consider merging this work, or parts of it. For example, one easy thing to start with could be the rename from self.client to self.request.

@DirectXMan12
Copy link
Member

@astrand : sorry for the slow response. Can you update you pull request to work with the most recent code, so we can figure out exactly what we want to merge? If there are any changes that would potentially break the OpenStack Nova-NoVNCProxy, I can help dealing with them, since my day job is being a Nova developer.

@astrand
Copy link
Author

astrand commented Nov 5, 2013

Good. Unfortunately I'm short on time right now, but I'll take a look at this soon.

Peter Åstrand (astrand) added 7 commits November 27, 2013 11:36
* commit '2d078e8cd83735dad7d98fadcece652d93e2a037':
  correctly include include directory in egg.
* commit '33e9a21ce4e38d52f43fe775fb154fc71c09c827':
  Add gimite/web-socket-js submodule for DFSG compliance.
* commit '477947ba96a00032ae35ac55fd02a4a5f485497e':
  Remove wsproxy references. Sync launch.sh from noVNC.
* commit 'b7f255ce0b21dc42189205b1f0e46b4f1d9854f9':
  Clarify messages when optional modules are not found.
* commit '6d27b5d321978586ea1601f757ead73dfba03da7':
  Add 2 arguments to websockify.WSRequestHandler

As of now, only implemented the first command; see #83 for details.
* commit 'd3ba23fa64d79eeb602ff1015ec31014fd8e9b35':
  Update to c0855c6cae of web-socket-js
Peter Åstrand (astrand) added 20 commits November 27, 2013 13:30
* commit '36fcd5784fa0825eedbf31d91bc42c970605ddb4':
  Add package file for websockify.js
* commit '36cb8f4676c7b5ff34bd22ad729e00e77efb6f00':
  Move javascript websockify files to other/js
* commit '264f8fdd7f12bd5b9f6813fb8de81c55b6328d9b':
  Update to version 0.5.1
* commit 'ab389d4e7114d7ddbfd085591d336ea5cc06c00d':
  Collect zombie child processes within server loop
* commit 'f30ad05c70ab2a43c9078e2f79da40f1dc0c60ec':
  Fix novnc#97: rebind.so not found when installed
* commit 'edde5cd0ff6059bddae10c9b9faf0b3e8f388a9e':
  Fixed wiki reference
* commit 'a7fa97f0e14926cc4433483fcb7581e0b3782140':
  WebSocketProxy: support non path target_cfg
* commit '4459824cc8196ad78fe9258b6c560ad46fe4cd52':
  websocket: do not exit at the middle of process
  websocket: restore signals after processing
  websocket: support SIGTERM as exit signal
* commit '477dce6cf86d61b20a394f3cbf3170e60d199658':
  websocket: use python logging module
  websocket: fix exception statement introduced by comment 903e3f0

Adapted to new standard SocketServer RequestHandler design. For
example, this means that self.i_am_client is not needed.
* commit 'a61ae52610642ae58e914dda705df8bb9c8213ec':
  fixed 1.8 compatibility bug for OpenSSL::SSL::SSLSocket#read_nonblock vs #readpartial tested in 1.8 and 2.0
  adding SSL support and Ruby1.9 support
* commit '0e5c3ecfda3b1506b41412052db75d84df2b4ae7':
  Handle SIGCHLD properly for multiprocessing
* commit '32b0567343aee7753b2b6be1bc1ee9a69657ba26':
  Fix syntax errors in other/websockify.rb
* commit 'b4e0b534d5d04d57265045b4baf49dd81555064b':
  Fix crash when an import is missing
* commit '13c99bcf053f7f3af8ba84c0d963a9591e020f49':
  Fix search path construction in tests.
* commit 'c3acdc2e38f871e28ffda1847b4338c4b02296b8':
  Adds optional TCP_KEEPALIVE to WebSocketServer
* commit 'a47be21f9fa69ddf8d888ff9e3c75cdfc9e31c00':
  Added unit tests for websocket and websocketproxy
* commit 'a04edfe80f54b44df5a3579f71710560c6b7b4fc':
  Added temp dir for unit test data and cleanup
* commit '34a1b68d79a13c03aa63b5c4194796341c9383fe':
  Clarify ssl module build for old python versions.
in request handler class.
@astrand
Copy link
Author

astrand commented Nov 28, 2013

I've merged all changes on master now. That was a lot of work! Many of the changes were difficult to merge automatically - they were based on the peculiarities of the current design, and had to be slightly changed. For example, self.i_am_client is not necessary when the request handler and server are different classes.

It would be great if we could merge this refactoring as soon as possible, to avoid having to "port" any additional work to this new design.

As I mentioned before, we could start with renaming self.client to self.request. That would make the diff smaller. I can create a pull request for this, stay tuned.

@astrand astrand merged commit db93395 into novnc:master Nov 28, 2013
astrand pushed a commit to astrand/websockify that referenced this pull request Nov 28, 2013
request novnc#72. The standard Python SocketServer/BaseRequestHandler
requires this name.
astrand pushed a commit to astrand/websockify that referenced this pull request Nov 28, 2013
name in the SocketServer/HTTPServer request handler hierarchy. Prepare
for merge pull request novnc#72. This work has been picked out of
7b3dd8a .
@astrand
Copy link
Author

astrand commented Nov 28, 2013

NOTE: I pushed this work to the main master branch by mistake. This has been corrected, but note that this issue/pull is NOT merged or closed.

@astrand
Copy link
Author

astrand commented Nov 28, 2013

Since it is apparently impossible to reopen a closed pull request, let's continue on #111.

astrand pushed a commit that referenced this pull request Dec 16, 2013
Rename self.client and new_client - prepare for #72 / #111
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants