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

Update WSGIWrapper to be spec compliant (pep 333/3333). #155

Closed
wants to merge 6 commits into from

Conversation

apollo13
Copy link
Contributor

The commit contains the following changes:

  • Error out early if the application forgets to call start_response
  • Send a proper content-length if the iterable returned has only one item
  • Always call close() on the returned iterable if it exists.

This potentially fixes #154. Haven't looked at tests yet, but should provide a base for discussion…

The commit contains the following changes:

 * Error out early if the application forgets to call start_response
 * Send a proper content-length if the iterable returned has only one
   item
 * Always call `close()` on the returned iterable if it exists.
@apollo13
Copy link
Contributor Author

@pgjones Could you reapprove that workflow? I have added a few tests and improved the code so no unnecessary http.response.body events are sent. Let me know what you think.

For what it is worth I'd consider this a rather critical bug. Not calling .close() will not properly clean up connections in Django but might close them during gc collection in subsequent requests, leading to hard to debug errors.

@pgjones
Copy link
Owner

pgjones commented Dec 27, 2023

Thanks, I've taken a different approach in 2d2c62b.

I've tested other implementations and gunicorn and I don't believe any others set the content-length (rather they'll use chunked transfer encoding). So I've decided against this to avoid the complexity. As I understand PEP 3333 considers this optional.

@pgjones pgjones closed this Dec 27, 2023
@apollo13
Copy link
Contributor Author

Mhm, yeah as long as the framework sets Content-Length then gunicorn (and hypercorn) will happily serve it as such without chunking, so I guess the "optimization" would have only applied if the framework didn't set the content length and is probably not worth it. Sorry for opening the PR prematurely, should have waited to hear your thoughts.

@apollo13 apollo13 deleted the wsgi-spec-conformance branch December 27, 2023 19:09
@pgjones
Copy link
Owner

pgjones commented Dec 27, 2023

Thanks for opening the PR, it was very helpful - sorry I didn't merge it directly. I'm often conflicted between accepting the PR as is, and wanting it to match "my style" - I usually try both (merge the PR then add a commit adjusting it).

@apollo13
Copy link
Contributor Author

apollo13 commented Dec 28, 2023 via email

@pgjones
Copy link
Owner

pgjones commented Dec 28, 2023

I can't add any more dependencies if I want Hypercorn to be considered as the replacement for the Werkzeug development server. Happy to work with a2wsgi though and link to it in the docs.

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.

WSGI Adapter does not seem to release resources (spec inconformance)
2 participants