Closed lbeuster closed 4 years ago
I'm pretty sure that the sychronized blocks are not needed because the underlying map is already a ConcurrentHashMap.
I added a new branch which removes the sychronized blocks and will also change the value of the signal lists from List<> to Queue<>, so the value should also be threadsafe like the map.
Please try the changes here: https://github.com/hypfvieh/dbus-java/tree/no-sychronized I'll merge it upstream if we are sure that this will not cause other issues.
The map may be thread safe for single method calls, but you still need sync across several calls, e.g. addSigHandler
sync {
get()
doSomething
put/add
}
And beside this the access to the dbusSignalList (ArrayList) is also not synchronized.
Maybe Map.computeXXX/putIfAbsent will help.
That is what the old code was doing, synchronize while some work is performed.
I don't think the get() needs to be in the synchronized block, because get() on ConcurrentHashMap will be atomic.
I was already thinking of the computeXX methods but this does not work when throwing exceptions. Every time e.g. addMatch is called a DBusExceptionException maybe thrown. Catching, wrapping and re-throwing is something I dont want to do.
I've pushed some changes to the new branch which will use computeIfAbsent to create a new collection. Calling addMatch is performed outside of the lambda so the exception does not have to be caught and rethrown.
That was the point, the addMatch must be outside of any synchronization code ;)
Thanks for the quick fix - works for me.
With the current master if have indeterministic synchronization issues resulting in 'org.freedesktop.dbus.errors.NoReply: No reply within specified time'. This issue came with commit cfa0e0f8932dbd15aff760aeed1e244a2ce037b6.
The problem is the synchronization of getHandledSignals in DBusConnection.addSigHandler. While holding the lock on the handlers a remote call (dbus.AddMatch) is executed (in the previous commit this call was outside of the sync-block). At the same time a NameAcquired messages/signal NameAcquired in that also needs the lock on the handlers - but the signal has to wait because it is hold by addSigHandler. Seems like the server NameAcquired-signal and the clients AddMatch request block each other.
Here are some log statements showing the problem
I have tested inside a Ubuntu 18.04 VM with 4 processors.