ldo / dbussy

Python binding for D-Bus using asyncio
91 stars 22 forks source link

Unreleased ref to pending_done in PendingCall #21

Closed danaru closed 5 years ago

danaru commented 5 years ago

Hello, First of all thank you for sharing and maintaining this library. I'm using dbussy to obtain login1.Manager Inhibit lock as follows:

async def take(conn, inh, what, how):
    message = dbus.Message.new_method_call(
        destination="org.freedesktop.login1",
        path="/org/freedesktop/login1",
        iface="org.freedesktop.login1.Manager",
        method="Inhibit")
    argobjs = [what, 'My Test', 'Testing', how]
    sig = [dbus.BasicType(dbus.TYPE.STRING)] * 4
    message.append_objects(sig, *argobjs)
    reply = await conn.send_await_reply(message)
    inh.fd = int(reply.expect_return_objects("h")[0])

class Inhibitor(object):
    def __init__(self):
        self.fd = -1

inh = Inhibitor()
loop = asyncio.get_event_loop()
conn = dbus.Connection.bus_get(DBUS.BUS_SYSTEM, private=False)
conn.attach_asyncio(loop)
task = loop.create_task(take(conn, inh, 'sleep', 'delay'))
loop.run_forever()

Getting file descriptor from reply will return duplicated FD and closing returned FD will release Inhibit lock. From documentations of dbus_message_iter_append_basic():

For Unix file descriptors this function will internally duplicate the descriptor you passed in. Hence you may close the descriptor immediately after this call.

However original FD not closed since PendingCall.await_reply..pending_done not released. python3 14154 root 7w FIFO 0,22 0t0 225 /run/systemd/inhibit/16.ref python3 14154 root 8w FIFO 0,22 0t0 225 /run/systemd/inhibit/16.ref

Checking with gc return the following references:

<dbussy.PendingCall object at 0x7f7acac99570>
<function PendingCall.await_reply.<locals>.pending_done at 0x7f7acacad158>
<weakref at 0x7f7acaca03b8; dead>
<cell at 0x7f7acacfdd68: function object at 0x7f7acacad158>
<cell at 0x7f7acacfdd98: PendingCall object at 0x7f7acac99570>
<cell at 0x7f7acacfddc8: weakref object at 0x7f7acaca03b8>
<function PendingCall.set_notify.<locals>._wrap_notify at 0x7f7acacad0d0>
<_ctypes.CThunkObject object at 0x7f7acaca8430>
(<cell at 0x7f7acacfdd08: _asyncio.Future object at 0x7f7acac95828>,)
(<cell at 0x7f7acacfdd68: function object at 0x7f7acacad158>, <cell at 0x7f7acacfdd98: PendingCall object at 0x7f7acac99570>, <cell at 0x7f7acacfddc8: weakref object at 0x7f7acaca03b8>)
<cell at 0x7f7acacfdd08: _asyncio.Future object at 0x7f7acac95828>

Proposed fix:

diff --git a/dbussy.py b/dbussy.py
index c947b59..e4934db 100644
--- a/dbussy.py
+++ b/dbussy.py
@@ -4918,6 +4918,7 @@ class PendingCall :
         self.set_notify(pending_done, weak_ref(self))
           # avoid reference circularity self → pending_done → self
         reply = await done
+        self.set_notify(None, None)
         return \
             reply
     #end await_reply
ldo commented 5 years ago

The problem is a bit more fundamental than that.

After dc0127c5f1bcc0983d60915ac7d96a1248d8f347, I only see the process keep one open FD For /run/systemd/inhibit/«n».ref, instead of 2 as before (and as you report). Can you confirm it fixes the problem for you?

danaru commented 5 years ago

Yes, it fixes the problem Thanks a lot

danaru commented 5 years ago

One more question , when will be next release?

On Sun, Sep 15, 2019 at 12:59 AM ldo notifications@github.com wrote:

Closed #21 https://github.com/ldo/dbussy/issues/21.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ldo/dbussy/issues/21?email_source=notifications&email_token=ABDWN32IFJLAQLTWMHLEH2DQJVNEFA5CNFSM4IVQ2KPKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOTUCRB7I#event-2634354941, or mute the thread https://github.com/notifications/unsubscribe-auth/ABDWN3Z2RZMXEPRYMRBGFY3QJVNEFANCNFSM4IVQ2KPA .

--