Skip to content

Commit

Permalink
[modbus] Fix possible NPE when connection construction fails (#3490)
Browse files Browse the repository at this point in the history 
Signed-off-by: Sami Salonen <ssalonen@gmail.com>
  • Loading branch information
@ssalonen
ssalonen authored Apr 15, 2023
1 parent 7a9b76d commit fdea66a
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 27 deletions.
7 changes: 4 additions & 3 deletions ...bus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusConnectionPool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@
*
*/
@NonNullByDefault
public class ModbusConnectionPool extends GenericKeyedObjectPool<ModbusSlaveEndpoint, ModbusSlaveConnection> {
public class ModbusConnectionPool extends GenericKeyedObjectPool<ModbusSlaveEndpoint, @Nullable ModbusSlaveConnection> {

public ModbusConnectionPool(KeyedPooledObjectFactory<ModbusSlaveEndpoint, ModbusSlaveConnection> factory) {
public ModbusConnectionPool(
KeyedPooledObjectFactory<ModbusSlaveEndpoint, @Nullable ModbusSlaveConnection> factory) {
super(factory, new ModbusPoolConfig());
}

@Override
public void setConfig(@Nullable GenericKeyedObjectPoolConfig<ModbusSlaveConnection> conf) {
public void setConfig(@Nullable GenericKeyedObjectPoolConfig<@Nullable ModbusSlaveConnection> conf) {
if (conf == null) {
return;
} else if (!(conf instanceof ModbusPoolConfig)) {
Expand Down
14 changes: 7 additions & 7 deletions ...modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusManagerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ModbusSlaveEndpoint, ModbusSlaveConnection> connectionPool;
private volatile @Nullable KeyedObjectPool<ModbusSlaveEndpoint, @Nullable ModbusSlaveConnection> connectionPool;
private volatile @Nullable ModbusSlaveConnectionFactoryImpl connectionFactory;
private volatile Map<PollTask, ScheduledFuture<?>> scheduledPollTasks = new ConcurrentHashMap<>();
/**
Expand All @@ -360,7 +360,7 @@ public void accept(AggregateStopWatch timer, WriteTask task, ModbusSlaveConnecti
private void constructConnectionPool() {
ModbusSlaveConnectionFactoryImpl connectionFactory = new ModbusSlaveConnectionFactoryImpl(
DEFAULT_POOL_CONFIGURATION);
GenericKeyedObjectPool<ModbusSlaveEndpoint, ModbusSlaveConnection> genericKeyedObjectPool = new ModbusConnectionPool(
GenericKeyedObjectPool<ModbusSlaveEndpoint, @Nullable ModbusSlaveConnection> genericKeyedObjectPool = new ModbusConnectionPool(
connectionFactory);
genericKeyedObjectPool.setSwallowedExceptionListener(new SwallowedExceptionListener() {

Expand All @@ -379,7 +379,7 @@ public void onSwallowException(@Nullable Exception e) {

private Optional<ModbusSlaveConnection> borrowConnection(ModbusSlaveEndpoint endpoint) {
Optional<ModbusSlaveConnection> connection = Optional.empty();
KeyedObjectPool<ModbusSlaveEndpoint, ModbusSlaveConnection> pool = connectionPool;
KeyedObjectPool<ModbusSlaveEndpoint, @Nullable ModbusSlaveConnection> pool = connectionPool;
if (pool == null) {
return connection;
}
Expand All @@ -405,7 +405,7 @@ private Optional<ModbusSlaveConnection> borrowConnection(ModbusSlaveEndpoint end
}

private void invalidate(ModbusSlaveEndpoint endpoint, Optional<ModbusSlaveConnection> connection) {
KeyedObjectPool<ModbusSlaveEndpoint, ModbusSlaveConnection> pool = connectionPool;
KeyedObjectPool<ModbusSlaveEndpoint, @Nullable ModbusSlaveConnection> pool = connectionPool;
if (pool == null) {
return;
}
Expand All @@ -423,7 +423,7 @@ private void invalidate(ModbusSlaveEndpoint endpoint, Optional<ModbusSlaveConnec
}

private void returnConnection(ModbusSlaveEndpoint endpoint, Optional<ModbusSlaveConnection> connection) {
KeyedObjectPool<ModbusSlaveEndpoint, ModbusSlaveConnection> pool = connectionPool;
KeyedObjectPool<ModbusSlaveEndpoint, @Nullable ModbusSlaveConnection> pool = connectionPool;
if (pool == null) {
return;
}
Expand Down Expand Up @@ -454,7 +454,7 @@ private void returnConnection(ModbusSlaveEndpoint endpoint, Optional<ModbusSlave
*/
private <R, C extends ModbusResultCallback, F extends ModbusFailureCallback<R>, T extends TaskWithEndpoint<R, C, F>> Optional<ModbusSlaveConnection> getConnection(
AggregateStopWatch timer, boolean oneOffTask, @NonNull T task) throws PollTaskUnregistered {
KeyedObjectPool<ModbusSlaveEndpoint, ModbusSlaveConnection> connectionPool = this.connectionPool;
KeyedObjectPool<ModbusSlaveEndpoint, @Nullable ModbusSlaveConnection> connectionPool = this.connectionPool;
if (connectionPool == null) {
return Optional.empty();
}
Expand Down Expand Up @@ -983,7 +983,7 @@ protected void activate(Map<String, Object> configProperties) {
@Deactivate
protected void deactivate() {
synchronized (this) {
KeyedObjectPool<ModbusSlaveEndpoint, ModbusSlaveConnection> connectionPool = this.connectionPool;
KeyedObjectPool<ModbusSlaveEndpoint, @Nullable ModbusSlaveConnection> connectionPool = this.connectionPool;
if (connectionPool != null) {
for (ModbusCommunicationInterface commInterface : this.communicationInterfaces) {
try {
Expand Down
2 changes: 1 addition & 1 deletion ....modbus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusPoolConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
*
*/
@NonNullByDefault
public class ModbusPoolConfig extends GenericKeyedObjectPoolConfig<ModbusSlaveConnection> {
public class ModbusPoolConfig extends GenericKeyedObjectPoolConfig<@Nullable ModbusSlaveConnection> {

@SuppressWarnings("unused")
private EvictionPolicy<ModbusSlaveConnection> evictionPolicy = new DefaultEvictionPolicy<>();
Expand Down
44 changes: 28 additions & 16 deletions ...g/openhab/core/io/transport/modbus/internal/pooling/ModbusSlaveConnectionFactoryImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,14 @@
*/
@NonNullByDefault
public class ModbusSlaveConnectionFactoryImpl
extends BaseKeyedPooledObjectFactory<ModbusSlaveEndpoint, ModbusSlaveConnection> {
extends BaseKeyedPooledObjectFactory<ModbusSlaveEndpoint, @Nullable ModbusSlaveConnection> {

class PooledConnection extends DefaultPooledObject<ModbusSlaveConnection> {
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);
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -147,8 +150,8 @@ public ModbusSlaveConnectionFactoryImpl(
}

@Override
public ModbusSlaveConnection create(ModbusSlaveEndpoint endpoint) throws Exception {
return endpoint.accept(new ModbusSlaveEndpointVisitor<ModbusSlaveConnection>() {
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());
Expand Down Expand Up @@ -183,27 +186,34 @@ public ModbusSlaveConnection create(ModbusSlaveEndpoint endpoint) throws Excepti
}

@Override
public PooledObject<ModbusSlaveConnection> wrap(ModbusSlaveConnection connection) {
public PooledObject<@Nullable ModbusSlaveConnection> wrap(@Nullable ModbusSlaveConnection connection) {
return new PooledConnection(connection);
}

@Override
public void destroyObject(ModbusSlaveEndpoint endpoint, @Nullable PooledObject<ModbusSlaveConnection> 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<ModbusSlaveConnection> 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()) {
Expand All @@ -226,7 +236,8 @@ public void activateObject(ModbusSlaveEndpoint endpoint, @Nullable PooledObject<
}

@Override
public void passivateObject(ModbusSlaveEndpoint endpoint, @Nullable PooledObject<ModbusSlaveConnection> obj) {
public void passivateObject(ModbusSlaveEndpoint endpoint,
@Nullable PooledObject<@Nullable ModbusSlaveConnection> obj) {
if (obj == null) {
return;
}
Expand All @@ -238,8 +249,9 @@ public void passivateObject(ModbusSlaveEndpoint endpoint, @Nullable PooledObject
}

@Override
public boolean validateObject(ModbusSlaveEndpoint key, @Nullable PooledObject<ModbusSlaveConnection> 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;
Expand Down Expand Up @@ -272,7 +284,7 @@ public EndpointPoolConfiguration getEndpointPoolConfiguration(ModbusSlaveEndpoin
.orElseGet(() -> defaultPoolConfigurationFactory.apply(endpoint));
}

private void tryConnect(ModbusSlaveEndpoint endpoint, PooledObject<ModbusSlaveConnection> obj,
private void tryConnect(ModbusSlaveEndpoint endpoint, PooledObject<@Nullable ModbusSlaveConnection> obj,
ModbusSlaveConnection connection, EndpointPoolConfiguration config) throws Exception {
if (connection.isConnected()) {
return;
Expand Down

0 comments on commit fdea66a

Please sign in to comment.