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

add a router.request_context context manager #3086

Merged
merged 1 commit into from
Jun 20, 2017

Conversation

mmerickel
Copy link
Member

@mmerickel mmerickel commented Jun 15, 2017

the context manager may be used by execution policies to push/pop threadlocals if necessary

@mmerickel mmerickel force-pushed the set-execution-context branch from 912c5ef to d3af568 Compare June 15, 2017 07:00
@mmerickel
Copy link
Member Author

mmerickel commented Jun 15, 2017

I'm a little iffy on whether I should add this to the router interface or simply provide this as a new API for managing threadlocals. Currently pyramid provides config.begin() and config.end() as a way to manage threadlocals at config-time. There are also quite a few other places in the code that manage threadlocals:

  • config.begin
  • router.invoke_request
  • router.execution_context (in this PR)
  • request.invoke_exception_view
  • pyramid.scripting.prepare (used by pyramid.paster.bootstrap)
  • pyramid.scripting.get_root

Arguably every single one of these could be replaced with a common API such as with ExecutionContext(registry, request=None). The main reason I'm thinking about this right now is that router.execution_context just feels not quite right because the context itself does not depend on the router - it's just a way to keep semi-hiding what's going on under the hood.

This is the only thing holding up the 1.9 release and I would really appreciate some input.

@digitalresistor
Copy link
Member

I think adding a new ExecutionContext API that does the threadlocal pushing is a much better idea than shoe-horning it into the router.

The fact that each of those places where we currently push the threadlocals manually can now use the API is great too since it means we have one place and only one place where we need to actually do verification that threadlocals are being pushed and popped.

There may be other areas where someone might want to push/pop the threadlocals, and right now they would be entering private API territory, with a new API they can do what they need to do.

Just thinking about tweens that might want to potentially render a different request as part of the pipeline and thus make sure the threadlocals are correct, or other such use cases.

I am a 👎 on this current PR as it stands, it feels wrong. I've been thinking about this for the past couple of days and even-though it's a pain to have yet another API, I think it is a good idea to introduce it.

@mmerickel
Copy link
Member Author

I've decided not to go that route because I prefer Pyramid's API to force users to do the right thing when possible versus just adding an API they could use. I'm going to keep things about the same but with a couple changes:

  1. router.invoke_request will no longer push threadlocals - instead we will force the user to do it inside the policy.
  2. router.make_request will be router.request_context instead and will return a context. The simple policy will then be the following, with the option of instead using ctx.begin and ctx.end instead of the with-statement if desired:
def default_execution_policy(environ, router):
    with router.request_context(environ) as request:
        try:
            return router.invoke_request(request)
        except Exception:
            return request.invoke_exception_view(reraise=True)

the request context is to be used by execution policies to push/pop
threadlocals and access the created request
@mmerickel mmerickel force-pushed the set-execution-context branch from d3af568 to 8226534 Compare June 18, 2017 04:53
@mmerickel
Copy link
Member Author

@bertjwregeer please review this again and let me know what you think.

@mmerickel mmerickel changed the title add a router.execution_context context manager add a router.request_context context manager Jun 18, 2017
@mmerickel mmerickel merged commit 22e61f1 into Pylons:master Jun 20, 2017
mmerickel added a commit that referenced this pull request Jun 20, 2017
Copy link
Member

@digitalresistor digitalresistor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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