-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
linuxhenhao
wants to merge
28
commits into
tornadoweb:master
Choose a base branch
from
linuxhenhao:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
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 a8a0757
WIP:
linuxhenhao 83e1693
[bug-fix] now redirect is working, but error info is annoying
linuxhenhao 146ec51
add finish to websocket connection, avoid redirect exc info
linuxhenhao 63b533d
unittest for websocket redirect added
linuxhenhao 3d96466
flake8 correction
linuxhenhao 047f2fb
mypy fix
linuxhenhao 4d9695c
bugfix: fixed bugs induced by mypy changes
linuxhenhao 6a1204c
flake8 fixes
linuxhenhao a32e12e
black fixes
linuxhenhao 2c1d310
mypy fix
linuxhenhao 5e6a2f5
WIP: redirection in websocket_connect handshake supported
linuxhenhao 6da3945
WIP:
linuxhenhao 710274d
[bug-fix] now redirect is working, but error info is annoying
linuxhenhao f30fb3f
add finish to websocket connection, avoid redirect exc info
linuxhenhao bfb9b84
unittest for websocket redirect added
linuxhenhao e9020e2
flake8 correction
linuxhenhao 8cebeb6
mypy fix
linuxhenhao a42ad6c
bugfix: fixed bugs induced by mypy changes
linuxhenhao e689ea1
flake8 fixes
linuxhenhao 6da973a
black fixes
linuxhenhao b47da1b
mypy fix
linuxhenhao 31b16b3
rebase && using web.RedirectHandler
linuxhenhao 7940a56
Merge branch 'websocket_redirect_support_rebased'
linuxhenhao 5d5cd26
removed unnecessary redirect exception setting code
linuxhenhao 1dd024f
deleted dumplicated code
linuxhenhao d11212b
added type anotation for self.headers
linuxhenhao ef2d725
black format fix
linuxhenhao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 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
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.
SimpleAsyncHTTPClient.fetch()
andwebsocket_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. Sincewebsocket_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 couldwebsocket_connect()
do the exact same.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.
@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.