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: improve error message when returning without start_response #444

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AGSPhoenix
Copy link
Contributor

Today I discovered that if you return from your WSGI function without calling start_response() first, you get a rather unhelpful traceback:

Traceback (most recent call last):
  File "your-python/site-packages/eventlet/wsgi.py", line 515, in handle_one_response
    'Content-Length' not in [h for h, _v in headers_set[1]]:
IndexError: list index out of range

That you have to call start_response() before returning is WSGI 101, but I probably should have been asleep a few hours ago, so it took me a while to work backwards from this traceback to the error in my code. This patch should be a relatively simple change that makes it clear what the issue is.

@AGSPhoenix
Copy link
Contributor Author

Just took a look at the CI results, and this definitely breaks some things. Given the number of failed tests, I looked over PEP-333 to check my assumption about start_response being required and found this:

(Note: the application must invoke the start_response() callable before the iterable yields its first body string, so that the server can send the headers before any body content. However, this invocation may be performed by the iterable's first iteration, so servers must not assume that start_response() has been called before they begin iterating over the iterable.)

...which is exactly what my PR does. I'd still like for this to be changed, but it's late and I don't have any clever ideas for how to go about it. Moving this to an issue.

@temoto
Copy link
Member

temoto commented Nov 2, 2017

Thank you, I'll take it from here.
Maybe we had invalid tests that didn't call start_response.
Issue/PR is very same thing to me, so in future don't bother with bureaucracy.

@AGSPhoenix AGSPhoenix reopened this Nov 2, 2017
@codecov-io
Copy link

Codecov Report

Merging #444 into master will increase coverage by 6%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #444     +/-   ##
=======================================
+ Coverage      45%    51%     +6%     
=======================================
  Files          89     84      -5     
  Lines       11223   9674   -1549     
  Branches     1982   1687    -295     
=======================================
- Hits         5132   5017    -115     
+ Misses       5748   4319   -1429     
+ Partials      343    338      -5
Flag Coverage Δ
#py26epolls 7% <ø> (-1%) ⬇️
#py26poll 7% <ø> (-1%) ⬇️
#py26selects 7% <ø> (-1%) ⬇️
#py27epolls 55% <ø> (ø) ⬆️
#py27poll 55% <ø> (ø) ⬆️
#py27selects 54% <ø> (ø) ⬆️
#py33epolls 48% <ø> (ø) ⬆️
#py33poll 48% <ø> (ø) ⬆️
#py33selects 47% <ø> (ø) ⬆️
#py34epolls 48% <ø> (ø) ⬆️
#py34poll 48% <ø> (ø) ⬆️
#py34selects 47% <ø> (ø) ⬆️
#py35epolls 48% <ø> (ø) ⬆️
#py35poll 48% <ø> (ø) ⬆️
#py35selects 47% <ø> (ø) ⬆️
#py36epolls 48% <ø> (ø) ⬆️
#py36poll 48% <ø> (ø) ⬆️
#py36selects 47% <ø> (ø) ⬆️
Impacted Files Coverage Δ
eventlet/backdoor.py 87% <0%> (-7%) ⬇️
eventlet/websocket.py
eventlet/__init__.py
eventlet/patcher.py
eventlet/greenthread.py
eventlet/greenio/base.py 88% <0%> (ø) ⬆️
eventlet/zipkin/patcher.py 47% <0%> (+19%) ⬆️
eventlet/zipkin/greenthread.py 90% <0%> (+65%) ⬆️
examples/websocket.py 81% <0%> (+81%) ⬆️
eventlet/zipkin/_thrift/zipkinCore/__init__.py 97% <0%> (+88%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b8f5f5...4a77dee. Read the comment docs.

@AGSPhoenix
Copy link
Contributor Author

Would it make sense to move the Content-Length check and warning about start_response() after the first read of the returned data, but before any data is sent? That should satisfy the spec, although simply moving it into the for loop seems clunky.

@temoto
Copy link
Member

temoto commented Nov 2, 2017

Ah, now I think AssertionError is what has broken tests. It's an exception raised with assert statement. Try plain Exception or create custom subclass, though I don't believe it could be helpful.

Also, you're breaking all generator applications, because their call returns instantly and they execute only while iterating result. So, yes, moving start_response check into loop is not only sensible, it's required to support generator WSGI apps. Which every performance obsessed application must be.

Moving content-length check is orthogonal to this matter. It would be better for sure. But please make it a separate patch/commit.

@itamarst itamarst added the bug label Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants