Automatic unregistering of BindableProperty objects

[andybayer]

First draft to fix the issue reported in #4109.

Replaces the values in the binding.bindable_properties data structure, which acts as a "registry" for available bindable properties, with weakref.finalize objects. Previously, there was a permanent reference to the owner of the BindableProperty, which was never cleared unless explicitly removed with binding.remove.

I also added some very basic tests for this behavior. You may need to refactor these slightly.

What I did not test, and what in theory should still be a problem, is when 2 models have bindable properties and one model binds to a value of the other. Then permanent references to the models are kept in binding.bindings, which are never automatically cleaned up.

[falkoschindler]

Thanks a lot for this pull request, @andybayer!

I'm just wondering: Have you tried replacing the Dict with a Set, as mentioned in #4109 (comment)? Wouldn't it avoid the need for weakrefs?

[andybayer]

Yes, I tried replacing the Dict with a Set and everything worked as expected. I started to implement this fix, but suddenly I thought that the Set also needs to be cleaned up, or else deleted items will clutter it up. So I thought an automatic cleanup action was needed.

I added the cleanup action as a new value to the Dict. Actually, I think it's not the best design because the Dict now has two jobs. It serves as a quick lookup of whether an object-attribute-pair is a BindableProperty and also as a storage for abortable cleanup actions. Perhaps we should not store a reference to the cleanup action at all, since it does no harm to run it after the object has already been manually removed with binding.remove?

[andybayer]

So what would you suggest as the next step(s) @falkoschindler?

Shall I change something?

[falkoschindler]

Hi @andybayer,

Sorry for taking sooo long to get back to you. Let me try to recap where we were:

Assigning a value to a BindableProperty for the first time stores the owner object in a bindable_properties dictionary:

nicegui/nicegui/binding.py

Line 176 in 786e24f

bindable_properties[(id(owner), self.name)] = owner

For NiceGUI's UI elements this is probably ok, because bindings get cleaned up when removing the UI object. But for custom objects the user must need to call bindings.remove().

This is demonstrated with the following demo:

import gc
import weakref
from nicegui import binding, ui

clients: list[weakref.ReferenceType] = []
models: list[weakref.ReferenceType] = []

class Model:
    answer = binding.BindableProperty()

    def __init__(self) -> None:
        self.answer = 42

@ui.page('/')
def main() -> None:
    client = ui.context.client
    clients.append(weakref.ref(client))  # to count the number of clients
    weakref.finalize(client, lambda: print('Client finalized'))  # to print when the client is finalized
    print(f'Number of clients: {len([c for c in clients if c() is not None])}')

    model = Model()
    models.append(weakref.ref(model))  # to count the number of models
    weakref.finalize(model, lambda: print('Model finalized'))  # to print when the model is finalized
    print(f'Number of models: {len([m for m in models if m() is not None])}')

    ui.button('GC', on_click=gc.collect)
    ui.button('Remove bindings', on_click=lambda: binding.remove([m() for m in models if m() is not None]))
    print(f'Number of bindings: {len(binding.bindable_properties)}')

ui.run()

1. Reloading the page increases both, the number of clients as well as the number of models.
2. Running the garbage collector ("GC") reduces the number of clients (need a few seconds to disconnect), but does not affect the number of models.
3. Only removing the bindings reduces the number of models as well.

I suggested to resolve this problem by replacing the bindable_properties dictionary with a set, because we don't really need the dictionary values. For clarity, I sketched this approach in PR #4332.

After pushing this PR, I suddenly understood your problem with this approach: Even though the models are not held in the bindable_properties dictionary anymore, the set still grows if not pruned via bindings.remove(). This can be seen in the "Number of bindings" output in the demo above. This number grows rapidly with the number of clients, and even when removing clients via "GC" the number doesn't fall back to where it was.

So finally I fully understand the need for your weakrefs like in your approach. I just refactored the code a little and shorten it here and there. Tests are still green. Let's merge!