Skip to content

[Improve] add connection check when get connection #10

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.

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,11 @@

package org.apache.hertzbeat.collector.collect.common.cache;

import lombok.extern.slf4j.Slf4j;

/**
* AbstractConnection
*/
@Slf4j
public abstract class AbstractConnection<T> implements AutoCloseable {

public abstract class AbstractConnection<T> implements AutoCloseable {

/**
* @return Returns the connection.
Expand All @@ -35,8 +33,15 @@ public abstract class AbstractConnection<T> implements AutoCloseable {
*/
public abstract void closeConnection() throws Exception;

/**
* Check connection when get connection.
*/
public abstract void check() throws Exception;

@Override
public void close() throws Exception{
closeConnection();

this.closeConnection();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.hertzbeat.collector.collect.common.cache;

import java.sql.Connection;
import java.sql.SQLException;
import lombok.extern.slf4j.Slf4j;

/**
Expand All @@ -39,8 +40,24 @@ public void closeConnection() throws Exception {
}
}

@Override
public void check() throws SQLException {

if (connection.isClosed()) {
throw new SQLException("Connection is closed");
}
}

@Override
public Connection getConnection() {

try {
this.check();
}
catch (SQLException e) {
log.error(e.getMessage());
return null;
}
Comment on lines 40 to +60
Copy link

Choose a reason for hiding this comment

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

Returning null instead of throwing exceptions on connection validation failures

The getConnection() method silently returns null when connection validation fails instead of propagating the exception. This violates the fail-fast principle and will lead to NullPointerExceptions when client code attempts to use the connection. Since the method signature doesn't indicate that null can be returned, callers are unlikely to check for null values, resulting in runtime failures that are difficult to diagnose and potentially causing system instability.

Suggested change
}
}
@Override
public void check() throws SQLException {
if (connection.isClosed()) {
throw new SQLException("Connection is closed");
}
}
@Override
public Connection getConnection() {
try {
this.check();
}
catch (SQLException e) {
log.error(e.getMessage());
return null;
}
@Override
public Connection getConnection() {
try {
this.check();
return connection;
}
catch (SQLException e) {
log.error("Connection validation failed", e);
throw new RuntimeException("Failed to get valid JDBC connection", e);
}
}

return connection;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,24 @@ public void closeConnection() throws Exception {
}
}

@Override
public void check() throws Exception {

if (connection.getConnectionId().isEmpty()) {
throw new RuntimeException("connection is closed");
Comment on lines +43 to +47
Copy link

Choose a reason for hiding this comment

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

Potential NullPointerException in JmxConnect.check()

The JmxConnect.check() method doesn't handle the case where the connection itself might be null. If the connection object is null (which could happen if connection initialization failed), calling connection.getConnectionId() will throw a NullPointerException. This would bypass the intended error handling in the getConnection() method.

Suggested change
@Override
public void check() throws Exception {
if (connection.getConnectionId().isEmpty()) {
throw new RuntimeException("connection is closed");
if (connection == null || connection.getConnectionId().isEmpty()) {
throw new RuntimeException("connection is null or closed");

}
}

@Override
public JMXConnector getConnection() {

try {
this.check();
}
catch (Exception e) {
log.error(e.getMessage());
return null;
}
return connection;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import com.mongodb.client.MongoClient;
import lombok.extern.slf4j.Slf4j;
import org.bson.Document;

/**
* mongodb connect client
Expand All @@ -38,8 +39,23 @@ public void closeConnection() throws Exception {
}
}

@Override
public void check() throws Exception {

mongoClient.getDatabase("admin").runCommand(new Document("ping", 1));
}

@Override
public MongoClient getConnection() {

try {
this.check();
}
catch (Exception e) {
log.error(e.getMessage());
return null;
}

return mongoClient;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,24 @@ public void closeConnection() throws Exception {
}
}

@Override
public void check() throws Exception {

if (!reddishConnectSession.isOpen()) {
throw new RuntimeException("Connection is closed");
}
}

@Override
public ConnectSession getConnection() {

try {
this.check();
}
catch (Exception e) {
log.error(e.getMessage());
return null;
}
return reddishConnectSession;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,24 @@ public void closeConnection() throws Exception {
}
}

@Override
public void check() throws Exception {

if (!connection.isOpen()) {
throw new RuntimeException("Connection is closed");
}
}

@Override
public StatefulConnection<String, String> getConnection() {

try {
this.check();
}
catch (Exception e) {
log.error(e.getMessage());
return null;
}
return connection;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,23 @@ public void closeConnection() throws Exception {
}
}

@Override
public void check() throws Exception {

if (!clientSession.isOpen()) {
throw new Exception("ssh connection is not open");
}
}

public ClientSession getConnection() {

try {
this.check();
}
catch (Exception e) {
log.error(e.getMessage());
return null;
}
return clientSession;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -236,22 +236,26 @@ private StatefulRedisClusterConnection<String, String> getClusterConnection(Redi
* @return connection
*/
private StatefulConnection<String, String> getStatefulConnection(CacheIdentifier identifier) {
StatefulConnection<String, String> connection = null;

Optional<RedisConnect> cacheOption = connectionCommonCache.getCache(identifier, true);

if (cacheOption.isPresent()) {
RedisConnect redisConnect = cacheOption.get();
connection = redisConnect.getConnection();
if (!connection.isOpen()) {

try {
return redisConnect.getConnection();
} catch (RuntimeException e) {
log.info("The Redis connection from cache is invalid, closing and removing: {}", e.getMessage());
try {
connection.closeAsync();
} catch (Exception e) {
log.info("The redis connect form cache, close error: {}", e.getMessage());
redisConnect.getConnection().closeAsync();
} catch (Exception closeException) {
log.info("Error closing Redis connection: {}", closeException.getMessage());
}
connection = null;
connectionCommonCache.removeCache(identifier);
}
}
return connection;

return null;
}

Comment on lines 236 to 260
Copy link

Choose a reason for hiding this comment

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

Inefficient Redis Connection Management in RedisCommonCollectImpl

The getStatefulConnection method has been updated to use the new check() functionality, but it makes a second call to getConnection() when closing an invalid connection. This creates a performance inefficiency as it could trigger another validation check and potentially another exception. Additionally, it logs connection issues at INFO level instead of ERROR or WARN, which could hide important connection problems in production environments.

Suggested change
* @return connection
*/
private StatefulConnection<String, String> getStatefulConnection(CacheIdentifier identifier) {
StatefulConnection<String, String> connection = null;
Optional<RedisConnect> cacheOption = connectionCommonCache.getCache(identifier, true);
if (cacheOption.isPresent()) {
RedisConnect redisConnect = cacheOption.get();
connection = redisConnect.getConnection();
if (!connection.isOpen()) {
try {
return redisConnect.getConnection();
} catch (RuntimeException e) {
log.info("The Redis connection from cache is invalid, closing and removing: {}", e.getMessage());
try {
connection.closeAsync();
} catch (Exception e) {
log.info("The redis connect form cache, close error: {}", e.getMessage());
redisConnect.getConnection().closeAsync();
} catch (Exception closeException) {
log.info("Error closing Redis connection: {}", closeException.getMessage());
}
connection = null;
connectionCommonCache.removeCache(identifier);
}
}
return connection;
return null;
}
private StatefulConnection<String, String> getStatefulConnection(CacheIdentifier identifier) {
Optional<RedisConnect> cacheOption = connectionCommonCache.getCache(identifier, true);
if (cacheOption.isPresent()) {
RedisConnect redisConnect = cacheOption.get();
StatefulConnection<String, String> connection = null;
try {
connection = redisConnect.getConnection();
return connection;
} catch (RuntimeException e) {
log.warn("The Redis connection from cache is invalid, removing: {}", e.getMessage());
try {
if (connection != null) {
connection.closeAsync();
}
} catch (Exception closeException) {
log.warn("Error closing Redis connection: {}", closeException.getMessage());
}
connectionCommonCache.removeCache(identifier);
}
}
return null;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ public Object getConnection() {
@Override
public void closeConnection() throws Exception {
}

@Override
public void check() throws Exception {
}
};
}

Expand Down
Loading