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

websocket redirect ability #2557

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
ad4aff5
WIP: redirection in websocket_connect handshake supported
linuxhenhao Dec 22, 2018
a8a0757
WIP:
linuxhenhao Dec 22, 2018
83e1693
[bug-fix] now redirect is working, but error info is annoying
linuxhenhao Dec 23, 2018
146ec51
add finish to websocket connection, avoid redirect exc info
linuxhenhao Dec 23, 2018
63b533d
unittest for websocket redirect added
linuxhenhao Dec 23, 2018
3d96466
flake8 correction
linuxhenhao Dec 23, 2018
047f2fb
mypy fix
linuxhenhao Dec 23, 2018
4d9695c
bugfix: fixed bugs induced by mypy changes
linuxhenhao Dec 23, 2018
6a1204c
flake8 fixes
linuxhenhao Dec 23, 2018
a32e12e
black fixes
linuxhenhao Dec 23, 2018
2c1d310
mypy fix
linuxhenhao Dec 23, 2018
5e6a2f5
WIP: redirection in websocket_connect handshake supported
linuxhenhao Dec 22, 2018
6da3945
WIP:
linuxhenhao Dec 22, 2018
710274d
[bug-fix] now redirect is working, but error info is annoying
linuxhenhao Dec 23, 2018
f30fb3f
add finish to websocket connection, avoid redirect exc info
linuxhenhao Dec 23, 2018
bfb9b84
unittest for websocket redirect added
linuxhenhao Dec 23, 2018
e9020e2
flake8 correction
linuxhenhao Dec 23, 2018
8cebeb6
mypy fix
linuxhenhao Dec 23, 2018
a42ad6c
bugfix: fixed bugs induced by mypy changes
linuxhenhao Dec 23, 2018
e689ea1
flake8 fixes
linuxhenhao Dec 23, 2018
6da973a
black fixes
linuxhenhao Dec 23, 2018
b47da1b
mypy fix
linuxhenhao Dec 23, 2018
31b16b3
rebase && using web.RedirectHandler
linuxhenhao Jan 19, 2019
7940a56
Merge branch 'websocket_redirect_support_rebased'
linuxhenhao Jan 19, 2019
5d5cd26
removed unnecessary redirect exception setting code
linuxhenhao Jan 19, 2019
1dd024f
deleted dumplicated code
linuxhenhao Jan 19, 2019
d11212b
added type anotation for self.headers
linuxhenhao Jan 19, 2019
ef2d725
black format fix
linuxhenhao Jan 19, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion tornado/test/websocket_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from tornado.simple_httpclient import SimpleAsyncHTTPClient
from tornado.template import DictLoader
from tornado.testing import AsyncHTTPTestCase, gen_test, bind_unused_port, ExpectLog
from tornado.web import Application, RequestHandler
from tornado.web import Application, RequestHandler, RedirectHandler

try:
import tornado.websocket # noqa: F401
Expand Down Expand Up @@ -202,6 +202,8 @@ def get_app(self):
return Application(
[
("/echo", EchoHandler, dict(close_future=self.close_future)),
("/redirect", RedirectHandler, dict(url="/echo")),
("/double_redirect", RedirectHandler, dict(url="/redirect")),
("/non_ws", NonWebSocketHandler),
("/header", HeaderHandler, dict(close_future=self.close_future)),
(
Expand Down Expand Up @@ -344,6 +346,20 @@ def test_websocket_http_success(self):
with self.assertRaises(WebSocketError):
yield self.ws_connect("/non_ws")

@gen_test
def test_websocket_redirect(self):
conn = yield self.ws_connect("/redirect")
yield conn.write_message("hello redirect")
msg = yield conn.read_message()
self.assertEqual("hello redirect", msg)

@gen_test
def test_websocket_double_redirect(self):
conn = yield self.ws_connect("/double_redirect")
yield conn.write_message("hello redirect")
msg = yield conn.read_message()
self.assertEqual("hello redirect", msg)

@gen_test
def test_websocket_network_fail(self):
sock, port = bind_unused_port()
Expand Down
139 changes: 136 additions & 3 deletions tornado/websocket.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,21 @@
import abc
import asyncio
import base64
import copy
import hashlib
import os
import sys
import struct
import tornado.escape
import tornado.web
from urllib.parse import urlparse
from urllib.parse import urlparse, urljoin
import zlib

from tornado.concurrent import Future, future_set_result_unless_cancelled
from tornado.concurrent import (
Future,
future_set_result_unless_cancelled,
future_set_exception_unless_cancelled,
)
from tornado.escape import utf8, native_str, to_unicode
from tornado import gen, httpclient, httputil
from tornado.ioloop import IOLoop, PeriodicCallback
Expand Down Expand Up @@ -115,6 +120,23 @@ class WebSocketClosedError(WebSocketError):
pass


class WebSocketRedirect(WebSocketError):
"""Raised when code is 302/303 in received headers
while doing websocket handshake
"""

def __init__(self, target_location: str) -> None:
self.target_location = target_location


class RequestRedirectError(WebSocketError):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

"""Raised when request's max_redirects <= 0
or follow_redirect is False
"""

pass


class _DecompressTooLargeError(Exception):
pass

Expand Down Expand Up @@ -1454,6 +1476,7 @@ async def headers_received(
headers: httputil.HTTPHeaders,
) -> None:
assert isinstance(start_line, httputil.ResponseStartLine)

if start_line.code != 101:
await super(WebSocketClientConnection, self).headers_received(
start_line, headers
Expand All @@ -1464,7 +1487,7 @@ async def headers_received(
self.io_loop.remove_timeout(self._timeout)
self._timeout = None

self.headers = headers
self.headers = headers # type: httputil.HTTPHeaders
self.protocol = self.get_websocket_protocol()
self.protocol._process_server_headers(self.key, self.headers)
self.protocol.stream = self.connection.detach()
Expand All @@ -1480,6 +1503,19 @@ async def headers_received(

future_set_result_unless_cancelled(self.connect_future, self)

def finish(self) -> None:
if self._should_follow_redirect():
redirect_location = self.headers.get("Location", None)
if redirect_location:
self.connect_future.set_exception(WebSocketRedirect(redirect_location))
else:
raise ValueError("redirect location is None")
# close connection
self.on_connection_close()
self.connection.close()
else:
super().finish()

def write_message(
self, message: Union[str, bytes], binary: bool = False
) -> "Future[None]":
Expand Down Expand Up @@ -1638,6 +1674,103 @@ def websocket_connect(
httpclient.HTTPRequest,
httpclient._RequestProxy(request, httpclient.HTTPRequest._DEFAULTS),
)

def do_connect(
request: httpclient.HTTPRequest
) -> "Future[WebSocketClientConnection]":
conn_future = _websocket_connect(
request,
callback,
connect_timeout,
on_message_callback,
compression_options,
ping_interval,
ping_timeout,
max_message_size,
subprotocols,
)
return conn_future

def wrap_conn_future_callback(
conn_future: "Future[WebSocketClientConnection]"
) -> None:
try:
conn = conn_future.result()
except WebSocketRedirect as e:
try:
new_request = redirect_request(request, e.target_location)
except RequestRedirectError as e:
future_set_exception_unless_cancelled(wrapped_conn_future, e)
else:
conn_future = do_connect(new_request)
IOLoop.current().add_future(conn_future, wrap_conn_future_callback)
except Exception as e:
future_set_exception_unless_cancelled(wrapped_conn_future, e)
else:
future_set_result_unless_cancelled(wrapped_conn_future, conn)

wrapped_conn_future = Future() # type: Future[WebSocketClientConnection]
conn_future = do_connect(request)
IOLoop.current().add_future(conn_future, wrap_conn_future_callback)

return wrapped_conn_future


def redirect_request(
request: httpclient.HTTPRequest, target_location: str
) -> httpclient.HTTPRequest:
request_proxy = cast(httpclient._RequestProxy, request)

if not request_proxy.follow_redirects:
raise RequestRedirectError("follow_redirects is not True")

if request_proxy.max_redirects is None or request_proxy.max_redirects <= 0:
raise RequestRedirectError("max_redirects occurred, no more redirect")

original_request = getattr(request, "original_request", request)
new_request = copy.copy(request_proxy.request)
new_request.url = urljoin(request_proxy.request.url, target_location)

new_request.max_redirects = request_proxy.max_redirects - 1
del new_request.headers["Host"]
# http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3.4
# Client SHOULD make a GET request after a 303.
# According to the spec, 302 should be followed by the same
# method as the original request, but in practice browsers
# treat 302 the same as 303, and many servers use 302 for
# compatibility with pre-HTTP/1.1 user agents which don't
# understand the 303 status.
new_request.method = "GET"
new_request.body = cast(bytes, None)
for h in [
"Content-Length",
"Content-Type",
"Content-Encoding",
"Transfer-Encoding",
]:
try:
del new_request.headers[h]
except KeyError:
pass
new_request.original_request = original_request # type: ignore
new_request = cast(
httpclient.HTTPRequest,
httpclient._RequestProxy(new_request, httpclient.HTTPRequest._DEFAULTS),
)
return new_request


def _websocket_connect(
request: httpclient.HTTPRequest,
callback: Callable[["Future[WebSocketClientConnection]"], None] = None,
connect_timeout: float = None,
on_message_callback: Callable[[Union[None, str, bytes]], None] = None,
compression_options: Dict[str, Any] = None,
ping_interval: float = None,
ping_timeout: float = None,
max_message_size: int = _default_max_message_size,
subprotocols: List[str] = None,
) -> "Future[WebSocketClientConnection]":
conn = WebSocketClientConnection(
request,
on_message_callback=on_message_callback,
Expand Down