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
185 stars 73 forks source link

Wrong signal constructor retrieved #110

Closed purejava closed 4 years ago

purejava commented 4 years ago

Hi,

thanks a lot for providing and maintaining dbus-java!

I implement a signal handler for the KWallet interface, that has two signals with the same name, but different contructors:

<signal name="walletClosed">
  <arg name="wallet" type="s" direction="out"/>
</signal>
<signal name="walletClosed">
  <arg name="handle" type="i" direction="out"/>
</signal>

Reference: https://github.com/KDE/kwallet/blob/master/src/api/KWallet/org.kde.KWallet.xml

Demo code available at https://github.com/purejava/dbusjavatest.git - branch wip-kwallet-api -> simply run App.java. Please note, that you need a KDE desktop environment or at least kwallet installed.

As Java does not allow to have two classes / signals of the same name, I introduced a second interface AbstractInterface, that contains the second signal with the int signature.

On closing a wallet, both signals get emitted:

signal time=1594906367.214555 sender=:1.79 -> destination=(null destination) serial=16981 path=/modules/kwalletd5; interface=org.kde.KWallet; member=walletClosed
   int32 1765833520
signal time=1594906367.214570 sender=:1.79 -> destination=(null destination) serial=16982 path=/modules/kwalletd5; interface=org.kde.KWallet; member=walletClosed
   string "kdewallet"

Nevertheless, the instanceof-Code in the SignalHandler does identify both signals of the same type / instanceof KWallet, which is incorrect. One should be instanceof AbstractInterface.

Looking through your code I could not find out what's wrong. Thanks.

hypfvieh commented 4 years ago

Without digging too much in your sample code, did you try to use the @DBusMemberName(String name) annotation?

I would suggest you remove your 'AbstractInterface' Class and move the signal back to KWallet.class.

Then rename the signal so it is unique (e.g. closeWalletInt), add the @DBusMemberName annotation to change the name to whatever KWallet is expecting on the Bus.

purejava commented 4 years ago

Thanks for the suggestion. I changed the code accordingly, see https://github.com/purejava/dbusjavatest/commit/55e978fd7769183a28e43137cfd8ade8eece7732, but unfortunately it still fails.

The dbus signature of the parameters ist still string for both signals.

hypfvieh commented 4 years ago

I think it is not possible to listen for both signals at the same time.

Taking a look at DBusSignal.java, you will see that each signal is registered once in a map, where the key ist the name of the signal. If you add a second signal with the same name but different signature, it will overwrite the entry in the map. Therefore only one of the two signals will be handled.

hypfvieh commented 4 years ago

Just another quick note: When looking at the source code of kwalletd, you can see that it will always emit both signals: int (handle) and string (wallet)

So it may be suit your needs to listen to one of them.

purejava commented 4 years ago

Thanks for elaborating on this.

I agree. First, adding a SignalHandler for either one of the signals (so for the String one or for the int one) does work. I tested this yesterday. The respective signal can be handled then. You’ll have to decide on which signal you want to work with. This is sufficient for every kwallet application I can think of.

Second, after your comment, I looked through the D-Bus specification and could not find anything that describes a requirement for the case described, hence to be able to listen for both signals at the same time.

purejava commented 4 years ago

I agree. First, adding a SignalHandler for either one of the signals (so for the String one or for the int one) does work.

I have to correct myself, it did work with the test application, but not with my real world code. You can run the test on a locked wallet, you need kwallet installed though.

As you can see, I registered the signal in question once, as you advised above, but the one with the String signature gets also handled by the int-Signal with leads to an IllegalArgumentException.

[main] INFO org.freedesktop.dbus.handlers.SignalHandler - await signal org.kde.KWallet$walletAsyncOpened(/modules/kwalletd5) within 300 seconds.
[DBus Worker Thread-2] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.walletOpened: kdewallet
[DBus Worker Thread-3] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.walletAsyncOpened: 70 / 1590081089
[main] INFO org.purejava.KDEWalletTest2 - Wallet 'kdewallet' successfully opened.
[main] INFO org.purejava.KDEWalletTest2 - Received handle: 1590081089.
[main] INFO org.purejava.KDEWalletTest2 - Folder 'Test-Folder' successfully created.
[DBus Worker Thread-4] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.folderListUpdated: kdewallet
[main] INFO org.purejava.KDEWalletTest2 - Folder 'Test-Folder' successfully deleted.
[DBus Worker Thread-1] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.folderListUpdated: kdewallet
[main] INFO org.freedesktop.dbus.handlers.SignalHandler - await signal org.kde.KWallet$walletClosedInt(/modules/kwalletd5) within 300 seconds.
[DBus Worker Thread-2] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.walletClosedInt: 1590081089
[DBus Worker Thread-3] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.allWalletsClosed: /modules/kwalletd5
[DBus Worker Thread-2] WARN org.freedesktop.dbus.connections.impl.DBusConnection - Exception while running signal handler 'org.freedesktop.dbus.handlers.SignalHandler@c3ba747' for signal 'DBusSignal [clazz=class org.kde.KWallet$walletClosedInt]':
org.freedesktop.dbus.exceptions.DBusException: java.lang.IllegalArgumentException: argument type mismatch
  at org.freedesktop.dbus.messages.DBusSignal.createReal(DBusSignal.java:228)
  at org.freedesktop.dbus.connections.AbstractConnection$3.run(AbstractConnection.java:904)
  at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
  at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
  at java.base/java.lang.Thread.run(Thread.java:832)
Caused by: java.lang.IllegalArgumentException: argument type mismatch
  at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
  at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
  at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
  at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
  at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481)
  at org.freedesktop.dbus.messages.DBusSignal.createReal(DBusSignal.java:221)
  ... 4 more
[main] INFO org.purejava.KDEWalletTest2 - Wallet 'kdewallet' with handle '1590081089' successfully closed.

Process finished with exit code 0

Bildschirmfoto 2020-07-25 um 17 58 12

Bildschirmfoto 2020-07-25 um 17 58 37

The full TRACE is available on HiDrive.

Edit: Updated link to source code.

hypfvieh commented 4 years ago

The problem is, that dbus-java identifies signals by it's name. Both signals use the same name, but different signatures.

When dbus-java tries to call the constructor of the signal using the received parameters, dbus-java does not check if the parameters received are compatible to the constructor. It just calls the constructor passing the received parameters. This will cause the failures you see, because you expect an int, but receive both, string and int.

I guess this will not be easy to fix. I added a new branch ('ambigous-signals') which tries to handle that problem. I don't know if its working already ,I can't test it on my machine because kwallet does not work (don't know why). I may setup a virtual machine next week to dig further into it.

purejava commented 4 years ago

Thanks for looking into that so quickly!

I tested my code with your new branch. The change swallows a couple of signals. Here is a comparison what happened in my code vs dbus-monitor:

my code

[main] INFO org.freedesktop.dbus.handlers.SignalHandler - await signal org.kde.KWallet$walletAsyncOpened(/modules/kwalletd5) within 20 seconds.
[DBus Worker Thread-2] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.walletOpened: kdewallet

dbus-monitor during the same time:

signal time=1595782007.618134 sender=:1.79 -> destination=(null destination) serial=646 path=/modules/kwalletd5; interface=org.kde.KWallet; member=walletOpened
   string "kdewallet"
signal time=1595782007.636057 sender=:1.79 -> destination=(null destination) serial=649 path=/modules/kwalletd5; interface=org.kde.KWallet; member=walletAsyncOpened
   int32 27
   int32 1558283562
signal time=1595782013.273413 sender=:1.79 -> destination=(null destination) serial=653 path=/modules/kwalletd5; interface=org.kde.KWallet; member=walletListDirty
signal time=1595782013.273432 sender=:1.79 -> destination=(null destination) serial=654 path=/modules/kwalletd5; interface=org.kde.KWallet; member=walletListDirty
signal time=1595782013.273435 sender=:1.79 -> destination=(null destination) serial=655 path=/modules/kwalletd5; interface=org.kde.KWallet; member=walletListDirty
hypfvieh commented 4 years ago

I updated the branch, could you please try again?

purejava commented 4 years ago

Absolutely!

Getting closer, but we are not yet there:

[main] INFO org.freedesktop.dbus.handlers.SignalHandler - await signal org.kde.KWallet$walletAsyncOpened(/modules/kwalletd5) within 20 seconds.
[DBus Worker Thread-2] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.walletOpened: kdewallet
[DBus Worker Thread-3] WARN org.freedesktop.dbus.messages.DBusSignal - Could not find suitable constructor for class org.kde.KWallet$walletAsyncOpened with argument-types: [class java.lang.Integer, class java.lang.Integer]

The full TRACE is available on HiDrive.

hypfvieh commented 4 years ago

Another update on the branch.

The issue was handling of primitives. While you will always get the wrapper version of primitives when gathering the class types of the received parameters, you will get the primitive version when asking the constructor for it's parameters if primitives were used in declaration.

I tested it in a freshly installed kubuntu 20.04 virtual machine using the sample code you provided. Now I receive both, walletClosedInt and asyncOpened.

Maybe you can have another look.

purejava commented 4 years ago

Maybe you can have another look.

I love to, as your fix is required for my code to work correctly. And I opened this issue, so testing from my side is required.

Now I receive both, walletClosedInt and asyncOpened.

Great! I tested your change as well and receive all signals that get emitted now too and: registering both variants of the walletClosed signal (int and String) one after another does work correctly.

walletClosedInt

[main] INFO org.freedesktop.dbus.handlers.SignalHandler - await signal org.kde.KWallet$walletAsyncOpened(/modules/kwalletd5) within 20 seconds.
[DBus Worker Thread-2] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.walletOpened: kdewallet
[DBus Worker Thread-3] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.walletAsyncOpened: 1 / 402600963
[main] INFO org.purejava.KDEWalletTest2 - Wallet 'kdewallet' successfully opened.
[main] INFO org.purejava.KDEWalletTest2 - Received handle: 402600963.
[main] INFO org.purejava.KDEWalletTest2 - Folder 'Test-Folder' successfully created.
[DBus Worker Thread-4] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.folderListUpdated: kdewallet
[DBus Worker Thread-1] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.folderListUpdated: kdewallet
[main] INFO org.purejava.KDEWalletTest2 - Folder 'Test-Folder' successfully deleted.
[main] INFO org.freedesktop.dbus.handlers.SignalHandler - await signal org.kde.KWallet$walletClosedInt(/modules/kwalletd5) within 20 seconds.
[DBus Worker Thread-2] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.walletClosedInt: 402600963
[DBus Worker Thread-3] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.allWalletsClosed: /modules/kwalletd5
[DBus Worker Thread-2] WARN org.freedesktop.dbus.messages.DBusSignal - Could not find suitable constructor for class org.kde.KWallet$walletClosedInt with argument-types: [class java.lang.String]
[main] INFO org.purejava.KDEWalletTest2 - Wallet 'kdewallet' with handle '402600963' successfully closed.

Process finished with exit code 0

The full TRACE on HiDrive.

walletClosed

[main] INFO org.freedesktop.dbus.handlers.SignalHandler - await signal org.kde.KWallet$walletAsyncOpened(/modules/kwalletd5) within 20 seconds.
[DBus Worker Thread-2] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.walletOpened: kdewallet
[DBus Worker Thread-3] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.walletAsyncOpened: 7 / 1474875233
[main] INFO org.purejava.KDEWalletTest2 - Wallet 'kdewallet' successfully opened.
[main] INFO org.purejava.KDEWalletTest2 - Received handle: 1474875233.
[main] INFO org.purejava.KDEWalletTest2 - Folder 'Test-Folder' successfully created.
[DBus Worker Thread-4] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.folderListUpdated: kdewallet
[DBus Worker Thread-1] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.folderListUpdated: kdewallet
[main] INFO org.purejava.KDEWalletTest2 - Folder 'Test-Folder' successfully deleted.
[DBus Worker Thread-3] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.walletClosed: kdewallet
[DBus Worker Thread-3] INFO org.freedesktop.dbus.handlers.SignalHandler - KWallet.allWalletsClosed: /modules/kwalletd5
[main] INFO org.freedesktop.dbus.handlers.SignalHandler - await signal org.kde.KWallet$walletClosed(/modules/kwalletd5) within 20 seconds.
[DBus Worker Thread-2] WARN org.freedesktop.dbus.messages.DBusSignal - Could not find suitable constructor for class org.kde.KWallet$walletClosed with argument-types: [class java.lang.Integer]
[main] ERROR org.freedesktop.dbus.handlers.SignalHandler - java.util.concurrent.TimeoutException
[main] INFO org.purejava.KDEWalletTest2 - Wallet 'kdewallet' successfully closed.

Process finished with exit code 0

The full TRACE on HiDrive.

Thanks for the change to handle these ambiguous signals - I appreciate it very much!