irmen / Pyro5

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

Are autoproxy lifetimes broken? #92

Open TheSven73 opened 1 month ago

TheSven73 commented 1 month ago

I'm trying to take an existing Python library, and allow it to be used remotely over the network. The library creates and returns Python objects which contain C library handles, so AFAIK they cannot be serialized, and need to be auto-proxied.

So far so good. But, I am running into trouble with autoproxy object lifetimes. When an autoproxy proxy handle goes out of scope, the corresponding server object is never released.

Some pytests which I believe should succeed, yet two fail. Edit: python==3.12.3, Pyro5==5.15, pytest==8.2.2

from concurrent.futures import ThreadPoolExecutor

import pytest
from Pyro5.api import Daemon, Proxy, behavior, expose

@expose
class Child:
    def __init__(self, name: str, alive: set[str]):
        self.__name = name
        self.__alive = alive
        alive.add(name)

    def __del__(self):
        self.__alive.remove(self.__name)

    def __repr__(self):
        return self.__name

    def ping(self):
        return f"I am {self.__name}"

_live_objects: set[str]

@expose
@behavior(instance_mode="session", instance_creator=lambda clazz: clazz(_live_objects))
class Parent:
    _pyroDaemon: Daemon

    def __init__(self, alive: set[str]):
        self.__name = f"outer {id(self)}"
        self.__alive = alive
        alive.add(self.__name)

    def __del__(self):
        self.__alive.remove(self.__name)

    def __repr__(self):
        return self.__name

    def ping(self):
        return f"I am {self.__name}"

    def createInstance(self, weak=False):
        i = Child(name="single instance", alive=self.__alive)
        self._pyroDaemon.register(i, weak)
        return i

    def createGenerator(self, weak=False):
        ii = [
            Child(name=f"iterator instance {i}", alive=self.__alive) for i in range(4)
        ]
        for i in ii:
            self._pyroDaemon.register(i, weak)
            yield i

@pytest.fixture
def proxy():
    global _live_objects
    _live_objects = set()
    with ThreadPoolExecutor(max_workers=1) as e, Daemon() as daemon:
        uri = daemon.register(Parent, force=True)
        must_shutdown = False
        _ = e.submit(daemon.requestLoop, lambda: not must_shutdown)
        with Proxy(uri) as proxy1, Proxy(uri) as proxy2:
            yield proxy1, proxy2
        must_shutdown = True
    assert not _live_objects, "some objects were not cleaned up"

# succeeds
def test_proxy_lifetime(proxy):
    p1, p2 = proxy
    assert "outer" in p1.ping()
    assert "outer" in p2.ping()

# fails teardown: AssertionError: some objects were not cleaned up
def test_instance_lifetime(proxy):
    p, _ = proxy
    assert "outer" in p.ping()
    inner = p.createInstance()
    assert "single instance" in inner.ping()

# fails teardown: AssertionError: some objects were not cleaned up
def test_generator_lifetime(proxy):
    p, _ = proxy
    for inner in p.createGenerator():
        assert "iterator instance" in inner.ping()
irmen commented 1 month ago

I think the daemon's shutdown() is never called and it remains running in the requestLoop(). Setting the must_shutdown to true after the call, is not magically going to change the value of the boolean argument to requestLoop(). Talking about the proxy() function above.

TheSven73 commented 1 month ago

The must_shutdown variable is captured in the lambda, which is a closure. Setting must_shutdown to True makes the lambda return False so that the requestLoop eventually exits. The ThreadPoolExecutor context manager blocks until all futures scheduled on the executor have finished. proxy() can only exit if the RequestLoop has stopped.

I clarified the code by waiting for the requestloop future to finish explicitly, however this produces the exact same result:

@pytest.fixture
def proxy():
    global _live_objects
    _live_objects = set()
    with ThreadPoolExecutor(max_workers=1) as e, Daemon() as daemon:
        uri = daemon.register(Parent, force=True)
        must_shutdown = False
        fut = e.submit(daemon.requestLoop, lambda: not must_shutdown)
        with Proxy(uri) as proxy1, Proxy(uri) as proxy2:
            yield proxy1, proxy2
        must_shutdown = True
        fut.result() # <==== added
    assert not _live_objects, "some objects were not cleaned up"

The fixture approach using an executor is perhaps too complex. I can create a test example where the daemon requestloop runs inside a thread, would you prefer this approach?

TheSven73 commented 1 month ago

Followed your suggestion to use daemon.shutdown() this works faster, great ! Same result though, the two tests still fail:

@pytest.fixture
def proxy():
    global _live_objects
    _live_objects = set()
    with ThreadPoolExecutor(max_workers=1) as e, Daemon() as daemon:
        uri = daemon.register(Parent, force=True)
        fut = e.submit(daemon.requestLoop)
        with Proxy(uri) as proxy1, Proxy(uri) as proxy2:
            yield proxy1, proxy2
        daemon.shutdown()
    assert not _live_objects, "some objects were not cleaned up"
irmen commented 1 month ago

Hmm... okay I was mistaken about the boolean

It's been a few years since I wrote the unit-tests for pyro5, but there are a bunch in the test_server module that use a running daemon. Maybe I did something different there? I am not sure if I ever cared about leaking refs in those tests though.

TheSven73 commented 1 month ago

I added a few simple (currently failing) tests that verify autoproxy lifetimes here: https://github.com/irmen/Pyro5/pull/93

Maybe these might be useful as a starting point to investigate the issue further?