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
Comments
I don't see how this can safely be changed without risking to break people's code if they store stuff on Besides that, instantiating a class is quite cheap... |
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. |
Storing things on |
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 |
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 |
Maybe add a |
If we add this I would rather go for this thing to always be configurable or even better add a |
I'll have to think on how we could do |
Isn't storing stuff on 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). |
|
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. |
What about object pooling? |
You'll need to explain what you mean. |
This was done intentionally because people can stuff state on self between functions. It matches the behavior in Django. |
We could probably do something with |
@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. |
Changing this now will break a lot of code since this is also explicitly documented behavior:
|
@mitsuhiko would you be ok with a new SingletonView subclass? |
@davidism i could imagine a Generally the only values I think make sense are |
davidism commentedNov 14, 2017
•
edited
View.as_view
creates a function that instantiates the class every request. This seems inefficient for no real gain.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.
The text was updated successfully, but these errors were encountered: