websocket redirect ability

[linuxhenhao]

After meeting the same problem as issue #2405 descripts, following the 'raise an exception and do new websocket_connect to target location' mechanism, this PR was made.

• To be compliant with already existed HTTP1Connection workflow, This PR added a finish function to
  WebSocketConnection class to set WebsocketRedirect Exception for redirect cases, and added several lines of code in headers_received function to save the target_location for finish.

• A wrapper is added upon the original websocket_connect to handle WebsocketRedirect Exception

[bdarnell]

What if we just refactor SimpleAsyncHTTPClient.fetch so that instead of calling self.client.fetch directly, it calls a new method self.send_redirect_request(new_request), and we can override this method in WebSocketClientConnection? This looks like it would need less duplication with simple_httpclient.

[bdarnell]

You could import tornado.web.RedirectHandler instead of defining a new one here.

[linuxhenhao]

fixed now

[bdarnell]

We don't have an error like this for simple_httpclient (we just raise the last HTTPError with status code 3xx, I think). Why introduce one specific to websockets? (I don't think a new error is a bad idea, but I think we should try to be consistent)

[linuxhenhao]

Be consistent is always very import for a project. But from my perspective something like SimpleHTTPClient or AsyncSimpleHTTPClient is very different compared to a WebSocketClientConnection, HTTPClient is one level higher than a connection. That's also why a redirect exception is raised here. For a HTTPClient, when redirecting, just do another fetch to target
request is enough. But for a connection, redirect means this connection is going to be closed and we should start a new connection to the redirected location. I cannot find a HTTPError(302/303) in
simple_httpclient.py. WebSocketRedirectError is what I could find as a simple and clear word to represent what is going to happen. I'm really open to accept any other suitable name

[ploxiln]

SimpleAsyncHTTPClient.fetch() and websocket_connect() are in the same situation regarding redirects. Since the Simple client does not (currently) support keep-alive connections, another fetch is always another connection. Since websocket_connect() uses the Simple client, that's true for it too. But if SimpleAsyncHTTPClient supported keep-alive connections, then it would re-use the same connection for a redirect to another path on the same host:port, and so could websocket_connect() do the exact same.

[linuxhenhao]

@ploxiln websocket_connect do not use Simple client, That's why in issue #2405 , self.client.fetch raises a None object do not has fetch method exception. At here, None is passed to WebSocketClientConnection's super class _HTTPConnction as a client.

[bdarnell]

This seems like the wrong place for this. I'd expect the redirect to be handled in _on_http_response or finish so by the time on_connection_close is called connect_future is done.

[linuxhenhao]

Totally right, An redirect exception is already set in finish, there is no need to add these code here, removed.

[bdarnell]

I'm confused about the flow here. Isn't _on_http_response the last thing that gets called, after finish? This seems too late (which may be why you needed the on_connection_close hook instead of doing everything from finish).

[linuxhenhao]

These code are used to save redirect location, but We can get it from self.headers in finish. So these lines were removed.

[linuxhenhao]

What if we just refactor SimpleAsyncHTTPClient.fetch so that instead of calling self.client.fetch directly, it calls a new method self.send_redirect_request(new_request), and we can override this method in WebSocketClientConnection? This looks like it would need less duplication with simple_httpclient.

To my understand, WebSocketConnection is one level lower than AsyncHTTPClient. A HTTPClient can have
many connections, there is no HTTPClient in websocket.py. In websocket_connect, only a connection is established and if there is no redirect, this connection is deteached and then used by user. That's why a wrapper is added upon the original websocket_connect function, one redirect means a connection is closed and a new connection should be established, only the last one without redirect is detached and returned.

[bdarnell]

To my understand, WebSocketConnection is one level lower than AsyncHTTPClient. A HTTPClient can have many connections, there is no HTTPClient in websocket.py.

Well, there was no HTTPClient when this was originally written because there was exactly one connection, but now that it's possible to follow redirects that's not true. I think it's better to introduce abstractions similar to HTTPClient to manage redirections in the same way between HTTP and WS instead of duplicating logic as you have done here (especially in redirect_request).