Merge websocket_connect into the WebSocketClientConnection class #934

Merged
merged 2 commits into from Jan 24, 2015

Projects

None yet

2 participants

Contributor

It is currently extremely difficult to subclass WebSocketClientConnection without reimplementing websocket_connect due to the fact that WebSocketClientConnection is never really exposed outside the websocket module. In my program, my websocket client is also operating as a HTTP server so needing to loop on the websocket client's read_message is awkward. I ended up subclassing WebSocketClientConnection and overriding its on_message to act more like an expected callback-type function but it seems like a hack since I also had to reimplement websocket_connect to use my own class.

My idea is to move the call the parent's init to a connect function (which admittedly is also hacky) and change the connect_future's eventual return value to True/False value instead of returning a handle to the websocket client object.

EDIT: I'm happy to submit a merge request if people aren't opposed to the init hackery.

Owner

WebSocketClientConnection isn't supposed to be subclassed; it's more or less an implementation detail. As much as possible I'd like to add functionality without inviting user-created subclasses. For example, we could add a set_message_callback function as an alternative to the (admittedly somewhat experimental) Future-based interface it has now.

Contributor

The ability to have a specified callback would satisfy my requirements just fine.

Would you want this to be specified at initialization or changeable during runtime? My concern about the latter is there may be some weird race condition with trying to empty the possible non-empty queue (presumably if you go from no callback to something, queued messages should call the callback) and attaching the new callback. I imagine one would want messages to be delivered in order so changing over the callback before emptying the queue would cause out of order delivery.

Owner
bdarnell commented Dec 1, 2013

I think initialization-time is probably best given the complexity of dealing with the queue. It's kind of analogous to the httpclient streaming_callback - I wonder if it should be on the HTTPRequest object (perhaps even just using streaming_callback?) or a parameter to websocket_connect.

Contributor

Admittedly I'm not really familiar with streaming_callback but wouldn't that interrupt the "normal" websocket processing if we hook into something that low in stack?

I'll come up with something that takes a on_message_callback on init and do a pull request. If I can figure out Hub I'll try to link it to this Issue.

@bdarnell bdarnell added the websocket label Jul 16, 2014
@bdarnell bdarnell merged commit 1e8582f into tornadoweb:master Jan 24, 2015

1 check passed

default The Travis CI build passed
Details
@Caligatio Caligatio deleted the unknown repository branch Jan 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment