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

Websockets 14 does not accept Transfer-Encoding header in error responses #1550

Open
Jolmberg opened this issue Nov 15, 2024 · 4 comments
Open

Comments

@Jolmberg
Copy link

After upgrading to websockets 14, websockets.connect is raising the exception NotImplementedError: transfer codings aren't supported in a scenario that previously raised InvalidStatusCode (503). I was expecting it to change to InvalidStatus, which would have been fine, but even if I catch the NotImplementedError, there is no way to extract an error code from it. The issue arises when my server, in response to the websocket upgrade request returns a 503 error, with transfer-encoding: chunked and then a short error message in the body. While it could be argued that this particular message could just use Content-Length rather than chunked encoding, this does seem like a regression on the part of websockets.

The entire back and forth looks something like this.
Request (sent with websockets 14.1):

GET /path HTTP/1.1
Host: 192.168.0.1:8888
Upgrade: websocket
Connection: Upgrade
Sec-WebSocket-Key: bDKBNWhBF+86IyQBcT176A==
Sec-WebSocket-Version: 13
Sec-WebSocket-Extensions: permessage-deflate; client_max_window_bits
Sec-WebSocket-Protocol: xyz
Authorization: Bearer 97eab12b-2766-46ca-9ec4-f65d8d26d64c
User-Agent: Python/3.12 websockets/14.1

Response:

HTTP/1.1 503 Service Unavailable
date: Fri, 15 Nov 2024 08:17:57 GMT
server: Cowboy
transfer-encoding: chunked

This did not go well
@aaugustin
Copy link
Member

This is happening because the legacy implementation didn't even attempt to read the HTTP response body. I added that functionality in the standard case but not in the chunked encoding case.

I believe that #1549 will wrap that error in an InvalidMessage, which is closer to what you want. However, it still doesn't give you the HTTP error code, because websockets parses the entire request first and only then starts the opening handshake logic.

I could make an argument for skipping the body rather than crashing in this case. Let me think about it.

@aaugustin
Copy link
Member

I forgot to mention — as a short term fix, you can use websockets.legacy.client.connect instead of websockets.connect and you buy five years. See https://websockets.readthedocs.io/en/latest/project/changelog.html and https://websockets.readthedocs.io/en/latest/howto/upgrade.html for context.

@Jolmberg
Copy link
Author

Thanks! I guess I'll go with the legacy code for now. For the record, though, I don't really care about the message body in this case, and I don't mind altering my code, I just need some way to figure out the error code.

@aaugustin
Copy link
Member

Yes, it's a legitimate request for improvement. The HTTP code is right there and it's reasonable to expect websockets to give it to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants