Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong signal constructor retrieved #110

Closed
purejava opened this issue Jul 16, 2020 · 12 comments
Closed

Wrong signal constructor retrieved #110

purejava opened this issue Jul 16, 2020 · 12 comments

Comments

@purejava
Copy link

purejava commented Jul 16, 2020

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
Copy link
Owner

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
Copy link
Author

Thanks for the suggestion.
I changed the code accordingly, see purejava/dbusjavatest@55e978f, but unfortunately it still fails.

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

@hypfvieh
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Author

purejava commented Jul 25, 2020

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.

@purejava purejava reopened this Jul 25, 2020
@hypfvieh
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

I updated the branch, could you please try again?

@purejava
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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!

hypfvieh added a commit that referenced this issue Aug 8, 2020
The changes introduced in #110 did not cover the use case of interfaces classes
used as constructor arguments. When e.g. using Map or List interface in constructor the
current expectedArgumentsList.equals(receivedArgumentsList) will fail, because the received arguments
are not the interface type, but a compatible class.
This requires some more attention, which means every expectedArgument has be be changed for
compatibility to the received argument type.

This behavior has been added in this commit.
Every received argument is now checked with 'isAssignable' to the expected arguments.
This will allow inheritance for objects as well as allowing the usage of interface classes in constructor
definitions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants