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

Fix support for generator-based WSGI apps #887

Merged
merged 15 commits into from
Mar 29, 2020

Conversation

Singletoned
Copy link
Contributor

The WSGI adapter was trying to take the status code from start_response straightaway, rather than waiting until the first non-empty chunk, as per the spec: https://www.python.org/dev/peps/pep-3333/#the-start-response-callable

I've made a small change that iterates through the response until it gets a non-empty chunk, and then passes everything on as normal. I've also added a test for that scenario.

@florimondmanca florimondmanca requested a review from a team March 28, 2020 21:02
Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks! I checked out the code locally and it looks good (tests are failing without the fix). Left some suggestions here to reuse existing tests.

httpx/_dispatch/wsgi.py Outdated Show resolved Hide resolved
httpx/_dispatch/wsgi.py Outdated Show resolved Hide resolved
tests/client/test_client.py Outdated Show resolved Hide resolved
@Singletoned
Copy link
Contributor Author

I removed the "Content-length" header from the tests because it isn't actually used anywhere in the code, and if people look at the tests as an example, it might be misleading.

Copy link
Contributor

@yeraydiazdiaz yeraydiazdiaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Singletoned! Just left a question regarding an edge case

Relevant section of PEP 3333 for reference:

However, the start_response callable must not actually transmit the response headers. Instead, it must store them for the server or gateway to transmit only after the first iteration of the application return value that yields a non-empty bytestring, or upon the application's first invocation of the write() callable. In other words, response headers must not be sent until there is actual body data available, or until the application's returned iterable is exhausted. (The only possible exception to this rule is if the response headers explicitly include a Content-Length of zero.)

This delaying of response header transmission is to ensure that buffered and asynchronous applications can replace their originally intended output with error output, up until the last possible moment. For example, the application may need to change the response status from "200 OK" to "500 Internal Error", if an error occurs while the body is being generated within an application buffer.

httpx/_dispatch/wsgi.py Show resolved Hide resolved
httpx/_dispatch/wsgi.py Outdated Show resolved Hide resolved
tests/test_wsgi.py Outdated Show resolved Hide resolved
tests/test_wsgi.py Outdated Show resolved Hide resolved
httpx/_dispatch/wsgi.py Show resolved Hide resolved
Singletoned and others added 3 commits March 29, 2020 11:47
Co-Authored-By: Florimond Manca <florimond.manca@gmail.com>
Co-Authored-By: Florimond Manca <florimond.manca@gmail.com>
Co-Authored-By: Florimond Manca <florimond.manca@gmail.com>
Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All set on my side once this last fix is in! 👍

Thank you so much!

httpx/_dispatch/wsgi.py Outdated Show resolved Hide resolved
Co-Authored-By: Florimond Manca <florimond.manca@gmail.com>
@florimondmanca florimondmanca changed the title Handle generator WSGI app Fix support for generator-based WSGI apps Mar 29, 2020
@florimondmanca florimondmanca merged commit 94323f9 into encode:master Mar 29, 2020
@tomchristie tomchristie mentioned this pull request May 21, 2020
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

Successfully merging this pull request may close these issues.

None yet

3 participants