search

hypfvieh / dbus-java

Improved version of java DBus library provided by freedesktop.org (https://dbus.freedesktop.org/doc/dbus-java/)
https://hypfvieh.github.io/dbus-java/
MIT License
176 stars 70 forks source link

Memory leak in AbstractConnectionBase#importedObjects #261

Open AlexIvchenko opened 2 weeks ago

AlexIvchenko commented 2 weeks ago

Hello!

We used 2019 year forked version of this library and now we are assessing the possibility to upgrade. We encountered that there is a memory leak in AbstractConnectionBase.importedObjects because objects is only added/queries to this map, but never removed. I suppose current version is affected by this issue. Our fix included change ConcurrentHashMap to Collections.synchronizedMap(new WeakHashMap()), but maybe it not the best option.

P.s. thank you for great library)

hypfvieh commented 2 weeks ago

I checked the usage of importedObjects in the current code base and in the original version from 2017. In both cases, importedObjects is used for writing and for look-ups - it was and is never cleaned.

The purpose of this Map is to maintain references of remote objects. Remote objects are objects which are fetched through DBus and represent the state of a remote application.

This ensures that if querying for the same object (using the object path) on DBus you will receive the same object instead of creating a new one. This also means, that if you have one connection and use that connection in different parts of your application, you will always get the same object.

The Map is part of the current connection. As soon as the connection is closed (by your application or by the remote end) the Map and anything related to the actual connection is eligible for garbage collection.

I'm not sure that using a WeakHashMap in place of a regular Map is a suitable solution. A WeakHashMap would allow the GC to remove remote objects which are currently not reachable. This would cause remote objects to get lost also if the application will access it from somewhere else later.

It may introduce other problems I'm not aware of of yet.

AlexIvchenko commented 2 weeks ago

I use dbus-java to manipulate systemd units using https://github.com/thjomnx/java-systemd (https://www.freedesktop.org/wiki/Software/systemd/dbus/). It uses single connection to work with systemd via dbus. In that scenario the connection keeps obsolete RemoteObjects for systemd units which are not needed and even not present in systemd anymore.

This would cause remote objects to get lost also if the application will access it from somewhere else later.

Is connection supposed to be reopened periodically to cleanup unnecessary objects?

hypfvieh commented 2 weeks ago

Is connection supposed to be reopened periodically to cleanup unnecessary objects?

There is nothing to clean up once a DBus connection is closed. At that time everything belonging to that connection is obsolete. This includes the importedObjects map which is a member of AbstractConnectionBase which in turn is the base class for all connections of this library (no matter which transport).

I would like to understand what your application is doing so this behavior becomes an issue. If your application connects to Systemd via DBus, does something and then disconnects everything should be fine. If the application keeps the connection for weeks, this may be problematic.

In that scenario the connection keeps obsolete RemoteObjects for systemd units which are not needed and even not present in systemd anymore.

It is impossible to know if an remote object is still valid from the library point of view. You have to do any sort of 'action' with that object and then only you know that it is broken - the library internals are not aware of that.

AlexIvchenko commented 2 weeks ago

I would like to understand what your application is doing so this behavior becomes an issue.

Communicating with systemd (starting systemd-units) via single connection for entire lifetime of application. Because of that it may keep connection for long time (basically until we need to update the application).

hypfvieh commented 2 weeks ago

I updated the code so you can now use WeakHashMap for imported objects. You can enable that using withImportWeakReferences on the DBusConnectionBuilder.

However, the default behavior of using regular (hard) references is unchanged. I may change that in the future, but I'm still unaware of the possible side effects. Nevertheless if you set the flag using the builder your will not be affected by future changes of the default.