Support websocket.http.response ASGI extension

[kristjanvalur]

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.

Note: The websockets module currently does not read the response body from a websockets upgrade requests, so we cannot effectively test that functionality, in the unittests. The exception thrown at the client contains no body, only status code.

[kristjanvalur]

I don't see hypercorn sending any messages to the app:
https://github.com/pgjones/hypercorn/blob/8ae17ca68204d9718389fb3649ca0ed6ba851906/src/hypercorn/protocol/ws_stream.py#L322

[Kludex]

I don't have more comments here. Beautiful PR, beautiful tests. 👍

Thanks @kristjanvalur 🙏

For me, before approving:

• Check if all lines are covered.
• Check Andrew's reply to the disconnect message.

[kristjanvalur]

Please see test test_server_reject_connection_with_invalid_msg()

I am unable to silence the wsproto internal error which happens when it is trying to send a 500 error.

1. wsproto will start a response as soon as it gets the .start message
2. It then gets an invalid message and attempts to send a 500 response
3. but it cannot because it has already started a response.
4. An exception is not raised via the async send() method but somewhere else which I cannot fully understand, so the test fails.

I've added a skip for the WSProto case, but maybe there is a better solution?
Skipping is not ideal, because this test was designed to increase coverage.

[kristjanvalur]

A separate question: The websockets library does not read the response body from a websocket upgrade request. Do you think it is worth making a PR against that repo to add that functionality?

[Kludex]

A separate question: The websockets library does not read the response body from a websocket upgrade request. Do you think it is worth making a PR against that repo to add that functionality?

That would be great 👍

First ask the maintainer if he would accept such a PR, please 👀

[Kludex]

Please see test test_server_reject_connection_with_invalid_msg()

I am unable to silence the wsproto internal error which happens when it is trying to send a 500 error.

1. wsproto will start a response as soon as it gets the .start message
2. It then gets an invalid message and attempts to send a 500 response
3. but it cannot because it has already started a response.
4. An exception is not raised via the async send() method but somewhere else which I cannot fully understand, so the test fails.

I've added a skip for the WSProto case, but maybe there is a better solution? Skipping is not ideal, because this test was designed to increase coverage.

We should have the analogous behavior that we have when using http.response.start and http.response.body. What's the behavior with those events (I think they should be the same on httptools and h11)?

[kristjanvalur]

We should have the analogous behavior that we have when using http.response.start and http.response.body. What's the behavior with those events (I think they should be the same on httptools and h11)?

Okay, I looked at the similar test for http (test_duplicate_start_message) and it, in fact, asserts that it does not get a 500 response, precisely because a 200 response has been previously written into the buffer.

for websockets, and the websockets protocol, we can't send the response incrementally. Instead, we prepare it and return it all at once. That means that we can in fact issue a 500 error in this case.

It is not a problem to modify the test for both cases.
The problem remains for me, how to run the test with wsproto, because it fails in some weird way. I'll try some things.

[kristjanvalur]

It is not a problem to modify the test for both cases.
The problem remains for me, how to run the test with wsproto, because it fails in some weird way. I'll try some things.

Turns out that we needed extra tests to check if response had been started before attempting to send a 500 response, similar to what is done in h11.

[kristjanvalur]

seems like workflows are disabled 🤔

Checking pipeline

[Kludex]

I cancelled a pipeline yesterday that it was not able to stop, maybe it's related to that. I don't know how to enable it again here.

[kristjanvalur]

Weird, yes, the last time a workflow ran in this PR it timed out.

[Kludex]

You can try to reopen a new PR. 🤔