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

WSGI support is not spec conformant #29

Closed
apollo13 opened this issue Jan 7, 2023 · 8 comments
Closed

WSGI support is not spec conformant #29

apollo13 opened this issue Jan 7, 2023 · 8 comments
Labels
bug Something isn't working
Milestone

Comments

@apollo13
Copy link

apollo13 commented Jan 7, 2023

I wanted to give granian a try but WSGI support seems to be somewhat out of spec. Eg try:

class Response:
    def __init__(self, parts):
        self.parts = parts

    def __iter__(self):
        return self

    def __next__(self):
        try:
            return self.parts.pop(0)
        except Exception:
            raise StopIteration

    def close(self):
        print("CLOSE CALLED")

def app(env, start_response):
    start_response('200 OK', [('Content-Type','text/html')])
    return Response([b"Hello", b" ", b"World"])

When running this, "Hello World" is returned properly but .close() is not called, see https://peps.python.org/pep-3333/#specification-details

@apollo13
Copy link
Author

apollo13 commented Jan 7, 2023

Using

def app(env, start_response):
    start_response('200 OK', [('Content-Type','text/html'), ('Content-Length', '300')])
    return Response([b"Hello", b" ", b"World"])

should not hang the server, from https://peps.python.org/pep-3333/#handling-the-content-length-header

Of course, if the application does not provide enough data to meet its stated Content-Length, the server should close the connection and log or otherwise report the error.

@apollo13
Copy link
Author

apollo13 commented Jan 7, 2023

start_response must return a write(body_data) callable, currently it returns None. Note that this mostly for older frameworks but the spec still requires it afaik.

start_response also need to support an optional exc_info

@apollo13
Copy link
Author

apollo13 commented Jan 7, 2023

If I am reading the code correctly, the whole response is also evaluated at once:

return (resp.status, resp.headers, b"".join(rv))

It would be better to write that back in a streaming fashion.

@cirospaciari
Copy link
Contributor

Using

def app(env, start_response):
    start_response('200 OK', [('Content-Type','text/html'), ('Content-Length', '300')])
    return Response([b"Hello", b" ", b"World"])

should not hang the server, from https://peps.python.org/pep-3333/#handling-the-content-length-header

Of course, if the application does not provide enough data to meet its stated Content-Length, the server should close the connection and log or otherwise report the error.

https://peps.python.org/pep-3333/#handling-the-content-length-header also says

Under some circumstances, however, the server or gateway may be able to either generate a Content-Length header, or at least avoid the need to close the client connection. If the application does not call the write() callable, and returns an iterable whose len() is 1, then the server can automatically determine Content-Length by taking the length of the first bytestring yielded by the iterable.

So I think you can also ignore Content-Length and automatically determine Content-Length, instead of closing the connection (with is a more pleasing experience for everyone I think)

@apollo13
Copy link
Author

apollo13 commented Jan 7, 2023

No you shouldn't ignore the Content-Length header, if it is there you are supposed to use it. This is especially important for streaming larger respones where the iterable doesn't have a length of 1 (like in my example above). That said, my example is flawed since I do not implement support for len() at all -- that said I do not think that granian checks this anywhere currently.

My example about closing the connection was mostly about setting content length to 300 when the actual payload was "hello world" (ie shorter than 300). In this case the server has no good option but to close the connection since it would already have written "hello world" to the wire (in an ideal case) when it realizes that the rest of the data does not come.

@cirospaciari
Copy link
Contributor

No you shouldn't ignore the Content-Length header, if it is there you are supposed to use it. This is especially important for streaming larger respones where the iterable doesn't have a length of 1 (like in my example above). That said, my example is flawed since I do not implement support for len() at all -- that said I do not think that granian checks this anywhere currently.

My example about closing the connection was mostly about setting content length to 300 when the actual payload was "hello world" (ie shorter than 300). In this case the server has no good option but to close the connection since it would already have written "hello world" to the wire (in an ideal case) when it realizes that the rest of the data does not come.

You are right.

@gi0baro gi0baro added this to the 0.2 milestone Jan 12, 2023
@gi0baro gi0baro added the bug Something isn't working label Jan 12, 2023
@gi0baro
Copy link
Member

gi0baro commented Jan 13, 2023

The issue with closing the response iterator is now fixed, so I'm closing this.

In regards to supporting iteration over responses, you can open up a dedicated issue.

@gi0baro gi0baro closed this as completed Jan 13, 2023
@apollo13
Copy link
Author

apollo13 commented Jan 13, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants