Add websocket subprotocol support #150
Merged
Conversation
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
|
PR looks good. i don't think anybody uses aiohttp websocket implementation, so i think it's fine if we break api. thanks! |
## Rationale:
We have to manually do this currently even while it should be part of the handshake.
This leads to ignorance of this feature, and chrome not working due to a missing header when testing only in firefox (which ignores if none of sent subprotocols come back)
## Implementation and usage:
I’m adding a kwarg to `do_handshake` which allows the user to specifiy subprotocols his/her client speaks. `do_handshake` returns an additional header with the chosen protocol if the handshake is successful. This doesn’t break the API in any way.
If the server speaks any of the supplied subprotocols, the first protocol spoken by both is selected from the the client-supplied list, else the handshake fails. Essentially this means that you order the `protocols` sequence by preference: best protocol first, worst last.
## Open Questions:
1. The agreed-upon protocol is valuable information not only for the Server, but the user: If we speak multiple protocols, we want to know which one the handshake selected. The user has to extract it from the headers, which are simply a sequence and no dict, making this impractical.
Should we change the response tuple to include the protocol as fifth mamber? This would break unpacking assignments like this:
```python
>>> code, response_headers, parser, writer = do_handshake('get', headers, transport)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: too many values to unpack (expected 4)
```
We could also return something unpackable to 4 elemets that also has a “protocol” field, but that would make the code more complex (tuples are very useful to avoid creating Java-style result-classes).
2. Autobahn|Python forbids duplicate mentions of protocols in the comma-separated list sent by the server. Should we also fail the handshake when this happens or be more lenient?
flying-sheep
force-pushed
the
patch-1
branch
from
4ca8df7 to
92ce18b
Compare
|
old API is ded, long live the new API :) tests pass, and i’m in the contributor list (Phil Schaf is not my real name, therefore the “A.”) |
|
Looks good for me. Is this ready for merge? |
|
you tell me. if the tests aren’t sufficient, i can add more, and if they are, well, you see the travis build passed. |
| protocol = proto | ||
| break | ||
| else: | ||
| raise errors.HttpBadRequest( |
There was a problem hiding this comment.
HttpBadRequest case is not test-covered as far as I see
fafhrd91
added a commit
that referenced
this pull request
Oct 9, 2014
Add websocket subprotocol support
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Rationale:
We have to manually do this currently even while it should be part of the handshake.
This leads to ignorance of this feature, and chrome not working due to a missing header when testing only in firefox (which ignores if none of sent subprotocols come back)
Implementation and usage:
I’m adding a kwarg to
do_handshakewhich allows the user to specifiy subprotocols his/her client speaks.do_handshakereturns an additional header with the chosen protocol if the handshake is successful. This doesn’t break the API in any way.If the server speaks any of the supplied subprotocols, the first protocol spoken by both is selected from the the client-supplied list, else the handshake fails. Essentially this means that you order the
protocolssequence by preference: best protocol first, worst last.Open Questions:
The agreed-upon protocol is valuable information not only for the Server, but the user: If we speak multiple protocols, we want to know which one the handshake selected. The user has to extract it from the headers, which are simply a sequence and no dict, making this impractical.
Should we change the response tuple to include the protocol as fifth mamber? This would break unpacking assignments like this:
We could also return something unpackable to 4 elemets that also has a “protocol” field, but that would make the code more complex (tuples are very useful to avoid creating Java-style result-classes).
Autobahn|Python forbids duplicate mentions of protocols in the comma-separated list sent by the server. Should we also fail the handshake when this happens or be more lenient?