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

WsgiToAsgi should stop sending response body after content-length bytes #195

Closed
kmichel opened this issue Sep 14, 2020 · 1 comment
Closed

Comments

@kmichel
Copy link
Contributor

kmichel commented Sep 14, 2020

Hi,

I was having issues when serving partial requests (using Range headers) from whitenoise wrapped in WsgiToAsgi. Some HTTP and ASGI servers in front of that complained about extra content in the response body (I've had the issue with traefik in front of uvicorn and with uvicorn itself when using the httptools protocol handler).

One contributing factor is whitenoise which returns the file handle to the whole file in a wsgiref.FileWrapper, seeked to the correct position for the range request. It then expects the server to only read as many bytes as defined in the Content-Length header.

The WSGI specs allows for this behavior and gunicorn implements it and stops reading after as many bytes as specified in Content-Length.

The ASGI spec does not say much about the consistency between Content-Length header and the "http.response.body" events. The relevant parts could be :

Protocol servers must flush any data passed to them into the send buffer before returning from a send call.

Yielding content from the WSGI application maps to sending http.response.body messages.

The first one suggests that any piece of body from an "http.response.body" event must be sent, the other one could mean that all behavior defined for WSGI should apply to ASGI, including not-sending extra body, maybe ?

uvicorn, daphe and hypercorn lean more towards the first interpretation and pass through the whole readable content or simply raise an exception.

If you agree that the WsgiToAsgi adapter should clamp the body I can work on a patch.

Aside from this, the ASGI spec could probably be improved by defining this point. I don't really know if ASGI servers should prefer clamping or forwarding extra body responses but I have the vague feeling that allowing ASGI events to generate incorrectly framed HTTP messages could generate hard to diagnose issues in the future without providing any benefit.

If the sendfile extension from #184 is added, the same point will probably arise, it would be very inconvenient to only be able to use sendfile when serving whole files. WSGI also specifies the clamping behavior when using sendfile and gunicorn implements it.

@andrewgodwin
Copy link
Member

I consider the fact that WSGI parses the content-length header a slightly weird behaviour, so that's why it's not in the core ASGI spec - however, I think this is definitely a bug in WsgiToAsgi since that should absolutely obey the WSGI spec. Don't think it should be hard to fix that up.

As for whether there should be clamping in WSGI itself... I'm not sure. I could definitely be convinced, but I also feel it's a little bit of a specialisation.

The zerocopy extension actually has a count option in it so you can explicitly say that you only want so many bytes read from the file descriptor, so it wouldn't be needed for that use case. But, as you say, allowing people to make invalid HTTP responses is also not great.

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

No branches or pull requests

2 participants