Signals are not handled in the order they are sent

[brett-smith]

I am using the Freedesktop Notification API (https://specifications.freedesktop.org/notification-spec/latest/ar01s09.html). Everything works fine, except for handling of "Actions", i.e. Buttons on the notification popup (or the default click).

When a notification action is activated, two signals will be sent. Firstly, ActionInvoked will be sent, followed closely by a NotificationClosed signal. Both of these signals of course contain the ID of the popup.

However, on the client side, there is no guarantee the signals will be processed in the same order they were sent. This makes it impossible to reliably track which popup the signal has come from. In my user code, I am keeping a map of the popup ID and the action handling code. This mapping gets removed when NotificationClosed, is received, so when ActionInvoked is received as the 2nd signal, its mapping has gone. Now I could add a work around in my code, and keep the mapping around for a little while after the notification is closed, but this seems hacky.

The cause is AbstractConnection.workerThreadPool and its default size of 4 threads. Signal handling occurs on this pool, so this explains how signals can arrive out of order.

I am not sure what the DBus specification says about this (if anything), so I haven't created a PR yet. The following ideas occur.

1. Reduce THREAD_COUNT to a fixed value of 1. If users want put their own signal handling on a different thread they can.
2. Don't use workerThreadPool at all, just call on main thread.
3. Make THREAD_COUNT configurable on the connection with a default value of 4 to preserve existing behavior.
4. Make THREAD_COUNT configurable on the connection with a default value of 1.

[hypfvieh]

This is really an "old" issue. It has been raised several times.

Waiting for signals in "main" thread is not a suitable solution. It will always block the whole process and is also single-threaded.

Actually there already is a method to change the threadpool size (Using AbstractConnection.changeThreadCount(int)).
This is currently the only way to get the messages in order.

[brett-smith]

Oh, thanks for quick reply :)

So there is! Ok, thanks, that'll do for now.

[brett-smith]

That's got it. Slightly strange signature on that method, its actually a byte not an int. So connection.changeThreadCount((byte)1).

I wonder if an annotation on the signal impl might be a nice way to solve this. @Synchronous or something, which puts any signals for that on a single threaded executor?

Anyway. Feel free to close.

[hypfvieh]

Nice that this is working for you.
I've investigated the problem one more time, and I think I got a better solution.

The main problem is, that all messages retrieved from DBus (responses) will be put in the same threadpool.
No matter if the message is a signal, a method-call or return or even an error. So the order will always be messed up. The messages are retrieved in order, but you never know which thread will be finish his work first.

To address this, I removed the threadpool (and the method to change its size) and will now use multiple threadpools - one for each type of message. While method-return, signal and error are single threaded pools, the pool for method-call has 4 threads to allow recursive calls of remote methods (a remote method which calls another remote method etc).
This should help when receiving signals as these are handled single-threaded (but not in main thread).
I use threadpools instead of threads, so I always have a valid thread even if the previous thread died somehow.

I also removed some useless code like the "Disconnect" signal which is not part of the DBus protocol spec. DBus spec says a connection can be terminated by the client at any time by closing the connection. There is no need to send a "disconnect" message.

It would be great if you can try the latest changes. As I'm not sure if this is really working code right now, I added a new branch (thread-mess).
Unit tests are all passing, but I think you have a more advanced setup than what the unit tests provide.

[brett-smith]

Awesome. I've tried this out with the notification API, that all works good.

A second more substantial app that uses a lot more of dbus, and in particular makes heavy use of signals, also still works as expected.

However, the removal of the Disconnected signal breaks part of this second app. This app is the one that has an embedded daemon. The daemon runs in one process along with an exported interface. The client makes use of the Disconnected signal to notice when the bus disappears and start a retry cycle waiting for it to come back.

So I am not against the removal of this signal, if there is some other way to know when the bus disappears. I realise I could re-implement the Disconnected signal myself if it comes to it.

[hypfvieh]

This is what the dbus specification says:

There is no corresponding "disconnect" request; if a client wishes to disconnect from the bus, it simply closes the socket (or other communication channel).

So in my eyes doing anything like the Disconnect signal just did is not needed (at least not directly in the library). For any other use case I think you will be better off implementing it yourself.

With the removal of the Disconnect signal I was able to remove some ugly workarounds and special treatments for a feature which is only used if dbus-java is connecting against dbus-java has no use when connecting to the "real" DBus.

The Disconnect signal always was a bit problematic as it has to be handled while the connection is getting closed.

• Stop receiving messages
• Send the signal
• disconnect the transport (close MessageReader/MessageWriter)

This means you have some sort of "partial" disconnect where you still send a message but do not want to read any incoming messages.
So while closing the connection it was forced to send another message to signal the disconnection, and no matter if the other end expected the message or wants to reply to this signal in any way, the connection was just closed.

So if disconnection signaling is a crucial part of your application, it should be implemented on the application itself. This would also allow you to do additional things before the actual connection is closed.
For example the client sends the disconnect to the server and the server can still answer with something before the client closes the connection (e.g. to store/update some information), or signal the client to wait for some more messages etc.

[brett-smith]

Ok understood.

I think what I actually want here is notification the underyling transport for a DBUSConnection has closed for any reason. I don't really care why it was closed, just that "The bus is gone". The usage of the disconnect signal was just a way that seemed to work. Given you description above I wonder if it wasn't working as well as I thought, such as if the broker was kill -9'd.

