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

Pass thread priority setting into NameableThreadFactory constructor #190

Closed ghost closed 1 year ago

ghost commented 1 year ago

We've hooked into the D-Bus using 4.2.1 as follows:

return DBusConnectionBuilder
        .forSessionBus()
        .receivingThreadConfig()
        .withSignalThreadPriority(MIN_PRIORITY)
        .withRetryHandler((t, ex) -> {
            // A NullPointerException occurred; the JVM has entered an irrecoverable state.
            new ShutdownEvent(ex).publish();

            // Terminate the D-Bus thread's retry handler.
            return false;
        })
        .connectionConfig()
        .build();

The call to withSignalThreadPriority performs:

config.setSignalThreadPriority(Util.checkIntInRange(_priority, Thread.MIN_PRIORITY, Thread.MAX_PRIORITY))

That call assigns the thread priority:

void setSignalThreadPriority(int _signalThreadPriority) {
  signalThreadPriority = _signalThreadPriority;
}

The signal thread priority doesn't appear to be used because the getSignalThreadPriority() method isn't invoked.

Within the constructor for ReceivingService, a new NameableThreadFactory instance is added to the internal executors map:

ReceivingService(ReceivingServiceConfig _rsCfg) {
  ReceivingServiceConfig rsCfg = Optional.ofNullable(_rsCfg).orElse(ReceivingServiceConfigBuilder.getDefaultConfig());
  executors.put(ExecutorNames.SIGNAL, Executors.newFixedThreadPool(rsCfg.getSignalThreadPoolSize(), new NameableThreadFactory("DBus-Signal-Receiver-", true)));

The ReceivingService invokes the two-argument constructor for NameableThreadFactory, which uses a default priority of Thread.NORM_PRIORITY:

public NameableThreadFactory(String _name, boolean _daemonizeThreads) {
  this(_name, _daemonizeThreads,Thread.NORM_PRIORITY);
}

Does the ReceivingService constructor need to call a different NameableThreadFactory constructor? That is, the one that takes the thread priority? Such as by calling _rsCfg.getSignalThreadPriority():

ReceivingService(ReceivingServiceConfig _rsCfg) {
  ReceivingServiceConfig rsCfg = Optional.ofNullable(_rsCfg).orElse(ReceivingServiceConfigBuilder.getDefaultConfig());
  executors.put(ExecutorNames.SIGNAL, Executors.newFixedThreadPool(
    rsCfg.getSignalThreadPoolSize(),
    new NameableThreadFactory(
      "DBus-Signal-Receiver-",
      true,
      _rsCfg.getSignalThreadPriority()))); // Is this parameter needed?

Also, do we need to pass in the priority for the sending side? That is, in AbstractConnection:

  receivingService = new ReceivingService(_rsCfg);
  senderService =
    Executors.newFixedThreadPool(1, new NameableThreadFactory("DBus Sender Thread-", false));

Does that need to pass _rsCfg.getSignalThreadPriority() as well?

hypfvieh commented 1 year ago

Thanks for reporting and explaining everything so well. I appreciate that. But providing a PR to fix these issues would be much more appreciated.
If the PR is fine, I will simply merge it, if there is something I don't like we can discuss that.

This would spare me some time fixing all of that by myself. Even if the solution is easy, I think you should take the credit for fixing because you found the problem.