Support the WebSocket Denial Response ASGI extension #1916
Conversation
It looks like it's related to this PR. I just cancelled the two previous jobs. |
tests/protocols/test_websocket.py
Outdated
| if ws_protocol_cls == WSProtocol: | ||
| # wsproto automatically makes the message chunked | ||
| assert response.headers["transfer-encoding"] == "chunked" | ||
| else: | ||
| # websockets automatically adds a content-length | ||
| assert response.headers["content-length"] == "8" |
There was a problem hiding this comment.
It makes sense for WebSockets to add the content-length, since we can't have a chunked one there.
There was a problem hiding this comment.
well, this is only about what happens if the app doesn't provide a content-length and only provides the raw body data. the defaults for these two protocols are different. Technically, you can provide chunked data over the websockets: The app, provides the "chunked" header, and then provides the chunks, with the appropriate chunk headers (as an application should do if it provides that header (I think). I think that nowadays it is rare for an application to specify content length or transfer encoding...
|
Interesting... I've just found this:
Ref.: https://asgi.readthedocs.io/en/latest/specs/www.html#response-start-send-event Meaning that we shouldn't be sending the 404 if nobody is provided... But that also means that we have an issue with the HTTP implementations 🤔 |
Yes, h11 definitely starts sending the header before receiving the first body. This can be fixed, I can fix both h11 and websockets to wait for the first body... I can do it for websockets in this PR and then a followup one for h11_impl.py |
Well, I haven't touched any of the .github/ code, and this is what the actions log says: |
|
seems things are running smoothly again. there was hanging test due to an introduced bug, but that should not have caused these weird symptoms. Now fixed. |
|
Awesome, thanks! |
kristjanvalur
force-pushed
the
kristjan/ext
branch
from
f7e4982
to
5f41c9c
Compare
kristjanvalur
force-pushed
the
kristjan/ext
branch
from
5f41c9c
to
0ceaa81
Compare
|
I guess there are some changes upstream. Do you want me to update this PR? |
|
Yes. I'll get to this tomorrow. |
Kludex
changed the title
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
There was a problem hiding this comment.
@kristjanvalur Please check my changes, and if we are fine, let's merge this.
| # Create the event here but do not send it, the ASGI spec | ||
| # suggest that we wait for the body event before sending. | ||
| # https://asgi.readthedocs.io/en/latest/specs/www.html#response-start-send-event | ||
| self.reject_event = event |
There was a problem hiding this comment.
We already discussed this: django/asgiref#387
There was a problem hiding this comment.
I know. I disagree, I think the recommendation is a good one because it enlarges the window during which the server can still send a 500 error in case the application messes up. It is not a requirement but (IMHO) good practice in a library, and that is the reason I wrote the code that way.
But it is your call of course, happy to get this merged either way.
There was a problem hiding this comment.
I understand, and it makes sense... But I'll rather not have a mismatch in how the HTTP implementations behave, and the WebSockets.
| @@ -199,7 +199,7 @@ class WebSocketResponseStartEvent(TypedDict): | |||
| class WebSocketResponseBodyEvent(TypedDict): | |||
| type: Literal["websocket.http.response.body"] | |||
| body: bytes | |||
| more_body: bool | |||
| more_body: NotRequired[bool] | |||
There was a problem hiding this comment.
If you say so :), We didn't have those fancy type decorations in the old days.
uvicorn now can return a response from websocket apps that choose to reject a connection with a custom response.
See https://github.com/encode/starlette/pull/2041/files for related work.
This is a re-submission of pr #1907