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

Lifetime of objects owning a BindableProperty #4109

Open
andybayer opened this issue Dec 16, 2024 · 2 comments
Open

Lifetime of objects owning a BindableProperty #4109

andybayer opened this issue Dec 16, 2024 · 2 comments

Comments

@andybayer
Copy link

andybayer commented Dec 16, 2024

When I stumbled across BindableProperty, I thought it would also be very useful for modeling data that can be modified through various "widgets" and keyboard shortcuts. Basically, a model-like object that encapsulates the single source of truth for some data. A small example on the official website seems to support that this is an intended use case.

So I built a small application using this idea. It seems to work quite well. Since I also needed to clean up some resources for which a with-statement would not be very practical (and would also require some minor refactoring), I simply used weakref.finalize to add a "finalizer" to the model that does the cleanup. This is similar to RAII in C++, but without the deterministic behavior of when exactly the finalizer will run.

Then I noticed that the resources were never cleaned up, even when the garbage collection was forced to run. So I looked for references to my model. Since I used BindableProperty and bindings in general a lot, there were several references to my model in the global objects in the bindings module. I then looked up how these are all cleaned up. It seems that they are only cleaned up when the client is deleted (except for some special behavior for ui.scene)? The client by default only calls binding.remove for elements added to it. But at least it seems to remove related references to the model's BindableProperty as well.

What is left is a reference to the model in binding.bindable_properties, since it is the owner of the property. This will keep the model alive if it is not explicitly deleted, and will also leak memory. You could add a deleter manually in app.on_disconnect or as a client finalizer, but I am not sure if this is really a good solution. Maybe it would be better to remove the reference to the owner in binding.bindable_properties as I cannot see that it is currently needed anywhere, and add a finalizer using weakref.finalize to clean up the (id, name) tuples? But maybe the reference to the owner has some use I have not seen.

Below is a small example that shows the behavior described above. The naive model is not deleted when you disconnect from the client. The fixed model is deleted when you disconnect and start the next session.

import gc
import weakref
from collections.abc import Callable
from datetime import datetime

from nicegui import app, ui
from nicegui.binding import bindable_properties, BindableProperty


class Model:
    name = BindableProperty()
    instances: list[weakref.ReferenceType] = list()

    def __init__(self, name: str) -> None:
        self.created = datetime.now()
        self.name = name

        self.instances.append(weakref.ref(self))

        weakref.finalize(self, lambda: log(f'Model "{name}" FINALIZED'))
        log(f'Model "{name}" initialized')

      @classmethod
      def alive(cls) -> list[Self]:  # only for simplified debugging
          return [target for reference in cls.instances if (target := reference()) is not None]


def log(message: str) -> None:
    now = datetime.now().time().isoformat(timespec='seconds')
    print(f'{now}  {message}')


@ui.page('/')
async def main() -> None:
    gc.collect()  # force GC run to make the example more reproducible

    client = ui.context.client
    name = str(client.id)
    weakref.finalize(client, lambda: log(f'Client {name} FINALIZED'))

    await client.connected()
    log(f'Client {name} connected')

    naive = Model('naive')
    fixed = Model('fixed')

    def cleanup(model: Model) -> Callable[[], None]:
        def finalizer() -> None:
            found = [key for key, obj in bindable_properties.items() if obj is model]
            for key in found:
                del bindable_properties[key]

            gc.collect()
            log(f'Manually removed model "{model.name}" from bindable properties')

        return finalizer

    weakref.finalize(client, cleanup(fixed))

    ui.label('Bound Models').classes('text-h6')
    ui.label(naive.name).bind_text(naive, 'name')
    ui.label(fixed.name).bind_text(fixed, 'name')


ui.run(show=False, reload=False, show_welcome_message=False)
@falkoschindler
Copy link
Contributor

Interesting observation, @andybayer!

If I understand correctly, the main issue is the object reference stored in the bindable_properties dictionary, which is filled with values like this:

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

The dictionary is used in three places:

  • Two checks like the following only use the dictionary keys:
    if (id(self_obj), self_name) not in bindable_properties:
  • This part uses id(obj), but could also use obj_id instead:
    for (obj_id, name), obj in list(bindable_properties.items()):
        if id(obj) in object_ids:
            del bindable_properties[(obj_id, name)]

So it should be possible to replace the dictionary with a set of keys, removing the object references. Do you think this fixes the memory leak?

@andybayer
Copy link
Author

Thank you very much for your quick reply.

I tested replacing the dict for binding.bindable_properties with a set using the following reduced example.

import weakref
from datetime import datetime

from nicegui import ui
from nicegui.binding import BindableProperty


class Model:
    name = BindableProperty()

    def __init__(self, name: str) -> None:
        self.name = name

        weakref.finalize(self, lambda: log(f'Model "{name}" FINALIZED'))
        log(f'Model "{name}" initialized')


@ui.page('/')
async def main() -> None:
    automatic = Model('automatic')

    ui.label('Models').classes('text-h6')
    ui.label(automatic.name).bind_text(automatic, 'name')


def log(message: str) -> None:
    now = datetime.now().time().isoformat(timespec='seconds')
    print(f'{now}  {message}')


ui.run(show=False, reload=False, show_welcome_message=False)

I got the following output after connecting to the UI via the browser and closing the browser tab shortly after. So it seems to work. There is even no need to force a manual garbage collection run. At least on my system (OpenSUSE Tumbleweed, Python 3.11.10, NiceGUI 2.8.0).

13:04:20  Model "automatic" initialized
13:04:28  Model "automatic" FINALIZED

However, I hope I didn't misinterpret anything, as my knowledge of web technologies and GUIs in particular is quite limited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants