`post_save` signal does not get disconnected when class gets unreferenced

[sjoerdjob]

Describe the bug
While running Django migrations, it can happen that subclasses of DirtyFieldsMixin get created (and later unreferenced, and thus destroyed). When a class gets unreferenced, the location that class holds in memory gets cleared up again, and a new Python object could be instantiated in that location. That new Python object might be another model class.

To Reproduce

See the attached project for a minimal example.
Running python manage.py migrate (after having installed the latest version of Django and django-dirtyfields) triggers the issue most of the time.

minimal.tar.gz

Expected behavior
Migrations always run completely pass.

Actual Behavior

Traceback (most recent call last):
  File "/home/sjoerdjobpostmus/dev/minimal/./manage.py", line 22, in <module>
    main()
    ~~~~^^
  File "/home/sjoerdjobpostmus/dev/minimal/./manage.py", line 18, in main
    execute_from_command_line(sys.argv)
    ~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line
    utility.execute()
    ~~~~~~~~~~~~~~~^^
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/core/management/__init__.py", line 436, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/core/management/base.py", line 413, in run_from_argv
    self.execute(*args, **cmd_options)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/core/management/base.py", line 459, in execute
    output = self.handle(*args, **options)
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/core/management/base.py", line 107, in wrapper
    res = handle_func(*args, **kwargs)
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/core/management/commands/migrate.py", line 357, in handle
    post_migrate_state = executor.migrate(
        targets,
    ...<3 lines>...
        fake_initial=fake_initial,
    )
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/db/migrations/executor.py", line 135, in migrate
    state = self._migrate_all_forwards(
        state, plan, full_plan, fake=fake, fake_initial=fake_initial
    )
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/db/migrations/executor.py", line 167, in _migrate_all_forwards
    state = self.apply_migration(
        state, migration, fake=fake, fake_initial=fake_initial
    )
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/db/migrations/executor.py", line 255, in apply_migration
    state = migration.apply(state, schema_editor)
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/db/migrations/migration.py", line 132, in apply
    operation.database_forwards(
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~^
        self.app_label, schema_editor, old_state, project_state
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/db/migrations/operations/special.py", line 196, in database_forwards
    self.code(from_state.apps, schema_editor)
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/sjoerdjobpostmus/dev/minimal/stress/migrations/0002_auto_20241119_1734.py", line 28, in update_withoutdirty_instance
    inst.save()
    ~~~~~~~~~^^
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/db/models/base.py", line 891, in save
    self.save_base(
    ~~~~~~~~~~~~~~^
        using=using,
        ^^^^^^^^^^^^
    ...<2 lines>...
        update_fields=update_fields,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/db/models/base.py", line 1012, in save_base
    post_save.send(
    ~~~~~~~~~~~~~~^
        sender=origin,
        ^^^^^^^^^^^^^^
    ...<4 lines>...
        using=using,
        ^^^^^^^^^^^^
    )
    ^
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/django/dispatch/dispatcher.py", line 189, in send
    response = receiver(signal=self, sender=sender, **named)
  File "/home/sjoerdjobpostmus/.virtualenvs/minimal/lib/python3.13/site-packages/dirtyfields/dirtyfields.py", line 172, in reset_state
    new_state = instance._as_dict(check_relationship=True)
                ^^^^^^^^^^^^^^^^^
AttributeError: 'WithoutDirty' object has no attribute '_as_dict'

Environment (please complete the following information):

• OS: Debian (but also noticed under Ubuntu)
• Python version: 3.12 and 3.13.
• Django version: 4.2+
• django-dirtyfields version: 1.9.5 and newer.

Additional context

A related Django issue is https://code.djangoproject.com/ticket/35801#comment:6 . I'm not sure if either of the codebases can fix this bug separately though, or whether it requires changes in both projects.

Suggested improvements:

• add a class-attribute registered_post_save_signal (or something) to make sure post_save.connect is only called once per model class.
• In the code that is calling post_save.connect, also add 'something' that makes sure that post_save.disconnect(reset_state, sender=self.__class__, dispatch_uid=...) gets called whenever the model class (not instance) gets unreferenced.

[LincolnPuzey]

Hi @sjoerdjob Thanks for the bug report. This looks like an interesting issue 😃

I think that django ticket you linked to is the right one relevant to this.
It looks like the maintainers there are considering two options

1. Changing Django to hold a (weak)ref to the sender class
2. Documenting that the signal should be disconnected before the class is de-referenced.

In the case of (1) I imagine that would fix this issue.
For (2) we may have to adjust our code to disconnect the signal at the right time.

---

Your first suggested improvement is a good idea, but I don't think it will fix this issue.

In the code that is calling post_save.connect, also add 'something'

Do you have any suggestions on what that 'something' should be?
I was going to say we would need to implement __del__ on the metaclass,
but then I found weakref.finalize which might be the right thing to use.

[sjoerdjob]

Using weakref.finalize indeed does seem like the 'something' we might be looking for.

Do note the following comment though:

Note

It is important to ensure that func, args and kwargs do not own any references to obj, either directly or indirectly, since otherwise obj will never be garbage collected. In particular, func should not be a bound method of obj.

In this case, obj would be the class for which the signal should be disconnected. However, looking at Django's 'disconnect', it seems clear that Django requires the caller to pass the same value for sender as which was used to register the connect call.

So the most pressing question here is: During the callback phase of weakref.finalize, can we still get the class? First of all, we shouldn't keep an actual reference alive, as that would make the class never garbage collected. But we could keep a weakref alive, and hope that when the callback of weakref.finalize gets called, that weakref could be used to resurrect the model class temporarily, just long enough to call post_save.disconnect.

The weakref.finalize is a bit of synthetic sugar around weakref.ref(obj, callback), which has the following comment:

If callback is provided and not None, and the returned weakref object is still alive, the callback will be called when the object is about to be finalized; the weak reference object will be passed as the only parameter to the callback; the referent will no longer be available.

Which makes me think that at the time the callback of weakref.finalize is called, the model class is no longer obtainable, and as such it's too late to call disconnect(). Initial experimentation shows that the weakref pointing to the class would be 'dead' by the time it's called.

As such, without any changes in Django itself, I think the only way forward would be to create a metaclass that registers a __del__, which at least should be called when the class is still 'alive'. Just in time.

However, I myself personally don't have fun experiences with extending metaclasses for Django Model classes. There are quite some constraints there. In particular, Python will raise a TypeError if multiple bases are used which define metaclasses, and there is not a 'most derived' metaclass:

https://docs.python.org/3/reference/datamodel.html#determining-the-appropriate-metaclass

Which makes using metaclasses also a less-than-ideal solution.

[sjoerdjob]

After considering the above a bit more, I think a different route might also be taken.

Instead of registering reset_state per class, how about globally connecting to reset_state and m2m_changed with sender=None, and then augment reset_state as follows:

def reset_state(sender, instance, **kwargs):
    if not isinstance(instance, DirtyFieldsMixin):
        return

There are upsides and downsides to this approach.

• Upside: the number of listeners to post_save and m2m_changed is reduced from arbitrarily many to just 1. This could theoretically result in a minor performance gain on .save(), as there are less receivers to be checked.
• Upside: a simple ModelClass() will not trigger the 'connect' flow right now, so should become a lot faster.
• Downside: for classes that don't extend DirtyFieldsMixin, a call to reset_state(...) will now be done, making .save() a little bit slower for model instances that are not compatible with DirtyFieldsMixin.

The performance characteristics of this change would heavily depend on which model types get the most .save() calls. If a flow is heavy in .save() calls on DirtyFieldsMixin instances, a performance increase would likely be the result. If a flow is heavy in .save() calls on non-DirtyFieldsMixin instances, a performance loss would likely be the result.

Since the connect has always been in __init__ since one of the first commits (df8aa43#diff-3d39ad67eacb5d622dfbde83549c6d3d81eb3984047284ddf34219ebeeb1e06fR7), I find it hard to evaluate whether a benchmark has been done before deciding this route.

[sjoerdjob]

I just saw that Django fixed the underlying issue. Probably this means that this issue is fixed too in newer versions of Django.