irmen / Pyro5

Pyro 5 - Python remote objects
https://pyro5.readthedocs.io
MIT License
303 stars 36 forks source link

Allow registering object through a weakref #47

Closed eudoxos closed 2 years ago

eudoxos commented 3 years ago

I meet quite often the need to register a non-permanent object in a daemon, and have to remember to unregister it later at some point. This cannot be done in the destructor, as the daemon holding the object will prevent its reference count from dropping to zero, thus a separate bookkeeping has to be done.

There is the weakref module just for that:

A primary use for weak references is to implement caches or mappings holding large objects, where it’s desired that a large object not be kept alive solely because it appears in a cache or mapping.

I can imagine Daemon.register(...,weak=False) where weak=True would call something like weakref.finalize(obj,self,unregister(obj)) after having registered the weakref.ref(obj), plus an extra typecheck in request handling (check the weakref is not stale and use the object referred).

Do you see this as something potentially useful?

irmen commented 3 years ago

This sounds useful indeed! Do we even need a weak parameter for register()? It can find out by itself if it's passed a weakref

eudoxos commented 3 years ago

Do we even need a weak parameter for register()?

I did it with the parameter; it is more semantic (hides the implementation detail) and less verbose. But I only remembered this remark when it was done, and see now it would actually be quite elegant.

eudoxos commented 3 years ago

The latest commit in the the PR https://github.com/irmen/Pyro5/pull/50 detects weakref.ref automatically, instead of relying on weak=True. Choose which one you like better.

irmen commented 2 years ago

@eudoxos I actually like the explicit weak=True parameter better now, after some consideration. ("explicit is better than implicit" and all that). So, I think if we remove that last commit, it's a pretty solid PR

irmen commented 2 years ago

is now available, by merging PR #50