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

let an http-exception object that ->can("as_psgi") recieve the psgi $env... #440

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jjn1056
Copy link

@jjn1056 jjn1056 commented Dec 26, 2013

... as its first argument

The idea here is that if the exception object is doing its own as_psgi, it might which to have access to $env so as to perform some content negotiation and provide the acceptable body content. Or for any other reasons. I don't think this change will cause any issues:

https://metacpan.org/pod/HTTP::Throwable#as_psgi

Says will accept $env but currently that is not used, so this change will work into the expectation I think.

HTTP::Exception does not have the as_psgi method so there's no compatibility problems.

I realized that the code that throws the exception object will also have $env and it is assumed that one could also do some content negotiation/etc there as well, but I think this change would make it easier for anyone working on a general framework to set some sane defaults.

code/test case/doc patch all included.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) when pulling 73830eb on jjn1056:master into 7dafe51 on plack:master.

@jjn1056
Copy link
Author

jjn1056 commented Dec 26, 2013

I added a minor test case for when the as_psgi response was a code ref since I was playing with this and didn't see that one already existed.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) when pulling b0b89a8 on jjn1056:master into 7dafe51 on plack:master.

@jjn1056
Copy link
Author

jjn1056 commented Dec 26, 2013

So that last commit is actually a pathological case that I discovered while trying to port Catalyst to use this middleware. What I found was that if the exception is raised inside a delayed style response, and that exception does the as_psgi method which was also using delayed or streaming, the code didn't unroll the reposes far enough. I added code here to recursively unroll the responses until no more errors occurred.

We could add a 'max recursive' limit if the recursion here was scary. Basically now an exception that does as_psgi could itself have an error that did as_psgi, and so on.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) when pulling 00e2a93 on jjn1056:master into 7dafe51 on plack:master.

@jjn1056
Copy link
Author

jjn1056 commented Dec 26, 2013

Added docs and test cases to cover the expected behavior as discussed on IRC

@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) when pulling b419595 on jjn1056:master into 7dafe51 on plack:master.

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.

2 participants