-
Notifications
You must be signed in to change notification settings - Fork 5.5k
websocket redirect ability #2557
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
base: master
Are you sure you want to change the base?
Changes from 11 commits
ad4aff5
a8a0757
83e1693
146ec51
63b533d
3d96466
047f2fb
4d9695c
6a1204c
a32e12e
2c1d310
5e6a2f5
6da3945
710274d
f30fb3f
bfb9b84
e9020e2
8cebeb6
a42ad6c
e689ea1
6da973a
b47da1b
31b16b3
7940a56
5d5cd26
1dd024f
d11212b
ef2d725
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,16 +18,21 @@ | |
|
||
import abc | ||
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 | ||
|
@@ -143,6 +148,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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
"""Raised when request's max_redirects <= 0 | ||
or follow_redirect is False | ||
""" | ||
|
||
pass | ||
|
||
|
||
class _DecompressTooLargeError(Exception): | ||
pass | ||
|
||
|
@@ -1376,6 +1398,7 @@ def __init__( | |
self.ping_interval = ping_interval | ||
self.ping_timeout = ping_timeout | ||
self.max_message_size = max_message_size | ||
self._redirect_location = None # type: Optional[str] | ||
|
||
scheme, sep, rest = request.url.partition(":") | ||
scheme = {"ws": "http", "wss": "https"}[scheme] | ||
|
@@ -1430,7 +1453,14 @@ def close(self, code: int = None, reason: str = None) -> None: | |
|
||
def on_connection_close(self) -> None: | ||
if not self.connect_future.done(): | ||
self.connect_future.set_exception(StreamClosedError()) | ||
if self._redirect_location is not None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like the wrong place for this. I'd expect the redirect to be handled in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally right, An redirect exception is already set in finish, there is no need to add these code here, removed. |
||
# websocket redirect, raise exception for reconnect | ||
# to redirected url | ||
self.connect_future.set_exception( | ||
WebSocketRedirect(self._redirect_location) | ||
) | ||
else: | ||
self.connect_future.set_exception(StreamClosedError()) | ||
self._on_message(None) | ||
self.tcp_client.close() | ||
super(WebSocketClientConnection, self).on_connection_close() | ||
|
@@ -1450,6 +1480,14 @@ async def headers_received( | |
headers: httputil.HTTPHeaders, | ||
) -> None: | ||
assert isinstance(start_line, httputil.ResponseStartLine) | ||
self.code = start_line.code | ||
self.reason = start_line.reason | ||
self.headers = headers | ||
|
||
if self._should_follow_redirect(): | ||
self._redirect_location = headers["Location"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused about the flow here. Isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These code are used to save redirect location, but We can get it from self.headers in |
||
return | ||
|
||
if start_line.code != 101: | ||
await super(WebSocketClientConnection, self).headers_received( | ||
start_line, headers | ||
|
@@ -1476,6 +1514,20 @@ async def headers_received( | |
|
||
future_set_result_unless_cancelled(self.connect_future, self) | ||
|
||
def finish(self) -> None: | ||
if self._should_follow_redirect(): | ||
if self._redirect_location is not None: | ||
self.connect_future.set_exception( | ||
WebSocketRedirect(self._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]": | ||
|
@@ -1634,6 +1686,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, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could import tornado.web.RedirectHandler instead of defining a new one here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed now