altdesktop / python-dbus-next

🚌 The next great DBus library for Python with asyncio support
https://python-dbus-next.readthedocs.io/en/latest/
MIT License
191 stars 60 forks source link

Proper cleanup of watches and message handlers #45

Closed nielsavonds closed 4 years ago

nielsavonds commented 4 years ago

Using this library for running BlueZ discovery for a whole day, I noticed some things were not being cleaned up properly. All interface objects remained in memory until the connection is closed. Instead, they should be cleaned up if nobody references them anymore. When they do get cleaned up, the corresponding message handler and DBus watch should be deleted, to avoid hitting the DBus per-connection watch limit.

acrisci commented 4 years ago

Yeah this does address a real problem.

This is how I fixed it on my other library: https://github.com/dbusjs/node-dbus-next/commit/bf32ecca6eebf2563d7ebec2309d15f98097a063

Match rules are not added at all until on() is called and then only for the associated member and then removed when off() is called. Consider if that improves the memory management portion of this. I haven't really dove too deep into that yet.

Some more notes:

acrisci commented 4 years ago

I've thought about it, and I don't like this approach. I tried to come up with some good tests for it, but couldn't do it. I don't like the idea of tying important library features to python garbage collection because that behavior is mostly undefined to our library. The approach I linked in node-dbus-next I think is preferable. Thanks for bringing this up because it is a serious issue that someone should get around to fixing soon.

acrisci commented 4 years ago

I've started work on this in 508edf8. It should work correctly, but there might be some subtle bugs in the approach.