From 8dd6e86f5897f647896cd96bcf768fa8899ffbac Mon Sep 17 00:00:00 2001 From: Wouter Born Date: Sat, 25 Apr 2020 20:48:35 +0200 Subject: [PATCH] [mihome] Fix NPE, add null annotations and name threads (#7474) Fixes #7425 Signed-off-by: Wouter Born Signed-off-by: Daan Meijer --- .../XiaomiBridgeDiscoveryService.java | 15 +- .../internal/handler/XiaomiBridgeHandler.java | 4 +- .../internal/socket/XiaomiBridgeSocket.java | 28 +-- .../socket/XiaomiDiscoverySocket.java | 17 +- .../mihome/internal/socket/XiaomiSocket.java | 214 ++++++++---------- .../internal/socket/XiaomiSocketListener.java | 3 + 6 files changed, 134 insertions(+), 147 deletions(-) diff --git a/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/discovery/XiaomiBridgeDiscoveryService.java b/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/discovery/XiaomiBridgeDiscoveryService.java index 4b57504d80a65..7887c92dcdb03 100644 --- a/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/discovery/XiaomiBridgeDiscoveryService.java +++ b/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/discovery/XiaomiBridgeDiscoveryService.java @@ -19,6 +19,7 @@ import java.util.Map; import java.util.Set; +import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.smarthome.config.discovery.AbstractDiscoveryService; import org.eclipse.smarthome.config.discovery.DiscoveryResultBuilder; import org.eclipse.smarthome.config.discovery.DiscoveryService; @@ -38,6 +39,7 @@ * @author Patrick Boos - Initial contribution * @author Kuba Wolanin - logger fixes */ +@NonNullByDefault @Component(service = DiscoveryService.class, immediate = true, configurationPid = "discovery.mihome") public class XiaomiBridgeDiscoveryService extends AbstractDiscoveryService implements XiaomiSocketListener { @@ -45,7 +47,7 @@ public class XiaomiBridgeDiscoveryService extends AbstractDiscoveryService imple private static final int DISCOVERY_TIMEOUT_SEC = 30; private final Logger logger = LoggerFactory.getLogger(XiaomiBridgeDiscoveryService.class); - private XiaomiDiscoverySocket socket; + private final XiaomiDiscoverySocket socket = new XiaomiDiscoverySocket("discovery"); public XiaomiBridgeDiscoveryService() { super(SUPPORTED_THING_TYPES, DISCOVERY_TIMEOUT_SEC, false); @@ -53,8 +55,7 @@ public XiaomiBridgeDiscoveryService() { @Override protected void startScan() { - socket = (socket == null) ? new XiaomiDiscoverySocket() : socket; - socket.intialize(); + socket.initialize(); logger.debug("Start scan for bridges"); socket.registerListener(this); discoverGateways(); @@ -65,17 +66,13 @@ protected synchronized void stopScan() { super.stopScan(); logger.debug("Stop scan"); removeOlderResults(getTimestampOfLastScan()); - if (socket != null) { - socket.unregisterListener(this); - } + socket.unregisterListener(this); } @Override public void deactivate() { super.deactivate(); - if (socket != null) { - socket.unregisterListener(this); - } + socket.unregisterListener(this); } @Override diff --git a/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/handler/XiaomiBridgeHandler.java b/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/handler/XiaomiBridgeHandler.java index acd67512bbb19..842839aebeabb 100644 --- a/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/handler/XiaomiBridgeHandler.java +++ b/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/handler/XiaomiBridgeHandler.java @@ -131,8 +131,8 @@ public void initialize() { return; } logger.debug("Init socket on Port: {}", port); - socket = new XiaomiBridgeSocket(port); - socket.intialize(); + socket = new XiaomiBridgeSocket(port, getThing().getUID().getId()); + socket.initialize(); socket.registerListener(this); scheduler.schedule(() -> { diff --git a/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiBridgeSocket.java b/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiBridgeSocket.java index b21a401e5d901..b78af9ab9d783 100644 --- a/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiBridgeSocket.java +++ b/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiBridgeSocket.java @@ -13,10 +13,10 @@ package org.openhab.binding.mihome.internal.socket; import java.io.IOException; -import java.net.DatagramSocket; import java.net.InetAddress; import java.net.MulticastSocket; +import org.eclipse.jdt.annotation.NonNullByDefault; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -26,35 +26,37 @@ * @author Dieter Schmidt - Initial contribution * */ +@NonNullByDefault public class XiaomiBridgeSocket extends XiaomiSocket { private final Logger logger = LoggerFactory.getLogger(XiaomiBridgeSocket.class); - public XiaomiBridgeSocket(int port) { - super(port); + public XiaomiBridgeSocket(int port, String owner) { + super(port, owner); } /** * Sets up the {@link XiaomiBridgeSocket}. * * Connects the socket to the specific multicast address and port. - * Starts the {@link ReceiverThread} for the socket. */ @Override - synchronized DatagramSocket setupSocket() { - DatagramSocket openSocket = getOpenSockets().get(getPort()); - if (openSocket != null) { - return openSocket; + protected synchronized void setupSocket() { + MulticastSocket socket = (MulticastSocket) getSocket(); + if (socket != null) { + logger.debug("Socket already setup"); + return; } + try { logger.debug("Setup socket"); - setSocket(new MulticastSocket(getPort())); // must bind receive side - ((MulticastSocket) getSocket()).joinGroup(InetAddress.getByName(MCAST_ADDR)); - logger.debug("Initialized socket to {}:{} on {}:{}", getSocket().getRemoteSocketAddress(), - getSocket().getPort(), getSocket().getLocalAddress(), getSocket().getLocalPort()); + socket = new MulticastSocket(getPort()); + setSocket(socket); // must bind receive side + socket.joinGroup(InetAddress.getByName(MCAST_ADDR)); + logger.debug("Initialized socket to {}:{} on {}:{}", socket.getRemoteSocketAddress(), socket.getPort(), + socket.getLocalAddress(), socket.getLocalPort()); } catch (IOException e) { logger.error("Setup socket error", e); } - return getSocket(); } } diff --git a/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiDiscoverySocket.java b/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiDiscoverySocket.java index ca4058824c856..c76afe41f13e3 100644 --- a/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiDiscoverySocket.java +++ b/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiDiscoverySocket.java @@ -17,6 +17,7 @@ import java.net.InetAddress; import java.net.UnknownHostException; +import org.eclipse.jdt.annotation.NonNullByDefault; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -26,35 +27,35 @@ * @author Dieter Schmidt - Initial contribution * */ +@NonNullByDefault public class XiaomiDiscoverySocket extends XiaomiSocket { private static final int MCAST_PORT = 4321; private final Logger logger = LoggerFactory.getLogger(XiaomiDiscoverySocket.class); - public XiaomiDiscoverySocket() { - super(); + public XiaomiDiscoverySocket(String owner) { + super(owner); } /** * Sets up the {@link XiaomiDiscoverySocket}. * * Connects the socket to the specific multicast address and port. - * Starts the {@link ReceiverThread} for the socket. */ @Override - DatagramSocket setupSocket() { + protected void setupSocket() { synchronized (XiaomiDiscoverySocket.class) { try { logger.debug("Setup discovery socket"); - setSocket(new DatagramSocket(0)); - logger.debug("Initialized socket to {}:{} on {}:{}", getSocket().getInetAddress(), - getSocket().getPort(), getSocket().getLocalAddress(), getSocket().getLocalPort()); + DatagramSocket socket = new DatagramSocket(0); + setSocket(socket); + logger.debug("Initialized socket to {}:{} on {}:{}", socket.getInetAddress(), socket.getPort(), + socket.getLocalAddress(), socket.getLocalPort()); } catch (IOException e) { logger.error("Setup socket error", e); } } - return getSocket(); } /** diff --git a/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiSocket.java b/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiSocket.java index d999845ac7a84..b6f50e3ed0021 100644 --- a/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiSocket.java +++ b/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiSocket.java @@ -17,11 +17,11 @@ import java.net.DatagramSocket; import java.net.InetAddress; import java.nio.charset.StandardCharsets; -import java.util.List; -import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.CopyOnWriteArrayList; +import java.util.Set; +import java.util.concurrent.CopyOnWriteArraySet; +import org.eclipse.jdt.annotation.NonNullByDefault; +import org.eclipse.jdt.annotation.Nullable; import org.openhab.binding.mihome.internal.discovery.XiaomiBridgeDiscoveryService; import org.openhab.binding.mihome.internal.handler.XiaomiBridgeHandler; import org.slf4j.Logger; @@ -38,71 +38,66 @@ * @author Dieter Schmidt - JavaDoc, refactored, reviewed * */ +@NonNullByDefault public abstract class XiaomiSocket { static final String MCAST_ADDR = "224.0.0.50"; - private static final int BUFFER_LENGTH = 1024; - private DatagramPacket datagramPacket = new DatagramPacket(new byte[BUFFER_LENGTH], BUFFER_LENGTH); - - private List listeners = new CopyOnWriteArrayList<>(); + private static final int BUFFER_LENGTH = 1024; private static final JsonParser PARSER = new JsonParser(); private final Logger logger = LoggerFactory.getLogger(XiaomiSocket.class); - private static Map openSockets = new ConcurrentHashMap<>(); + private final DatagramPacket datagramPacket = new DatagramPacket(new byte[BUFFER_LENGTH], BUFFER_LENGTH); + private final Set listeners = new CopyOnWriteArraySet<>(); - private int port; - private DatagramSocket socket; - private Thread socketReceiveThread; + private final int port; + private @Nullable DatagramSocket socket; + private final Thread socketReceiveThread = new Thread(this::receiveData); /** * Sets up an {@link XiaomiSocket} with the MiHome multicast address and a random port * + * @param owner identifies the socket owner */ - public XiaomiSocket() { - this(0); + public XiaomiSocket(String owner) { + this(0, owner); } /** * Sets up an {@link XiaomiSocket} with the MiHome multicast address and a specific port * - * @param port - the socket will be bound to this port + * @param port the socket will be bound to this port + * @param owner identifies the socket owner */ - public XiaomiSocket(int port) { + public XiaomiSocket(int port, String owner) { this.port = port; + socketReceiveThread.setName("XiaomiSocketReceiveThread(" + port + ", " + owner + ")"); } - public void intialize() { + public void initialize() { setupSocket(); - runReceiveThread(); - } - - protected void runReceiveThread() { - socketReceiveThread = new ReceiverThread(); - socketReceiveThread.start(); - if (getSocket() != null) { - getOpenSockets().put(getSocket().getLocalPort(), getSocket()); - logger.debug("There are {} open sockets: {}", getOpenSockets().size(), getOpenSockets()); + if (!socketReceiveThread.isAlive()) { + logger.trace("Starting receiver thread {}", socketReceiveThread); + socketReceiveThread.start(); } } - abstract DatagramSocket setupSocket(); + protected abstract void setupSocket(); /** * Interrupts the {@link ReceiverThread} and closes the {@link XiaomiSocket}. */ private void closeSocket() { synchronized (XiaomiSocket.class) { - if (socketReceiveThread != null) { - logger.debug("Interrupting Thread {}", socketReceiveThread); - socketReceiveThread.interrupt(); - } - if (getSocket() != null) { - logger.debug("Closing socket {}", getSocket()); - openSockets.remove(getSocket().getLocalPort()); - getSocket().close(); - setSocket(null); + logger.debug("Interrupting receiver thread {}", socketReceiveThread); + socketReceiveThread.interrupt(); + + DatagramSocket socket = this.socket; + if (socket != null) { + logger.debug("Closing socket {}", socket); + socket.close(); + this.socket = null; } } } @@ -111,15 +106,16 @@ private void closeSocket() { * Registers a {@link XiaomiSocketListener} to be called back, when data is received. * If no {@link XiaomiSocket} exists, when the method is called, it is being set up. * - * @param listener - {@link XiaomiSocketListener} to be called back + * @param listener {@link XiaomiSocketListener} to be called back */ public synchronized void registerListener(XiaomiSocketListener listener) { - if (!getListeners().contains(listener)) { - logger.trace("Adding socket listener {}", listener); - getListeners().add(listener); + if (listeners.add(listener)) { + logger.trace("Added socket listener {}", listener); } - if (getSocket() == null) { - intialize(); + + DatagramSocket socket = this.socket; + if (socket == null) { + initialize(); } } @@ -127,12 +123,14 @@ public synchronized void registerListener(XiaomiSocketListener listener) { * Unregisters a {@link XiaomiSocketListener}. If there are no listeners left, * the {@link XiaomiSocket} is being closed. * - * @param listener - {@link XiaomiSocketListener} to be unregistered + * @param listener {@link XiaomiSocketListener} to be unregistered */ public synchronized void unregisterListener(XiaomiSocketListener listener) { - getListeners().remove(listener); + if (listeners.remove(listener)) { + logger.trace("Removed socket listener {}", listener); + } - if (getListeners().isEmpty()) { + if (listeners.isEmpty()) { closeSocket(); } } @@ -140,36 +138,35 @@ public synchronized void unregisterListener(XiaomiSocketListener listener) { /** * Sends a message through the {@link XiaomiSocket} to a specific address and port * - * @param message - Message to be sent - * @param address - Address, to which the message shall be sent - * @param port - Port, through which the message shall be sent + * @param message the message to be sent + * @param address the message destination address + * @param port the message destination port */ public void sendMessage(String message, InetAddress address, int port) { + DatagramSocket socket = this.socket; + if (socket == null) { + logger.error("Error while sending message (socket is null)"); + return; + } + try { byte[] sendData = message.getBytes(StandardCharsets.UTF_8); DatagramPacket sendPacket = new DatagramPacket(sendData, sendData.length, address, port); logger.trace("Sending message: {} to {}:{}", message, address, port); - getSocket().send(sendPacket); + socket.send(sendPacket); } catch (IOException e) { - logger.error("Sending error", e); + logger.error("Error while sending message", e); } } /** - * @return - the port number of this {@link XiaomiSocket} + * @return the port number of this {@link XiaomiSocket} */ public int getPort() { return port; } - /** - * @return - a list of already open sockets - */ - public static Map getOpenSockets() { - return openSockets; - } - - protected DatagramSocket getSocket() { + protected @Nullable DatagramSocket getSocket() { return socket; } @@ -177,71 +174,58 @@ protected void setSocket(DatagramSocket socket) { this.socket = socket; } - protected List getListeners() { - return listeners; - } - /** - * The thread, which waits for data on the {@link XiaomiSocket} and handles it, when received - * - * @author Patrick Boos - Initial contribution - * @author Dieter Schmidt - comments and synchronized block for callback instead of copy - * + * This method is the main method of the receiver thread for the {@link XiaomiBridgeSocket}. + * If the socket has data, it parses the data to a json object and calls all + * {@link XiaomiSocketListener} and passes the data to them. */ - private class ReceiverThread extends Thread { - @Override - public void run() { - logger.trace("Staring reveicer thread for socket on port {}", getSocket().getLocalPort()); - receiveData(getSocket(), datagramPacket); + private void receiveData() { + DatagramSocket socket = this.socket; + if (socket == null) { + logger.error("Failed to receive data (socket is null)"); + return; } - /** - * This method is the main method of the {@link ReceiverThread} for the {@link XiaomiBridgeSocket}. - * If the socket has data, it parses the data to a json object and calls all - * {@link XiaomiSocketListener} and passes the data to them. - * - * @param socket - The multicast socket to listen to - * @param dgram - The datagram to receive - */ - private void receiveData(DatagramSocket socket, DatagramPacket dgram) { - try { - while (true) { - logger.trace("Thread {} waiting for data on port {}", this, socket.getLocalPort()); - socket.receive(dgram); - InetAddress address = dgram.getAddress(); - logger.debug("Received Datagram from {}:{} on Port {}", address.getHostAddress(), dgram.getPort(), - socket.getLocalPort()); - String sentence = new String(dgram.getData(), 0, dgram.getLength()); - JsonObject message = PARSER.parse(sentence).getAsJsonObject(); - notifyAll(getListeners(), message, address); - logger.trace("Data received and notified {} listeners", getListeners().size()); - } - } catch (IOException e) { - if (!isInterrupted()) { - logger.error("Error while receiving", e); - } else { - logger.trace("Receiver thread was interrupted"); - } + Thread currentThread = Thread.currentThread(); + int localPort = socket.getLocalPort(); + + try { + while (!currentThread.isInterrupted()) { + logger.trace("Thread {} waiting for data on port {}", currentThread.getName(), localPort); + socket.receive(datagramPacket); + InetAddress address = datagramPacket.getAddress(); + logger.debug("Received Datagram from {}:{} on port {}", address.getHostAddress(), + datagramPacket.getPort(), localPort); + String sentence = new String(datagramPacket.getData(), 0, datagramPacket.getLength()); + JsonObject message = PARSER.parse(sentence).getAsJsonObject(); + notifyListeners(message, address); + logger.trace("Data received and notified {} listeners", listeners.size()); + } + } catch (IOException e) { + if (!currentThread.isInterrupted()) { + logger.error("Error while receiving", e); + } else { + logger.trace("Receiver thread was interrupted"); } - logger.debug("Receiver thread ended"); } + logger.debug("Receiver thread ended"); + } - /** - * Notifies all {@link XiaomiSocketListener} on the parent {@link XiaomiSocket}. First checks for any matching - * {@link XiaomiBridgeHandler}, before passing to any {@link XiaomiBridgeDiscoveryService}. - * - * @param listeners - a list of all {@link XiaomiSocketListener} to notify - * @param message - the data message as {@link JsonObject} - */ - synchronized void notifyAll(List listeners, JsonObject message, InetAddress address) { - for (XiaomiSocketListener listener : listeners) { - if (listener instanceof XiaomiBridgeHandler) { - if (((XiaomiBridgeHandler) listener).getHost().equals(address)) { - listener.onDataReceived(message); - } - } else if (listener instanceof XiaomiBridgeDiscoveryService) { + /** + * Notifies all {@link XiaomiSocketListener} on the parent {@link XiaomiSocket}. First checks for any matching + * {@link XiaomiBridgeHandler}, before passing to any {@link XiaomiBridgeDiscoveryService}. + * + * @param message the data message as {@link JsonObject} + * @param address the address from which the message was received + */ + private void notifyListeners(JsonObject message, InetAddress address) { + for (XiaomiSocketListener listener : listeners) { + if (listener instanceof XiaomiBridgeHandler) { + if (((XiaomiBridgeHandler) listener).getHost().equals(address)) { listener.onDataReceived(message); } + } else if (listener instanceof XiaomiBridgeDiscoveryService) { + listener.onDataReceived(message); } } } diff --git a/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiSocketListener.java b/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiSocketListener.java index 883af9605d6a0..47302e353b1c7 100644 --- a/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiSocketListener.java +++ b/bundles/org.openhab.binding.mihome/src/main/java/org/openhab/binding/mihome/internal/socket/XiaomiSocketListener.java @@ -12,6 +12,8 @@ */ package org.openhab.binding.mihome.internal.socket; +import org.eclipse.jdt.annotation.NonNullByDefault; + import com.google.gson.JsonObject; /** @@ -20,6 +22,7 @@ * * @author Patrick Boos - Initial contribution */ +@NonNullByDefault public interface XiaomiSocketListener { /** * Callback method for the {@link XiaomiSocketListener}