AbstractConnection.changeThreadCount() is fragile

[lbeuster]

The AbstractConnection.changeThreadCount() method is fragile since 3.x

1. A local var for workerThreadCount should be inroduced. Now a change to 3 and back to 4 would leave the worker count at 3 - or remove the check of old and new count at all.

2. There is a small time window when no valid executor is available, the old already shutdown, the new not created yet. This (eventually in combination with the next point 3.) results in

   org.freedesktop.dbus.connections.AbstractConnection$3@7a720633 rejected from java.util.concurrent.ThreadPoolExecutor@5855cbc6[Terminated, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 0]
   at java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(ThreadPoolExecutor.java:2063)
   at java.util.concurrent.ThreadPoolExecutor.reject(ThreadPoolExecutor.java:830)
   at java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1379)
   at org.freedesktop.dbus.connections.AbstractConnection.handleMessage(AbstractConnection.java:834)
   at org.freedesktop.dbus.connections.AbstractConnection.handleMessage(AbstractConnection.java:615)
   at org.freedesktop.dbus.connections.IncomingMessageThread.run(IncomingMessageThread.java:43)

   The stack trace is from 3.0.2, where the error occurred much more than in the current 3.2.0-SNAPSHOT. Could be fixed with the following code, but may change the order of the executed tasks (which should not matter anyway):

   ExecutorService oldWorkerThreadPool = this.workerThreadPool; 
   this.workerThreadPool =  Executors.newFixedThreadPool(newcount, ...);
   List<Runnable> remainingTasks = oldWorkerThreadPool.shutdownNow();

3. Since the workerThreadPool reference is mutable and shared across different threads it should be an AtomicReference (or volatile).

[lbeuster]

BTW: It's a great idea to remove the native code. It crashes the VM on ARM and Mac from time to time. ;)

[hypfvieh]

I've implemented your suggestions. The check for newcount vs. THREADCOUNT is nonsense ... It would always do nothing when the default thread count is used.

I changed it so it compares the new value vs. the max thread count of the current ExecutorService.

[lbeuster]

Thanks 👍

[lbeuster]

Unfortunately this fix did not solve the issue because the methods of the ExecutorService itself are of course not protected. I created a pull request (#60) that uses read/write locking on the thread pool.

With the new fix I did not have the problem anymore.

[Emill]

It would be nice to be able to set the thread pool count before connecting. Now we have to get a default-sized thread pool and once the connection is created it can be resized to 1.

Having the thread pool of size 1 is the only way, as far as I know, to make sure the signals are delivered in the correct order, which might be important for some applications.