I don't think I can handle this reliably in my own application without creating a custom transport? It would not be possible to extend DBusConnection and override some methods. A new listener on DBusConnection would be ideal. Something like addCloseListener(CloseListener listener). CloseListener should be a single method interface, perhaps something like closed(String reason).

[hypfvieh]

Handling unexpected disconnections is always a difficult topic.
So you asking for some sort of callback (not some message sent over DBus).

The transport is providing the connection over an arbitrary channel (e.g. tcp/unix/udp/pipe/whatever).
The resulting SocketChannel of the provider is passed to the MessageReader/MessageWriter implementation (which maybe custom in case you use FileDescriptor).
The default MessageReader will simply call read(buf) on the SocketChannel. This will throw an EOFException if the read(buf) methods reaches the end of the stream.

This means if the connection is killed for any reason, an EOFException will be thrown and will later be catched in AbstractConnection.readIncoming().
In that method the exception might be suppressed if the exception arrives while the connection is shutting down (the disconnect was a requested), or it will be suppressed if the current connection is a listening connection (so no exception will be logged or thrown if a client disconnects). In all other cases a FatalDBusException will be thrown.

The question is: When do you want to get notified? Only on unintended disconnects (when FatalDBusException is thrown) or also when a client disconnects (if you are the server)?

[brett-smith]

Yes that sounds perfect. On the client, I do not need to know if the disconnection was intended, i.e. disconnect() or close() is called. I do need to know if the disconnection was due to the transport closing.

I do not personally need to capture disconnections on the server, although can see that it may be useful.

[hypfvieh]

I just updated the branch adding support for IDisconnectCallback to AbstractConnection.
You may try it.

I will also put some more time in clean-up stuff... Some parts of the library do not "feel good".
I don't like the various constructors for DBusConnection...
For example: If you want to register the new callback, you have to create the session, then register the callback - but the connection will already be established in most cases before you can do that. If the connection fails between creation and registration of the callback, the callback will never be used.

I'm thinking about deprecating the various constructors and add a factory or builder to create new connections (like I did for transports). This way it would be possible to chain all required options for bootstrapping the connection before it will be connected at all.

Also the 'shared' connection stuff in DBusConnection is something which should be handled elsewhere if possible.
Maybe I'll find some time to investigate that in the next days.

[brett-smith]

That works well.

Only one very minor point, is that the logging output is slightly excessive. You'll see 2 exceptions when the connection is severed.

03-02-2022 16:20:54 [DBusConnection [listener=false]] ERROR IncomingMessageThread - FatalException in connection thread
org.freedesktop.dbus.exceptions.FatalDBusException: java.io.EOFException: Underlying transport returned EOF (1)
	at org.freedesktop.dbus.connections.AbstractConnection.readIncoming(AbstractConnection.java:1113)
	at org.freedesktop.dbus.connections.IncomingMessageThread.run(IncomingMessageThread.java:41)
Caused by: java.io.EOFException: Underlying transport returned EOF (1)
	at org.freedesktop.dbus.spi.message.InputStreamMessageReader.readMessage(InputStreamMessageReader.java:53)
	at org.freedesktop.dbus.connections.transports.AbstractTransport.readMessage(AbstractTransport.java:85)
	at org.freedesktop.dbus.connections.AbstractConnection.readIncoming(AbstractConnection.java:1102)
	... 1 more
03-02-2022 16:20:54 [DBusConnection [listener=false]] INFO  AbstractDBusClient - Stopping pinging.
03-02-2022 16:20:54 [DBusConnection [listener=false]] WARN  ReceivingService - Interrupted while waiting for termination of executor
java.lang.InterruptedException
	at java.base/java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.awaitNanos(AbstractQueuedSynchronizer.java:1649)
	at java.base/java.util.concurrent.ThreadPoolExecutor.awaitTermination(ThreadPoolExecutor.java:1456)
	at java.base/java.util.concurrent.Executors$DelegatedExecutorService.awaitTermination(Executors.java:709)
	at org.freedesktop.dbus.connections.ReceivingService.shutdown(ReceivingService.java:99)
	at org.freedesktop.dbus.connections.AbstractConnection.internalDisconnect(AbstractConnection.java:539)
	at org.freedesktop.dbus.connections.AbstractConnection.disconnect(AbstractConnection.java:565)
	at org.freedesktop.dbus.connections.impl.DBusConnection.disconnect(DBusConnection.java:987)
	at com.logonbox.vpn.common.client.AbstractDBusClient.busGone(AbstractDBusClient.java:316)
	at com.logonbox.vpn.common.client.AbstractDBusClient$2.disconnectOnError(AbstractDBusClient.java:216)
	at org.freedesktop.dbus.connections.AbstractConnection.lambda$2(AbstractConnection.java:532)
	at java.base/java.util.Optional.ifPresentOrElse(Optional.java:196)
	at org.freedesktop.dbus.connections.AbstractConnection.lambda$1(AbstractConnection.java:532)
	at java.base/java.util.Optional.ifPresent(Optional.java:178)
	at org.freedesktop.dbus.connections.AbstractConnection.internalDisconnect(AbstractConnection.java:530)
	at org.freedesktop.dbus.connections.IncomingMessageThread.run(IncomingMessageThread.java:58)

And I look forward to further improvements :)

[hypfvieh]

I merged the changes of the branch and also added some more changes like Builder for connections, allow changing the thread pool sizes, etc.