Environment variable breaking change

[ghost]

The following code introduces a breaking change in the API for the upcoming 4.1.1 release that's on the main branch:

    public static DBusConnectionBuilder forSessionBus(String _machineIdFileLocation) {
        BusAddress address = TransportBuilder.getRegisteredBusTypes().contains("UNIX") ? AddressBuilder.getSessionConnection(_machineIdFileLocation) : createTcpAddress();
        address = validateTransportAddress(address);
        DBusConnectionBuilder instance = new DBusConnectionBuilder(address, AddressBuilder.getDbusMachineId(_machineIdFileLocation));
        return instance;
    }

    private static BusAddress createTcpAddress() {
        try {
            return BusAddress.of(System.getProperty("DBUS_TCP_SESSION"));

Although it seems like a relatively simple fix (just declare the DBUS_TCP_SESSION environment variable), it:

• breaks compatibility with the D-Bus command-line tools (dbus-send, dbus-monitor, etc. only need a single DBUS_SESSION_BUS_ADDRESS to configure the D-Bus connection information); and
• breaks our code (InvalidBusAddressException exception org.freedesktop.dbus.exceptions.InvalidBusAddressException: No valid TCP connection address found, please specify 'DBUS_TCP_SESSION' system property).

Fixing this in our code requires changes to operating system files, startup scripts, developer tooling, deployment scripts, and will incur multiple pull requests (ergo code reviews by several developers) across at least two different projects.

Doesn't DBUS_SESSION_BUS_ADDRESS=tcp:host=localhost,port=55000,family=ipv4 contain all the information needed to establish/force a TCP/IP connection regardless of whether the JAR file for UNIX sockets is available? (In our situation, we have removed UNIX JAR files altogether to eliminate any possibility of using the JNI.)

To be backwards compatible with existing systems, would the following work:

1. Try to establish a connection parsing/using DBUS_ESSION_BUS_ADDRESS.
2. If the connection fails, verify that the connection type (TCP vs. UNIX) is supported (e.g., .contains("UNIX")).
3. If the connection type is unsupported (mismatched?), throw an exception.
4. If the connection type is supported and is TCP, fall back to using DBUS_TCP_SESSION.
5. If the connection fails, throw an exception.

Could we do this as 4.1.2?

Note: Forcing implementations to use a different (or additional) environment variable could be considered a breaking API change. Bumping the version to 4.2.0 would seem appropriate in that case. Preferably, we wouldn't need a second session variable.

[hypfvieh]

The usage of DBUS_TCP_SESSION is no breaking change because this setup is used for years now. The original intention was to provide a fallback to TCP if UNIX sockets are not available and also to allow using EmbeddedDBusDaemon in parallel to a running native DBus instance (therefore use of custom property). Also the property was only looked up using System.getProperty, not System.getenv which means it would only be used if set on the command line using JVM -D option or if the calling code has setup this property before.

With the latest additions and changes this seems a bit awkward. DBusConnectionBuilder should use the same code for validation of available transports/used address for both, system and session addresses - currently it only checks system bus addresses.

The special behavior which used DBUS_TCP_SESSION was used in unit tests to setup the TCP address created dynamically in tests when tcp-transport tests were run. This can be changed so it uses DBUS_SESSION_BUS_ADDRESS. In addition the lookup of DBUS_SESSION_BUS_ADDRESS can be changed to first try using System.getProperty and if that does not work, use System.getenv - and if that also fails try to read the session properties file. This will allow overriding DBUS_SESSION_BUS_ADDRESS without messing up the environment variables (and can than safely be used in unit tests or custom code).

I updated the code to do exactly that. The old property is no longer in use.

[ghost]

Replicate

Here are the steps to reproduce the problem.

mkdir $HOME/dev/
cd $HOME/dev/
git clone https://github.com/hypfvieh/dbus-java/
cd dbus-java
wget https://repo1.maven.org/maven2/org/slf4j/slf4j-api/1.7.9/slf4j-api-1.7.9.jar
mvn clean
mvn -Dmaven.test.skip=true -pl -dbus-java-transport-native-unixsocket,-dbus-java-tests,-dbus-java-transport-jnr-unixsocket,-dbus-java-utils,-dbus-java-examples package
cat > T.java <<- "EOF"
import org.freedesktop.dbus.connections.impl.*;

public class T {
  public static void main( String args[] ) throws Exception {
    DBusConnectionBuilder.forSessionBus().build();
  }
}
EOF
export CLASSPATH=$(find . -type f -name "*-SNAPSHOT.jar" -or -name "slf4j*" | tr '\n' ':').
export DBUS_SESSION_BUS_ADDRESS=tcp:host=192.168.66.159,port=55000,family=ipv4
javac -cp $CLASSPATH T.java
java -cp $CLASSPATH T

Expected results

No exception. This is the behaviour for 4.1.0.

Actual results

Exception is thrown, D-Bus connection via TCP/IP fails. Is this a regression?

Exception in thread "main" org.freedesktop.dbus.exceptions.InvalidBusAddressException: No valid TCP connection address found, please specify 'DBUS_TCP_SESSION' system property
    at org.freedesktop.dbus.connections.impl.DBusConnectionBuilder.createTcpAddress(DBusConnectionBuilder.java:56)
    at org.freedesktop.dbus.connections.impl.DBusConnectionBuilder.forSessionBus(DBusConnectionBuilder.java:46)
    at org.freedesktop.dbus.connections.impl.DBusConnectionBuilder.forSessionBus(DBusConnectionBuilder.java:77)
    at T.main(T.java:5)

If this isn't a regression, then I'd consider it a breaking change because it was working in 4.1.0.

Work around

Here's how we fixed the problem, continuing from the previous steps:

cat > DBusConnectionBuilder.patch <<- "EOF"
diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/DBusConnectionBuilder.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/DBusConnectionBuilder.java
index 70faf4a..027579a 100644
--- a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/DBusConnectionBuilder.java
+++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/DBusConnectionBuilder.java
@@ -41,9 +41,7 @@ public class DBusConnectionBuilder extends BaseConnectionBuilder<DBusConnectionB
      * @return {@link DBusConnectionBuilder}
      */
     public static DBusConnectionBuilder forSessionBus(String _machineIdFileLocation) {
-        BusAddress address = TransportBuilder.getRegisteredBusTypes().contains("UNIX") ? // unix socket provider available
-                AddressBuilder.getSessionConnection(_machineIdFileLocation) // use session based on file/environment
-                    : createTcpAddress(); // use TCP fallback when no unix socket provider found
+        BusAddress address = AddressBuilder.getSessionConnection(_machineIdFileLocation);
         address = validateTransportAddress(address);
         DBusConnectionBuilder instance = new DBusConnectionBuilder(address, getDbusMachineId(_machineIdFileLocation));
         return instance;
EOF
git apply DBusConnectionBuilder.patch
mvn -Dmaven.test.skip=true -pl -dbus-java-transport-native-unixsocket,-dbus-java-tests,-dbus-java-transport-jnr-unixsocket,-dbus-java-utils,-dbus-java-examples package
java -cp $CLASSPATH T

No exception is thrown, TCP/IP connection to D-Bus is established.

[hypfvieh]

I would say this was a regression which was fixed in my latest commit yesterday. It is not (or no longer) reproducible using your sample above. For me it fails when trying to connect due to a 'Connect refused' error because I don't have any DBus listening on TCP. The InvalidBusAddressException is not thrown (and should not be thrown as long as TCP transport is available on classpath and the given bus address is well-formed).

Also I'm not sure yet whether the next version will be 4.1.1 or 4.2.0. Considering the amount and size of all changes part of the current SNAPSHOT it would be reasonable to bump the version to 4.2.0.

[ghost]

bump the version to 4.2.0

Yes, that fits with https://semver.org/#spec-item-7

Closing, thank you for the update.