The Wayback Machine - https://web.archive.org/web/20221110125238/https://github.com/pallets/flask/issues/2520
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

Don't create View instances for each request #2520

Closed
davidism opened this issue Nov 14, 2017 · 19 comments · Fixed by #4624
Closed

Don't create View instances for each request #2520

davidism opened this issue Nov 14, 2017 · 19 comments · Fixed by #4624
Assignees
Milestone

Comments

@davidism
Copy link
Member

davidism commented Nov 14, 2017

View.as_view creates a function that instantiates the class every request. This seems inefficient for no real gain.

class DeleteView(View):
    methods = ['DELETE']

    # this only needs to happen once
    def __init__(self, model):
        self.model = model
        # imagine if we did something expensive here
        # like generating a form based on the model

The only argument I could think of for instantiating every request is if the constructor does something different based on the request. That seems like bad design, but any use that I can think of for that could be accomplished with properties instead.

Yes, there are ways to make it less expensive, like generating more ahead of time and passing more arguments, but that is not intuitive, and makes the class just a more complicated view factory.

@ThiefMaster
Copy link
Member

ThiefMaster commented Nov 14, 2017

I don't see how this can safely be changed without risking to break people's code if they store stuff on self e.g. because they call other methods in the same class and pass some thing via attributes instead of arguments.

Besides that, instantiating a class is quite cheap...

@davidism
Copy link
Member Author

davidism commented Nov 14, 2017

It's not cheap if you do something expensive in the constructor, which until I dug into the implementation was my intuition of how to use them.

@davidism
Copy link
Member Author

davidism commented Nov 14, 2017

Storing things on self from other methods is equivalent to storing globals, which would break for the usual reasons.

@ThiefMaster
Copy link
Member

ThiefMaster commented Nov 14, 2017

only if an instance is reused for more than one request. as long as the instance is request-scoped, storing stuff on self is like setting a local variable in a normal view function

@davidism
Copy link
Member Author

davidism commented Nov 14, 2017

Ah, I see. I wonder how common that is? Or at least how many are written in such a way that they would break with a single instance.

I'd rather change the behavior and put a big warning in the changelog / docs that they should store data on g.

@davidism
Copy link
Member Author

davidism commented Nov 14, 2017

Maybe add a every_request=True arg to as_view, and issue a deprecation warning that it will change to False in the future.

@ThiefMaster
Copy link
Member

ThiefMaster commented Nov 14, 2017

If we add this I would rather go for this thing to always be configurable or even better add a SingletonView base class that is instantiated once. Like this you can pick what suits you more instead of having

@davidism
Copy link
Member Author

davidism commented Nov 14, 2017

I'll have to think on how we could do SingletonView without copying the View.as_view code, which would be harder to maintain. But I'm ok with it in principal, assuming we change the docs to push it as the normal choice and View as a more advanced choice.

@jeffwidman
Copy link
Contributor

jeffwidman commented Nov 14, 2017

Isn't storing stuff on self already discouraged in favor of storing stuff on g?

Personally, I'd rather move people towards a best practice and worry less about breaking their code, as long as we do it in a major version bump (and the upcoming 1.0 feels like an ideal time).

@ThiefMaster
Copy link
Member

ThiefMaster commented Nov 14, 2017

g is something to use across the code (e.g. for stuff like the current user). Putting application data into g that is used within a single class seems pretty ugly to me.

@justanr
Copy link

justanr commented Nov 15, 2017

@davidism

I'll have to think on how we could do SingletonView without copying the View.as_view code, which would be harder to maintain. But I'm ok with it in principal, assuming we change the docs to push it as the normal choice and View as a more advanced choice.

I'd be okay with this, as View allows you to do stuff like request based DI into a view (e.g. make this instance if X otherwise make that one), plus self is cheap storage if the view class has multiple methods.

This call here can be modified in the SingletonView to create the instance and store the reference onto the class, something like:

class SingletonView(View):
    instances = {}

    @classmethod
    def as_view(cls, name, *class_args, **cls_kwargs):
        view = super().as_view(cls, name, *class_args, **kwargs)
        
        def singleton_view(*cls_args, **cls_kwargs):
            view_inst = cls.instances.get(name)
             if not view_inst:
                 view_inst = cls.instances[name] = cls(*cls_args, **cls_kwargs)
             return view_inst

        view.view_cls = singleton_view
        return view

That's just a rough draft idea, but something along those lines would not create a new instance for each request and allow the view class to be created for multiple end points.

@fcracker79
Copy link

fcracker79 commented Nov 15, 2017

What about object pooling?
This way no such code change will be required to benefit from such an improvement.

@davidism
Copy link
Member Author

davidism commented Nov 15, 2017

You'll need to explain what you mean.

@mitsuhiko
Copy link
Member

mitsuhiko commented Dec 30, 2017

This was done intentionally because people can stuff state on self between functions. It matches the behavior in Django.

@davidism
Copy link
Member Author

davidism commented Dec 31, 2017

We could probably do something with __setattr__, similar to how the Flask object prevents setup methods after handling requests. From my experience on Stack Overflow, the users who would use globals will find a way and insist it's ok anyway.

@mitsuhiko
Copy link
Member

mitsuhiko commented Dec 31, 2017

@davidism thing is that I have used the fact that self is fresh from request to request myself plenty of times. The allocation of the class instance is insignificant compared to what else happens on request dispatching.

@mitsuhiko
Copy link
Member

mitsuhiko commented Dec 31, 2017

Changing this now will break a lot of code since this is also explicitly documented behavior:

The way this works is that whenever the request is dispatched a new instance of the class is created and the dispatch_request() method is called with the parameters from the URL rule. The class itself is instantiated with the parameters passed to the as_view() function.

@davidism
Copy link
Member Author

davidism commented Dec 31, 2017

@mitsuhiko would you be ok with a new SingletonView subclass?

@mitsuhiko
Copy link
Member

mitsuhiko commented Dec 31, 2017

@davidism i could imagine a lifetime attribute on the view that the generic as_view function internally references. It could be 'request' which means it lives for the single request only, 'application' which binds it to the application instance or 'global' which makes it an actual singleton that then cannot have state.

Generally the only values I think make sense are 'request' and 'application' though.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants