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

start_response() reraises exc_info even if headers have not been sent #51

Closed
eli-collins opened this Issue Dec 17, 2013 · 4 comments

Comments

Projects
None yet
4 participants
@eli-collins
Contributor

eli-collins commented Dec 17, 2013

While working on a wsgi error handler, I think I ran across a (small) bug in waitress' start_response function...

PEP3333 says start_response(status, headers, exc_info) should only re-raise the exception if the headers haven't been sent to the client. Whereas WSGITask.execute() re-raises the error if start_response() has been called at all, even if the headers haven't been sent.

The following quick patch seems to fix things... (my apologies, I'm new to github, and can't for the life of me figure out how to make an attachment).

diff --git a/waitress/task.py b/waitress/task.py
index a4c8f2e..ab3282f 100644
--- a/waitress/task.py
+++ b/waitress/task.py
@@ -346,7 +346,7 @@ class WSGITask(Task):
                                      "without providing exc_info.")
             if exc_info:
                 try:
-                    if self.complete:
+                    if self.wrote_header:
                         # higher levels will catch and handle raised exception:
                         #1. "service" method in task.py
                         #2. "service" method in channel.py
@kgaughan

This comment has been minimized.

Show comment
Hide comment
@kgaughan

kgaughan Dec 18, 2013

Member

Hi, Eli!

First up, thanks! I'm in work, so somebody else will likely deal with the meat of the issue, but I though I'd cover how submissions are usually handled through GitHub.

The typical way of submitting a change in GitHub is to fork the repository into your own profile, create a branch, make and test the change, and then issue a pull request. The pull request appears as an issue on the original repo's issue tracker, and, if everything's good, can be merged with the click of a button. The nice thing about it is that any changes make on the source branch with the changes submitted in the pull request will appear as part of the pull request subsequently, so if there are any issues, there's not need to perform any further edits.

I don't suppose you could knock together a quick test case to demonstrate the issue? If not, I'll see what I can do after work if nobody else has dealt with the issue.

Member

kgaughan commented Dec 18, 2013

Hi, Eli!

First up, thanks! I'm in work, so somebody else will likely deal with the meat of the issue, but I though I'd cover how submissions are usually handled through GitHub.

The typical way of submitting a change in GitHub is to fork the repository into your own profile, create a branch, make and test the change, and then issue a pull request. The pull request appears as an issue on the original repo's issue tracker, and, if everything's good, can be merged with the click of a button. The nice thing about it is that any changes make on the source branch with the changes submitted in the pull request will appear as part of the pull request subsequently, so if there are any issues, there's not need to perform any further edits.

I don't suppose you could knock together a quick test case to demonstrate the issue? If not, I'll see what I can do after work if nobody else has dealt with the issue.

@eli-collins

This comment has been minimized.

Show comment
Hide comment
@eli-collins

eli-collins Dec 18, 2013

Contributor

Hi! Thanks for the pointers. I've file a pull request, and included unittests to detect the issue (sorry this seems to have created two issues entries through).

Contributor

eli-collins commented Dec 18, 2013

Hi! Thanks for the pointers. I've file a pull request, and included unittests to detect the issue (sorry this seems to have created two issues entries through).

@tseaver

This comment has been minimized.

Show comment
Hide comment
@tseaver

tseaver Dec 18, 2013

Member

Will be resolved with the merge of #52.

Member

tseaver commented Dec 18, 2013

Will be resolved with the merge of #52.

@tseaver tseaver closed this Dec 18, 2013

@mcdonc

This comment has been minimized.

Show comment
Hide comment
@mcdonc

mcdonc Jul 14, 2014

Member

Good catch. I've merged a derivation of this via #52. Can you supply another pull request that adds your name to CONTRIBUTORS.txt?

Member

mcdonc commented Jul 14, 2014

Good catch. I've merged a derivation of this via #52. Can you supply another pull request that adds your name to CONTRIBUTORS.txt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment