Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[modbus] Fix error handling with DNS resolution / Unknown host errors #3490

Merged
merged 1 commit into from
Apr 15, 2023
Merged

[modbus] Fix error handling with DNS resolution / Unknown host errors #3490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions ...bus/src/main/java/org/openhab/core/io/transport/modbus/internal/ModbusConnectionPool.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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>() {
Copy link
Contributor Author

@ssalonen ssalonen Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the thing IDE did not catch -- the anonymous ModbusSlaveEndpointVisitor methods were returning @Nullable ModbusSlaveConnection even though the generic type argument was <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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method raised NPE as well, but it was caught by the catch(Exception e) block below... leading to message logged.

@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();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one likely raised unhandled NullPointerException before -- with unknown implications. Might not be expected in apache commons pool2

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