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

merge app and request contexts into a single context #5639

Open
davidism opened this issue Nov 13, 2024 · 3 comments
Open

merge app and request contexts into a single context #5639

davidism opened this issue Nov 13, 2024 · 3 comments
Assignees
Milestone

Comments

@davidism
Copy link
Member

Right now we have two separate contexts managed separately, the app and request contexts. This makes the implementation pretty complicated, as we need to maintain two context var stacks, and do a bunch of checks in the request context to make sure we're managing the correct app context. It makes an already confusing topic more complicated to explain: app context is active for requests and cli commands, don't push an app context before making a request, etc.

I think merging the two contexts could be possible. The single context (ExecutionContext?) would have the g, request, and session attributes, but accessing request or session when not in a request would raise an error.

@davidism davidism self-assigned this Nov 13, 2024
@davidism
Copy link
Member Author

I was never clear on why, but the contexts support being pushed multiple times. If the same context is pushed multiple times, the teardown functions are only run once it's fully popped. I don't think I've ever seen this used, and I can't think of a use case. There is a test, but it just demonstrates that the system works, it doesn't demonstrate any intended use of it. There's no docs about it. Perhaps it's a holdover from how things were tracked as a stack on top of thread locals, before things were refactored?

Having to keep track of a stack of how many times a context has been pushed complicates the implementation, especially when the request context also has to track whether it had to push an app context as well or if one was already present. It would be much easier (and probably faster and less memory) to error if the context is currently pushed.

This is not the same as being able to push different contexts on top of each other.

@davidism davidism added this to the 3.2.0 milestone Nov 23, 2024
@davidism
Copy link
Member Author

Tentatively marking this for 3.2. I think I can do the merge and have uses of the old contexts issue deprecation warnings and redirect to the new context. Then it can be fully removed in a later release, maybe 4.0.

@davidism
Copy link
Member Author

So far I've still been using the "app context" name for the new merged context. Couldn't think of a better term.

teardown_appcontext and teardown_request need to be combined, but they currently run at different times. teardown_appcontext runs in a finally block after teardown_request was attempted, and after request is no longer bound. The signals appcontext_tearing_down and request_tearing_down share the same timing.

I'd prefer to move away from the squished word "appcontext" to teardown_context and context_tearing_down. It's not clear whether teardown_appcontext or teardown_request is more commonly used right now, so it would probably be equally disruptive either way, since one of them is also getting removed.

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

No branches or pull requests

1 participant