From 850410764a51e0fd6795d802c0a91415d6c5579b Mon Sep 17 00:00:00 2001 From: Sami Salonen Date: Sat, 15 Apr 2023 17:29:36 +0300 Subject: [PATCH] [modbus] Fix possible NPE when connection construction fails (#3490) Signed-off-by: Sami Salonen GitOrigin-RevId: fdea66ae7573db0f028965138c8cdb8a433e1a38 --- .../modbus/internal/ModbusConnectionPool.java | 7 +-- .../modbus/internal/ModbusManagerImpl.java | 14 +++--- .../modbus/internal/ModbusPoolConfig.java | 2 +- .../ModbusSlaveConnectionFactoryImpl.java | 44 ++++++++++++------- 4 files changed, 40 insertions(+), 27 deletions(-) diff --git a/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusConnectionPool.java b/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusConnectionPool.java index d084a105145..8f4e8373210 100644 --- a/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusConnectionPool.java +++ b/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusConnectionPool.java @@ -30,14 +30,15 @@ * */ @NonNullByDefault -public class ModbusConnectionPool extends GenericKeyedObjectPool { +public class ModbusConnectionPool extends GenericKeyedObjectPool { - public ModbusConnectionPool(KeyedPooledObjectFactory factory) { + public ModbusConnectionPool( + KeyedPooledObjectFactory factory) { super(factory, new ModbusPoolConfig()); } @Override - public void setConfig(@Nullable GenericKeyedObjectPoolConfig conf) { + public void setConfig(@Nullable GenericKeyedObjectPoolConfig<@Nullable ModbusSlaveConnection> conf) { if (conf == null) { return; } else if (!(conf instanceof ModbusPoolConfig)) { diff --git a/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusManagerImpl.java b/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusManagerImpl.java index 6af5e320582..b0ef72b900f 100644 --- a/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusManagerImpl.java +++ b/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusManagerImpl.java @@ -347,7 +347,7 @@ public void accept(AggregateStopWatch timer, WriteTask task, ModbusSlaveConnecti * - https://community.openhab.org/t/connection-pooling-in-modbus-binding/5246/ */ - private volatile @Nullable KeyedObjectPool connectionPool; + private volatile @Nullable KeyedObjectPool connectionPool; private volatile @Nullable ModbusSlaveConnectionFactoryImpl connectionFactory; private volatile Map> scheduledPollTasks = new ConcurrentHashMap<>(); /** @@ -360,7 +360,7 @@ public void accept(AggregateStopWatch timer, WriteTask task, ModbusSlaveConnecti private void constructConnectionPool() { ModbusSlaveConnectionFactoryImpl connectionFactory = new ModbusSlaveConnectionFactoryImpl( DEFAULT_POOL_CONFIGURATION); - GenericKeyedObjectPool genericKeyedObjectPool = new ModbusConnectionPool( + GenericKeyedObjectPool genericKeyedObjectPool = new ModbusConnectionPool( connectionFactory); genericKeyedObjectPool.setSwallowedExceptionListener(new SwallowedExceptionListener() { @@ -379,7 +379,7 @@ public void onSwallowException(@Nullable Exception e) { private Optional borrowConnection(ModbusSlaveEndpoint endpoint) { Optional connection = Optional.empty(); - KeyedObjectPool pool = connectionPool; + KeyedObjectPool pool = connectionPool; if (pool == null) { return connection; } @@ -405,7 +405,7 @@ private Optional borrowConnection(ModbusSlaveEndpoint end } private void invalidate(ModbusSlaveEndpoint endpoint, Optional connection) { - KeyedObjectPool pool = connectionPool; + KeyedObjectPool pool = connectionPool; if (pool == null) { return; } @@ -423,7 +423,7 @@ private void invalidate(ModbusSlaveEndpoint endpoint, Optional connection) { - KeyedObjectPool pool = connectionPool; + KeyedObjectPool pool = connectionPool; if (pool == null) { return; } @@ -454,7 +454,7 @@ private void returnConnection(ModbusSlaveEndpoint endpoint, Optional, T extends TaskWithEndpoint> Optional getConnection( AggregateStopWatch timer, boolean oneOffTask, @NonNull T task) throws PollTaskUnregistered { - KeyedObjectPool connectionPool = this.connectionPool; + KeyedObjectPool connectionPool = this.connectionPool; if (connectionPool == null) { return Optional.empty(); } @@ -983,7 +983,7 @@ protected void activate(Map configProperties) { @Deactivate protected void deactivate() { synchronized (this) { - KeyedObjectPool connectionPool = this.connectionPool; + KeyedObjectPool connectionPool = this.connectionPool; if (connectionPool != null) { for (ModbusCommunicationInterface commInterface : this.communicationInterfaces) { try { diff --git a/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusPoolConfig.java b/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusPoolConfig.java index 6e8c61c7616..8707e30191d 100644 --- a/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusPoolConfig.java +++ b/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusPoolConfig.java @@ -32,7 +32,7 @@ * */ @NonNullByDefault -public class ModbusPoolConfig extends GenericKeyedObjectPoolConfig { +public class ModbusPoolConfig extends GenericKeyedObjectPoolConfig<@Nullable ModbusSlaveConnection> { @SuppressWarnings("unused") private EvictionPolicy evictionPolicy = new DefaultEvictionPolicy<>(); diff --git a/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/pooling/ModbusSlaveConnectionFactoryImpl.java b/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/pooling/ModbusSlaveConnectionFactoryImpl.java index 1c56f17ae57..2ffca70177d 100644 --- a/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/pooling/ModbusSlaveConnectionFactoryImpl.java +++ b/bundles/org.opensmarthouse.core.io.transport.modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/pooling/ModbusSlaveConnectionFactoryImpl.java @@ -59,14 +59,14 @@ */ @NonNullByDefault public class ModbusSlaveConnectionFactoryImpl - extends BaseKeyedPooledObjectFactory { + extends BaseKeyedPooledObjectFactory { - class PooledConnection extends DefaultPooledObject { + class PooledConnection extends DefaultPooledObject<@Nullable ModbusSlaveConnection> { private volatile long lastConnected; private volatile @Nullable ModbusSlaveEndpoint endpoint; - public PooledConnection(ModbusSlaveConnection object) { + public PooledConnection(@Nullable ModbusSlaveConnection object) { super(object); } @@ -97,6 +97,9 @@ public boolean maybeResetConnection(String activityName) { long localLastConnected = lastConnected; ModbusSlaveConnection connection = getObject(); + if (connection == null) { + return false; + } EndpointPoolConfiguration configuration = getEndpointPoolConfiguration(localEndpoint); long reconnectAfterMillis = configuration.getReconnectAfterMillis(); @@ -147,8 +150,8 @@ public ModbusSlaveConnectionFactoryImpl( } @Override - public ModbusSlaveConnection create(ModbusSlaveEndpoint endpoint) throws Exception { - return endpoint.accept(new ModbusSlaveEndpointVisitor() { + public @Nullable ModbusSlaveConnection create(ModbusSlaveEndpoint endpoint) throws Exception { + return endpoint.accept(new ModbusSlaveEndpointVisitor<@Nullable ModbusSlaveConnection>() { @Override public @Nullable ModbusSlaveConnection visit(ModbusSerialSlaveEndpoint modbusSerialSlavePoolingKey) { SerialConnection connection = new SerialConnection(modbusSerialSlavePoolingKey.getSerialParameters()); @@ -183,27 +186,34 @@ public ModbusSlaveConnection create(ModbusSlaveEndpoint endpoint) throws Excepti } @Override - public PooledObject wrap(ModbusSlaveConnection connection) { + public PooledObject<@Nullable ModbusSlaveConnection> wrap(@Nullable ModbusSlaveConnection connection) { return new PooledConnection(connection); } @Override - public void destroyObject(ModbusSlaveEndpoint endpoint, @Nullable PooledObject obj) { + public void destroyObject(ModbusSlaveEndpoint endpoint, + @Nullable PooledObject<@Nullable ModbusSlaveConnection> obj) { if (obj == null) { return; } - logger.trace("destroyObject for connection {} and endpoint {} -> closing the connection", obj.getObject(), - endpoint); - obj.getObject().resetConnection(); + ModbusSlaveConnection connection = obj.getObject(); + if (connection == null) { + return; + } + logger.trace("destroyObject for connection {} and endpoint {} -> closing the connection", connection, endpoint); + connection.resetConnection(); } @Override - public void activateObject(ModbusSlaveEndpoint endpoint, @Nullable PooledObject obj) - throws Exception { + public void activateObject(ModbusSlaveEndpoint endpoint, + @Nullable PooledObject<@Nullable ModbusSlaveConnection> obj) throws Exception { if (obj == null) { return; } ModbusSlaveConnection connection = obj.getObject(); + if (connection == null) { + return; + } try { EndpointPoolConfiguration config = getEndpointPoolConfiguration(endpoint); if (!connection.isConnected()) { @@ -226,7 +236,8 @@ public void activateObject(ModbusSlaveEndpoint endpoint, @Nullable PooledObject< } @Override - public void passivateObject(ModbusSlaveEndpoint endpoint, @Nullable PooledObject obj) { + public void passivateObject(ModbusSlaveEndpoint endpoint, + @Nullable PooledObject<@Nullable ModbusSlaveConnection> obj) { if (obj == null) { return; } @@ -238,8 +249,9 @@ public void passivateObject(ModbusSlaveEndpoint endpoint, @Nullable PooledObject } @Override - public boolean validateObject(ModbusSlaveEndpoint key, @Nullable PooledObject p) { - boolean valid = p != null && p.getObject().isConnected(); + public boolean validateObject(ModbusSlaveEndpoint key, @Nullable PooledObject<@Nullable ModbusSlaveConnection> p) { + @SuppressWarnings("null") // p.getObject() cannot be null due to short-circuiting boolean condition + boolean valid = p != null && p.getObject() != null && p.getObject().isConnected(); ModbusSlaveConnection slaveConnection = p != null ? p.getObject() : null; logger.trace("Validating endpoint {} connection {} -> {}", key, slaveConnection, valid); return valid; @@ -272,7 +284,7 @@ public EndpointPoolConfiguration getEndpointPoolConfiguration(ModbusSlaveEndpoin .orElseGet(() -> defaultPoolConfigurationFactory.apply(endpoint)); } - private void tryConnect(ModbusSlaveEndpoint endpoint, PooledObject obj, + private void tryConnect(ModbusSlaveEndpoint endpoint, PooledObject<@Nullable ModbusSlaveConnection> obj, ModbusSlaveConnection connection, EndpointPoolConfiguration config) throws Exception { if (connection.isConnected()) { return;