Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Response callbacks not always invoked after exception view #3468

Closed
lrowe opened this issue Feb 27, 2019 · 5 comments · Fixed by #3496
Closed

Response callbacks not always invoked after exception view #3468

lrowe opened this issue Feb 27, 2019 · 5 comments · Fixed by #3496

Comments

@lrowe
Copy link
Contributor

lrowe commented Feb 27, 2019

In Using Hooks it is stated that "Response callbacks are, however, invoked when a exception view is rendered successfully." While this is true when the exception view is invoked by the excview_tween, it may also be invoked by the execution policy in which case the response callbacks are not invoked.

When ab exception is raised under excview_tween the following are executed in order:

request.invoke_exception_view(exc_info)
request._process_response_callbacks(response)
notify(NewResponse(request, response))
request._process_finished_callbacks()

When an exception is raised over excview_tween it is:

request._process_finished_callbacks()
request.invoke_exception_view(reraise=True)

In my case I had an exception view registered that called request.session.changed() to refresh the session cookie. BaseCookieSessionFactory.changed sets the cookie in a response callback so this no longer works.

I'm not sure if this a documentation or a behavioral issue.

@digitalresistor
Copy link
Member

At least in pyramid_retry, the only execution policy we currently have, it is a last ditch effort to try and render the exception. That would fall under the:

No response callback is called if an unhandled exception happens in application code, or if the response object returned by a view callable is invalid.

Part of the docs, since the exception is not a valid response object that is returned. I am not sure it is a good idea for pyramid_retry to call the response call backs since at that point you are far outside of the usual request cycle.

I would question why your exception is falling through to the execution policy of pyramid_retry, instead of being handled by the excview tween.

@digitalresistor
Copy link
Member

The same statements stand for the default execution policy: https://github.com/Pylons/pyramid/blob/master/src/pyramid/router.py#L274

@mmerickel
Copy link
Member

I think in hindsight it was probably a mistake to have the execution policy call invoke_exception_view. I did know it was a thing Pyramid didn't do before, but it seemed safe enough to try and get a response out. This type of inconsistency though feels not great. In general exception views need to be really careful about what they expect [1], since they can be called at weird spots in the lifecycle. It's unclear what a good solution is though.

[1] For example, pyramid_tm adds a tm_active predicate which exception views can use to ensure they only match if the transaction manager is still active. This type of predicate isn't supported for other things like request.session though.

@mmerickel
Copy link
Member

I should point out that pyramid_retry has this behavior builtin as well because it operates outside of the request lifecycle but does call invoke_exception_view. Does anyone have any suggestions what to do about this? If not, it'll probably stay as-is and people will need to be careful what they do in exception views.

I'd be open, at the very least, to changing Pyramid's default execution policy back to not calling invoke_exception_view.

@mmerickel
Copy link
Member

mmerickel commented Jun 7, 2019

I have pushed changes for this to #3496 and also to pyramid_retry. I'm hesitant to backport this as it's a sort of bw-incompat change but you could probably talk me into it. It'll at least be in 2.0. In the meantime you can fix it in your apps by defining your own excecution policy.

def my_execution_policy(environ, router):
    with router.request_context(environ) as request:
        return router.invoke_request(request)

config.set_execution_policy(my_execution_policy)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants