[knx] Improve thread safety, null-analysis (openhab#14509)

Carryover from smarthomej/addons#13 and smarthomej/addons#46, smarthomej/addons#60. Also-by: Jan N. Klug <github@klug.nrw> Signed-off-by: Holger Friedrich <mail@holger-friedrich.de>
FordPrfkt · Apr 19, 2023 · aa01e88 · aa01e88
1 parent 493bb2c
commit aa01e88
Show file tree

Hide file tree

Showing 10 changed files with 83 additions and 67 deletions.
diff --git a/...org.openhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/KNXTypeMapper.java b/...org.openhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/KNXTypeMapper.java
@@ -38,7 +38,7 @@ public interface KNXTypeMapper {
      * @return datapoint value as a string
      */
     @Nullable
-    public String toDPTValue(Type type, @Nullable String dpt);
+    String toDPTValue(Type type, @Nullable String dpt);
 
     /**
      * maps a datapoint value to an openHAB command or state
@@ -49,8 +49,8 @@ public interface KNXTypeMapper {
      * @return a command or state of openHAB
      */
     @Nullable
-    public Type toType(Datapoint datapoint, byte[] data);
+    Type toType(Datapoint datapoint, byte[] data);
 
     @Nullable
-    public Class<? extends Type> toTypeClass(@Nullable String dpt);
+    Class<? extends Type> toTypeClass(@Nullable String dpt);
 }
diff --git a/...penhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/channel/TypeDimmer.java b/...penhab.binding.knx/src/main/java/org/openhab/binding/knx/internal/channel/TypeDimmer.java
@@ -12,12 +12,10 @@
  */
 package org.openhab.binding.knx.internal.channel;
 
-import static java.util.stream.Collectors.toSet;
 import static org.openhab.binding.knx.internal.KNXBindingConstants.*;
 
 import java.util.Objects;
 import java.util.Set;
-import java.util.stream.Stream;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 
@@ -40,7 +38,7 @@ class TypeDimmer extends KNXChannelType {
 
     @Override
     protected Set<String> getAllGAKeys() {
-        return Stream.of(SWITCH_GA, POSITION_GA, INCREASE_DECREASE_GA).collect(toSet());
+        return Set.of(SWITCH_GA, POSITION_GA, INCREASE_DECREASE_GA);
     }
 
     @Override

diff --git a/...binding.knx/src/main/java/org/openhab/binding/knx/internal/channel/TypeRollershutter.java b/...binding.knx/src/main/java/org/openhab/binding/knx/internal/channel/TypeRollershutter.java
@@ -12,12 +12,10 @@
  */
 package org.openhab.binding.knx.internal.channel;
 
-import static java.util.stream.Collectors.toSet;
 import static org.openhab.binding.knx.internal.KNXBindingConstants.*;
 
 import java.util.Objects;
 import java.util.Set;
-import java.util.stream.Stream;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 
@@ -53,6 +51,6 @@ protected String getDefaultDPT(String gaConfigKey) {
 
     @Override
     protected Set<String> getAllGAKeys() {
-        return Stream.of(UP_DOWN_GA, STOP_MOVE_GA, POSITION_GA).collect(toSet());
+        return Set.of(UP_DOWN_GA, STOP_MOVE_GA, POSITION_GA);
     }
 }
diff --git a/....binding.knx/src/main/java/org/openhab/binding/knx/internal/client/AbstractKNXClient.java b/....binding.knx/src/main/java/org/openhab/binding/knx/internal/client/AbstractKNXClient.java
@@ -297,12 +297,11 @@ private synchronized void disconnect(@Nullable Exception e, Optional<ThingStatus
         }
     }
 
-    @SuppressWarnings("null")
     protected void releaseConnection() {
         logger.debug("Bridge {} is disconnecting from KNX bus", thingUID);
         var tmplink = link;
         if (tmplink != null) {
-            link.removeLinkListener(this);
+            tmplink.removeLinkListener(this);
         }
         busJob = nullify(busJob, j -> j.cancel(true));
         readDatapoints.clear();
@@ -321,7 +320,7 @@ protected void releaseConnection() {
         logger.trace("Bridge {} disconnected from KNX bus", thingUID);
     }
 
-    private <T> @Nullable T nullify(T target, @Nullable Consumer<T> lastWill) {
+    private <T> @Nullable T nullify(@Nullable T target, @Nullable Consumer<T> lastWill) {
         if (target != null && lastWill != null) {
             lastWill.accept(target);
         }

diff --git a/...hab.binding.knx/src/main/java/org/openhab/binding/knx/internal/dpt/KNXCoreTypeMapper.java b/...hab.binding.knx/src/main/java/org/openhab/binding/knx/internal/dpt/KNXCoreTypeMapper.java
@@ -977,7 +977,7 @@ private String quantityTypeToDPTValue(QuantityType<?> qt, int mainNumber, int su
             if (typeClass.equals(DateTimeType.class)) {
                 String date = formatDateTime(value, datapoint.getDPT());
                 if (date.isEmpty()) {
-                    logger.debug("toType: KNX clock msg ignored: date object null or empty {}.", date);
+                    logger.debug("toType: KNX clock msg ignored: date object empty {}.", date);
                     return null;
                 } else {
                     return DateTimeType.valueOf(date);
@@ -1047,7 +1047,7 @@ private String quantityTypeToDPTValue(QuantityType<?> qt, int mainNumber, int su
      * @return a formatted String like </code>yyyy-MM-dd'T'HH:mm:ss</code> which
      *         is target format of the {@link DateTimeType}
      */
-    private String formatDateTime(String value, String dpt) {
+    private String formatDateTime(String value, @Nullable String dpt) {
         Date date = null;
 
         try {

diff --git a/...binding.knx/src/main/java/org/openhab/binding/knx/internal/factory/KNXHandlerFactory.java b/...binding.knx/src/main/java/org/openhab/binding/knx/internal/factory/KNXHandlerFactory.java
@@ -14,8 +14,8 @@
 
 import static org.openhab.binding.knx.internal.KNXBindingConstants.*;
 
-import java.util.Arrays;
 import java.util.Collection;
+import java.util.Set;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
@@ -50,17 +50,19 @@
 @Component(service = ThingHandlerFactory.class, configurationPid = "binding.knx")
 public class KNXHandlerFactory extends BaseThingHandlerFactory {
 
-    public static final Collection<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Arrays.asList(THING_TYPE_DEVICE,
+    public static final Collection<ThingTypeUID> SUPPORTED_THING_TYPES_UIDS = Set.of(THING_TYPE_DEVICE,
             THING_TYPE_IP_BRIDGE, THING_TYPE_SERIAL_BRIDGE);
 
     @Nullable
     private NetworkAddressService networkAddressService;
     private final SerialPortManager serialPortManager;
 
     @Activate
-    public KNXHandlerFactory(final @Reference TranslationProvider translationProvider,
-            final @Reference LocaleProvider localeProvider, final @Reference SerialPortManager serialPortManager) {
+    public KNXHandlerFactory(final @Reference NetworkAddressService networkAddressService,
+            final @Reference TranslationProvider translationProvider, final @Reference LocaleProvider localeProvider,
+            final @Reference SerialPortManager serialPortManager) {
         KNXTranslationProvider.I18N.setProvider(localeProvider, translationProvider);
+        this.networkAddressService = networkAddressService;
         this.serialPortManager = serialPortManager;
         SerialTransportAdapter.setSerialPortManager(serialPortManager);
     }
@@ -84,7 +86,7 @@ public boolean supportsThingType(ThingTypeUID thingTypeUID) {
         if (THING_TYPE_DEVICE.equals(thingTypeUID)) {
             return super.createThing(thingTypeUID, configuration, thingUID, bridgeUID);
         }
-        throw new IllegalArgumentException("The thing type " + thingTypeUID + " is not supported by the KNX binding.");
+        return null;
     }
 
     @Override
@@ -116,13 +118,4 @@ private ThingUID getSerialBridgeThingUID(ThingTypeUID thingTypeUID, @Nullable Th
         String serialPort = (String) configuration.get(SERIAL_PORT);
         return new ThingUID(thingTypeUID, serialPort);
     }
-
-    @Reference
-    protected void setNetworkAddressService(NetworkAddressService networkAddressService) {
-        this.networkAddressService = networkAddressService;
-    }
-
-    protected void unsetNetworkAddressService(NetworkAddressService networkAddressService) {
-        this.networkAddressService = null;
-    }
 }
diff --git a/...g.knx/src/main/java/org/openhab/binding/knx/internal/handler/AbstractKNXThingHandler.java b/...g.knx/src/main/java/org/openhab/binding/knx/internal/handler/AbstractKNXThingHandler.java
@@ -52,7 +52,7 @@ public abstract class AbstractKNXThingHandler extends BaseThingHandler implement
     private final Logger logger = LoggerFactory.getLogger(AbstractKNXThingHandler.class);
 
     protected @Nullable IndividualAddress address;
-    private @Nullable Future<?> descriptionJob;
+    private @Nullable ScheduledFuture<?> descriptionJob;
     private boolean filledDescription = false;
     private final Random random = new Random();
 

diff --git a/...inding.knx/src/main/java/org/openhab/binding/knx/internal/handler/DeviceThingHandler.java b/...inding.knx/src/main/java/org/openhab/binding/knx/internal/handler/DeviceThingHandler.java
@@ -15,13 +15,12 @@
 import static org.openhab.binding.knx.internal.KNXBindingConstants.*;
 
 import java.math.BigDecimal;
-import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
 
@@ -69,11 +68,11 @@ public class DeviceThingHandler extends AbstractKNXThingHandler {
     private final Logger logger = LoggerFactory.getLogger(DeviceThingHandler.class);
 
     private final KNXTypeMapper typeHelper = new KNXCoreTypeMapper();
-    private final Set<GroupAddress> groupAddresses = new HashSet<>();
-    private final Set<GroupAddress> groupAddressesWriteBlockedOnce = new HashSet<>();
-    private final Set<OutboundSpec> groupAddressesRespondingSpec = new HashSet<>();
-    private final Map<GroupAddress, ScheduledFuture<?>> readFutures = new HashMap<>();
-    private final Map<ChannelUID, ScheduledFuture<?>> channelFutures = new HashMap<>();
+    private final Set<GroupAddress> groupAddresses = ConcurrentHashMap.newKeySet();
+    private final Set<GroupAddress> groupAddressesWriteBlockedOnce = ConcurrentHashMap.newKeySet();
+    private final Set<OutboundSpec> groupAddressesRespondingSpec = ConcurrentHashMap.newKeySet();
+    private final Map<GroupAddress, ScheduledFuture<?>> readFutures = new ConcurrentHashMap<>();
+    private final Map<ChannelUID, ScheduledFuture<?>> channelFutures = new ConcurrentHashMap<>();
     private int readInterval;
 
     public DeviceThingHandler(Thing thing) {
@@ -99,33 +98,33 @@ private void initializeGroupAddresses() {
     @Override
     public void dispose() {
         cancelChannelFutures();
-        freeGroupAdresses();
+        freeGroupAddresses();
         super.dispose();
     }
 
     private void cancelChannelFutures() {
-        for (ScheduledFuture<?> future : channelFutures.values()) {
-            if (!future.isDone()) {
-                future.cancel(true);
-            }
+        for (ChannelUID channelUID : channelFutures.keySet()) {
+            channelFutures.computeIfPresent(channelUID, (k, v) -> {
+                v.cancel(true);
+                return null;
+            });
         }
-        channelFutures.clear();
     }
 
-    private void freeGroupAdresses() {
+    private void freeGroupAddresses() {
         groupAddresses.clear();
         groupAddressesWriteBlockedOnce.clear();
         groupAddressesRespondingSpec.clear();
     }
 
     @Override
     protected void cancelReadFutures() {
-        for (ScheduledFuture<?> future : readFutures.values()) {
-            if (!future.isDone()) {
-                future.cancel(true);
-            }
+        for (GroupAddress groupAddress : readFutures.keySet()) {
+            readFutures.computeIfPresent(groupAddress, (k, v) -> {
+                v.cancel(true);
+                return null;
+            });
         }
-        readFutures.clear();
     }
 
     @FunctionalInterface
@@ -220,7 +219,9 @@ public boolean listensTo(GroupAddress destination) {
     @SuppressWarnings("null")
     private void rememberRespondingSpec(OutboundSpec commandSpec, boolean add) {
         GroupAddress ga = commandSpec.getGroupAddress();
-        groupAddressesRespondingSpec.removeIf(spec -> spec.getGroupAddress().equals(ga));
+        if (ga != null) {
+            groupAddressesRespondingSpec.removeIf(spec -> spec.getGroupAddress().equals(ga));
+        }
         if (add) {
             groupAddressesRespondingSpec.add(commandSpec);
         }
@@ -393,18 +394,18 @@ private void processDataReceived(GroupAddress destination, byte[] asdu, InboundS
                         && (type instanceof UnDefType || type instanceof IncreaseDecreaseType) && frequency > 0) {
                     // continuous dimming by the binding
                     if (UnDefType.UNDEF.equals(type)) {
-                        ScheduledFuture<?> future = channelFutures.remove(channelUID);
-                        if (future != null) {
-                            future.cancel(false);
-                        }
+                        channelFutures.computeIfPresent(channelUID, (k, v) -> {
+                            v.cancel(false);
+                            return null;
+                        });
                     } else if (type instanceof IncreaseDecreaseType) {
-                        ScheduledFuture<?> future = scheduler.scheduleWithFixedDelay(() -> {
-                            postCommand(channelUID, (Command) type);
-                        }, 0, frequency, TimeUnit.MILLISECONDS);
-                        ScheduledFuture<?> previousFuture = channelFutures.put(channelUID, future);
-                        if (previousFuture != null) {
-                            previousFuture.cancel(true);
-                        }
+                        channelFutures.compute(channelUID, (k, v) -> {
+                            if (v != null) {
+                                v.cancel(true);
+                            }
+                            return scheduler.scheduleWithFixedDelay(() -> postCommand(channelUID, (Command) type), 0,
+                                    frequency, TimeUnit.MILLISECONDS);
+                        });
                     }
                 } else {
                     if (type instanceof Command) {

diff --git a/...ding.knx/src/main/java/org/openhab/binding/knx/internal/handler/IPBridgeThingHandler.java b/...ding.knx/src/main/java/org/openhab/binding/knx/internal/handler/IPBridgeThingHandler.java
@@ -175,8 +175,9 @@ public void initializeLater() {
                 logger.debug("NetworkAddressService not available, cannot create bridge {}", thing.getUID());
                 updateStatus(ThingStatus.OFFLINE);
                 return;
+            } else {
+                localEndPoint = new InetSocketAddress(networkAddressService.getPrimaryIpv4HostAddress(), 0);
             }
-            localEndPoint = new InetSocketAddress(networkAddressService.getPrimaryIpv4HostAddress(), 0);
         }
 
         updateStatus(ThingStatus.UNKNOWN);

diff --git a/...inding.knx/src/test/java/org/openhab/binding/knx/internal/channel/KNXChannelTypeTest.java b/...inding.knx/src/test/java/org/openhab/binding/knx/internal/channel/KNXChannelTypeTest.java
@@ -36,67 +36,93 @@ void setup() {
         ct = new MyKNXChannelType("");
     }
 
-    @SuppressWarnings("null")
     @Test
     void testParseWithDptMultipleWithRead() {
         ChannelConfiguration res = ct.parse("5.001:<1/3/22+0/3/22+<0/8/15");
 
+        if (res == null) {
+            fail();
+            return;
+        }
+
         assertEquals("5.001", res.getDPT());
         assertEquals("1/3/22", res.getMainGA().getGA());
         assertTrue(res.getMainGA().isRead());
         assertEquals(3, res.getListenGAs().size());
         assertEquals(2, res.getReadGAs().size());
     }
 
-    @SuppressWarnings("null")
     @Test
     void testParseWithDptMultipleWithoutRead() {
         ChannelConfiguration res = ct.parse("5.001:1/3/22+0/3/22+0/8/15");
 
+        if (res == null) {
+            fail();
+            return;
+        }
+
         assertEquals("5.001", res.getDPT());
         assertEquals("1/3/22", res.getMainGA().getGA());
         assertFalse(res.getMainGA().isRead());
         assertEquals(3, res.getListenGAs().size());
         assertEquals(0, res.getReadGAs().size());
     }
 
-    @SuppressWarnings("null")
     @Test
     void testParseWithoutDptSingleWithoutRead() {
         ChannelConfiguration res = ct.parse("1/3/22");
 
+        if (res == null) {
+            fail();
+            return;
+        }
+
         assertNull(res.getDPT());
         assertEquals("1/3/22", res.getMainGA().getGA());
         assertFalse(res.getMainGA().isRead());
         assertEquals(1, res.getListenGAs().size());
         assertEquals(0, res.getReadGAs().size());
     }
 
-    @SuppressWarnings("null")
     @Test
     void testParseWithoutDptSingleWitRead() {
         ChannelConfiguration res = ct.parse("<1/3/22");
 
+        if (res == null) {
+            fail();
+            return;
+        }
+
         assertNull(res.getDPT());
         assertEquals("1/3/22", res.getMainGA().getGA());
         assertTrue(res.getMainGA().isRead());
         assertEquals(1, res.getListenGAs().size());
         assertEquals(1, res.getReadGAs().size());
     }
 
-    @SuppressWarnings("null")
     @Test
     void testParseTwoLevel() {
         ChannelConfiguration res = ct.parse("5.001:<3/1024+<4/1025");
+
+        if (res == null) {
+            fail();
+            return;
+        }
+
         assertEquals("3/1024", res.getMainGA().getGA());
         assertEquals(2, res.getListenGAs().size());
         assertEquals(2, res.getReadGAs().size());
     }
 
-    @SuppressWarnings("null")
     @Test
     void testParseFreeLevel() {
         ChannelConfiguration res = ct.parse("5.001:<4610+<4611");
+
+        if (res == null) {
+            fail();
+            return;
+        }
+
         assertEquals("4610", res.getMainGA().getGA());
         assertEquals(2, res.getListenGAs().size());
         assertEquals(2, res.getReadGAs().size());