Default WebSocket accept message headers to an empty list

[Klavionik]

Recently, #1361 introduced a possibility to pass a set of extra headers into the accept method. If no headers are passed, the default value is None. This value then gets sent to the protocol server.

This causes Uvicorn to raise an exception: TypeError: NoneType object is not iterable when using websockets and TypeError: can only concatenate list (not "NoneType") to list when using wsproto.

This PR fixes this, setting the headers argument to an empty list if it is not passed into the method. This is recommended by the ASGI spec (https://asgi.readthedocs.io/en/latest/specs/www.html#accept-send-event).

[aminalaee]

Thank you for this. I guess adding/editing the test would be valuable here so this is safe for future changes.
Maybe in the test_additional_headers test?

[Klavionik]

I've added a test, but I'm really not sure how adequate it is to do it that way. :) Just let me know if it's not.

[Kludex]

My interpretation of the ASGI spec is that the server will interpret the absence of headers as an empty array, not the application. If my understanding is correct, I think we should send the message without headers in this case.

[aminalaee]

Yes, I agree that's what it says. Wouldn't that be a breaking change? I mean that's like the case with code.

[Kludex]

No, because we didn't release it yet. This is a feature introduced a few days ago.

[aminalaee]

Oh that makes sense, I thought it was from the last release 😆

[Kludex]

Should we also change the TestClient.extra_headers default value as well? #1361 (comment)

[Klavionik]

@Kludex I see your point, but I'm not really sure about it, to be honest.

My humble reflection on this topic:

why should an application care about how exactly the server will interpret the absence of headers - be it an empty list or anything else? Why should the spec tell us about that? I'm leaning towards thinking that this is an instruction for application developers so that server developers would know what to expect. And in accordance to that, right now Uvicorn expects headers to be something iterable.

Anyway, if so, shouldn't we also remove the subprotocol too, when it is None? It's a similar case, I guess.

[Kludex]

And in accordance to that, right now Uvicorn expects headers to be something iterable.

Uvicorn only expects the headers to be iterable if present.

Anyway, if so, shouldn't we also remove the subprotocol too, when it is None? It's a similar case, I guess.

But yeah, you have a point. Ok, I'm fine with this 👍

[matiuszka]

@Klavionik Thanks for fixing that. I completely forgot about that.

[aminalaee]

Anyway, if so, shouldn't we also remove the subprotocol too, when it is None? It's a similar case, I guess.

I think we should pick one approach and stick with it.
In some cases we do send the optional values even if they are None, like subprotocol and code.
In some other places like the reason we don't send the value if it's None (still under review).

[Kludex]

Anyway, if so, shouldn't we also remove the subprotocol too, when it is None? It's a similar case, I guess.

I think we should pick one approach and stick with it. In some cases we do send the optional values even if they are None, like subprotocol and code. In some other places like the reason we don't send the value if it's None (still under review).

We send None when it's allowed by the ASGI spec, but yeah. I agree. We should probably adapt the reason PR to stick with the others. 🤔

[Klavionik]

I accepted all the suggestions (first time using it, hope I did it right).

@Klavionik Thanks for fixing that. I completely forgot about that.

Happy to help! I was actually tinkering with BlackSheep, trying to add support for WebSocket connections to it, when I discovered this. I was referencing Starlette and did the same thing with the accept method. Luckily I was testing it using the latest Uvicorn release - that's how I saw the server crashing on the accept call.

So maybe we should consider adding some kind of check to Uvicorn? Like assert isinstance(headers, typing.Iterable), "The 'headers' argument must be an iterable."? Just to make sure it will raise a more clear exception if someone tries to do the same thing.

[Kludex]

Thanks @Klavionik ! 🎉