Re: [Django] #34692: django.forms.renderers.get_default_renderer()'s template loader cache is not being reset on autoloads.
Django
not being reset on autoloads.
-------------------------------+--------------------------------------
Reporter: Andrew | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Description changed by Andrew:
Old description:
> Whenever a template file changes,
> django.template.autoreload.template_changed() is called, which in turn
> calls
> django.template.autoreload.reset_loaders(), which clears the template
> cache in every backend.engine's template loaders.
>
> However, Django's renderer for Forms uses
> forms.renderers.get_default_renderer(), which returns a mixin that has
> its own engine instance, and therefore its own loaders (and template
> caches). This engine is never reset on autoloads. This means that any
> changes to a template that is referenced as part of, EG
> "Form.template_name", is never refreshed on changes. The entire runserver
> process must be restarted for any templates cached through forms.renders
> to be reloaded. This is tedious and does not match the behavior seen by
> non-form cached templates.
>
> A crude fix can be demonstrated by changing reset_loaders() to the
> following:
>
> {{{
> def reset_loaders():
> for backend in engines.all():
> if not isinstance(backend, DjangoTemplates):
> continue
> for loader in backend.engine.template_loaders:
> loader.reset()
>
> # this code is new: reset the form renderer's template cache as well
> from django.forms.renderers import get_default_renderer
> for loader in get_default_renderer().engine.engine.template_loaders:
> loader.reset()
> }}}
New description:
Whenever a template file changes,
django.template.autoreload.template_changed() is called, which in turn
calls
django.template.autoreload.reset_loaders(), which clears the template
cache in every backend.engine's template loaders.
However, Django's renderer for Forms uses
forms.renderers.get_default_renderer(), which returns a mixin that has its
own engine instance, and therefore its own loaders (and template caches).
This engine is never reset on autoloads. This means that any changes to a
template that is referenced as part of, EG "Form.template_name", is never
refreshed on changes. The entire runserver process must be restarted for
any templates cached through forms.renders to be reloaded. This is tedious
and does not match the behavior seen by non-form cached templates.
A crude fix can be demonstrated by changing reset_loaders() to the
following:
{{{
def reset_loaders():
for backend in engines.all():
if not isinstance(backend, DjangoTemplates):
continue
for loader in backend.engine.template_loaders:
loader.reset()
# this code is new: reset the form renderer's template cache as well
from django.forms.renderers import get_default_renderer
backend = get_default_renderer().engine
if isinstance(backend, DjangoTemplates):
for loader in backend.engine.template_loaders:
loader.reset()
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34692#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
not being reset on autoloads.
------------------------+--------------------------------------
Reporter: Andrew | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: David Smith (added)
* component: Uncategorized => Forms
--
Ticket URL: <https://code.djangoproject.com/ticket/34692#comment:3>
Django
not being reset on autoloads.
------------------------+------------------------------------
Type: Bug | Status: new
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------+------------------------------------
* stage: Unreviewed => Accepted
Comment:
Thanks for the report. I was able to reproduce this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/34692#comment:4>
Django
not being reset on autoloads.
------------------------+------------------------------------
Reporter: Andrew | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------+------------------------------------
Comment (by Amir Karimi):
It seems if we don't want to add some codes like the above, we have to
change the flow of creation of the default renderer's engine. The default
engine is not included in the
{{{
engines.all()
}}}
@MariuszFelisiak
Do you think it is worth changing the flow? I suggest letting it be solved
in the simplest way.
--
Ticket URL: <https://code.djangoproject.com/ticket/34692#comment:5>
Django
not being reset on autoloads.
------------------------+------------------------------------
Reporter: Andrew | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------+------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:5 Amir Karimi]:
> It seems if we don't want to add some codes like the above, we have to
change the flow of creation of the default renderer's engine. The default
engine is not included in the
> {{{
> engines.all()
> }}}
>
> @MariuszFelisiak
> Do you think it is worth changing the flow? I suggest letting it be
solved in the simplest way.
I'm not sure what are you proposing. We didn't reject any proposition.
--
Ticket URL: <https://code.djangoproject.com/ticket/34692#comment:6>
Django
not being reset on autoloads.
Reporter: Andrew | Owner: Priyank Panchal
Type: Bug | Status: assigned
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Priyank Panchal):
* owner: nobody => Priyank Panchal
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34692#comment:7>
Django
not being reset on autoloads.
------------------------+-------------------------------------------
Reporter: Andrew | Owner: Priyank Panchal
Type: Bug | Status: assigned
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------+-------------------------------------------
Comment (by Amir Karimi):
Replying to [comment:6 Mariusz Felisiak]:
> Replying to [comment:5 Amir Karimi]:
> > It seems if we don't want to add some codes like the above, we have to
change the flow of creation of the default renderer's engine. The default
engine is not included in the
> > {{{
> > engines.all()
> > }}}
> >
> > @MariuszFelisiak
> > Do you think it is worth changing the flow? I suggest letting it be
solved in the simplest way.
>
> I'm not sure what are you proposing. We didn't reject any proposition.
Yeah. I just wanted to think about its optimal solution. I concluded that
maybe there is no cleaner way to solve this issue than simply coding as
suggested.
--
Ticket URL: <https://code.djangoproject.com/ticket/34692#comment:8>
Django
not being reset on autoloads.
------------------------+-------------------------------------------
Reporter: Andrew | Owner: Priyank Panchal
Type: Bug | Status: assigned
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
------------------------+-------------------------------------------
* needs_tests: 0 => 1
Comment:
[https://github.com/django/django/pull/17106/files PR needs regression
tests added]
--
Ticket URL: <https://code.djangoproject.com/ticket/34692#comment:9>
Django
not being reset on autoloads.
------------------------+-------------------------------------------
Reporter: Andrew | Owner: Priyank Panchal
Type: Bug | Status: assigned
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------+-------------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34692#comment:10>
Django
not being reset on autoloads.
------------------------+-------------------------------------------
Reporter: Andrew | Owner: Priyank Panchal
Type: Bug | Status: assigned
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
------------------------+-------------------------------------------
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/34692#comment:11>
Django
not being reset on autoloads.
Reporter: Andrew | Owner: Priyank Panchal
Type: Bug | Status: assigned
Component: Forms | Version: 4.2
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34692#comment:12>
Django
not being reset on autoloads.
------------------------+---------------------------------------------
Reporter: Andrew | Owner: Priyank Panchal
Component: Forms | Version: 4.2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------+---------------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"439242c5943e16dd5a3a68fadac76e5e723eb323" 439242c5]:
{{{
#!CommitTicketReference repository=""
revision="439242c5943e16dd5a3a68fadac76e5e723eb323"
Fixed #34692 -- Made autoreloader reset cached template loader for default
renderer.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34692#comment:13>