RedisGraph / JRedisGraph

Java API for RedisGraph
https://redisgraph.io
BSD 3-Clause "New" or "Revised" License
59 stars 20 forks source link

How to close the redisgraph connections #59

Open BhanuPrakash531 opened 4 years ago

BhanuPrakash531 commented 4 years ago

Hi Team,

I'm looking for a template how to close the redisgraph connection obtained from the JedisPool to handle the resource leakage. How can we achieve this on JRedisGraph dependency?

Below is an excerpt of the code that gets the RedisGraph API which is used by other classes to execute queries against the database.

@Configuration
public class Config {
   @Bean
   public RedisGraph redisGraph() {
      JedisPool pool = new JedisPool(new JedisPoolConfig(), host, port, timeout, password);
      return new RedisGraph(pool);
   }
}

We are then calling the created bean directly to send query to database, this is resulting on resource leakage

@Service
public class Util {
   @Autowired
   private RedisGraph redisGraph; 
 public ResultSet redisGraphApi(String query) {
      ResultSet resultSet = redisGraph.query(graphId, query);
      return resultSet;
   }
}
DvirDukhan commented 4 years ago

Thanks, @BhanuPrakash531 Can you please send the leak log? I think I have a possible solution but I want to verify.

BhanuPrakash531 commented 4 years ago

What kind of leak log are you referring here?

DvirDukhan commented 4 years ago

@BhanuPrakash531 where did you see the resource leakage?

BhanuPrakash531 commented 4 years ago

We get too many open connections errors on the logs and database is the only connection we have in our application

DvirDukhan commented 4 years ago

@BhanuPrakash531 can you provide a code sample so I can reproduce it?

BhanuPrakash531 commented 4 years ago

Configuration:

`@Configuration
public class Config {
   @Bean
   public RedisGraph redisGraph() {
      JedisPool pool = new JedisPool(new JedisPoolConfig(), host, port, timeout, password);
      return new RedisGraph(pool);
   }
}`

Service

`@Service
public class Util {
   @Autowired
   private RedisGraph redisGraph; 
 public ResultSet redisGraphApi(String query) {
      ResultSet resultSet = redisGraph.query(graphId, query);
      return resultSet;
   }
}`

Service caller

`@Service
public class Controller {
   @Autowired
   private Util  util ; 

   public Employee getEmployee() {
      ResultSet resultSet = util.redisGraphApi(query);
      if (resultSet.hasNext()) {
        return buildEmployee(resultSet.next());
      }
   }
}`

Health Check:

`private boolean isDBConnected() {
      boolean isConnected = false;
      try (JedisPool pool = new JedisPool(new JedisPoolConfig(), host, port, timeout, password)) {
         if (pool.getResource().ping().equalsIgnoreCase("PONG"))
            isConnected = true;
      }
      return isConnected;
}`
DvirDukhan commented 4 years ago

hi @BhanuPrakash531 you try-with-resource a new Jedis pool, and check if you can get a response from the server. The close method of the pool is closing the connections anyhow, once the `try-with-resource is done. can you please attach an example of a code that calls a redisgraph query and causes resource leakage?

BhanuPrakash531 commented 4 years ago

Added the Service caller above which calls the redisgraph.query. The method isConnected() is for health check of the database. this method is called every 100ms. Awful lot of times. After some time, cpu is reaching 100% and failing the system. Thread dumps says 195 threads in sun.misc.Unsafe.park(Native Method)

http-nio-8080-exec-200 priority:5 - threadId:0x00007fc49c0ef800 - nativeId:0xc83f - nativeId (decimal):51263 - state:WAITING stackTrace: java.lang.Thread.State: WAITING (parking) at sun.misc.Unsafe.park(Native Method) parking to wait for <0x00000000c026b6a0> (a java.util.concurrent.locks.ReentrantReadWriteLock$FairSync) at java.util.concurrent.locks.LockSupport.park(LockSupport.java:175) at java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:836) at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireQueued(AbstractQueuedSynchronizer.java:870) at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(AbstractQueuedSynchronizer.java:1199) at java.util.concurrent.locks.ReentrantReadWriteLock$WriteLock.lock(ReentrantReadWriteLock.java:943) at com.sun.jmx.mbeanserver.Repository.remove(Repository.java:623) at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.unregisterFromRepository(DefaultMBeanServerInterceptor.java:1940) at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.exclusiveUnregisterMBean(DefaultMBeanServerInterceptor.java:448) at com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.unregisterMBean(DefaultMBeanServerInterceptor.java:415) at com.sun.jmx.mbeanserver.JmxMBeanServer.unregisterMBean(JmxMBeanServer.java:546) at org.apache.commons.pool2.impl.BaseGenericObjectPool.jmxUnregister(BaseGenericObjectPool.java:852) at org.apache.commons.pool2.impl.GenericObjectPool.close(GenericObjectPool.java:699) locked <0x00000000f6928e30> (a java.lang.Object) at redis.clients.jedis.util.Pool.closeInternalPool(Pool.java:100) at redis.clients.jedis.util.Pool.destroy(Pool.java:87) at redis.clients.jedis.util.Pool.close(Pool.java:29) at com.bhanu.StatusCheck.isDBConnected(SvcLBStatusHandler.java:69)

gkorland commented 4 years ago

@BhanuPrakash531 it seems like the fact you're opening so many connection pools so fast is overloading the JMX unregister. The best practice will be to use the same thread pool and just recycle the connections and not opening new pool every 100ms.

private boolean isDBConnected() {
      boolean isConnected = false;
      try (Jedis jedis = this.pool.getResource()) {
         if (jedis.ping().equalsIgnoreCase("PONG"))
            isConnected = true;
      }
      return isConnected;
}
DvirDukhan commented 4 years ago

@BhanuPrakash531 I'm waiting for your input on @gkorland answer.

BhanuPrakash531 commented 4 years ago

Agreed @gkorland, for health checks to database servers, whats the recommended approach to handle this scenario from java services by using RedisGraph APIs?

gkorland commented 4 years ago

What is the reason for these checks? are you running a general Redis monitor?

BhanuPrakash531 commented 4 years ago

No. We have developed an application on top of redis graph, other services which uses our service does a frequent healt checks on our application, we need to check the health of redis database for each of the request we get. Hence we need that operation

DvirDukhan commented 4 years ago

@BhanuPrakash531 you need a health check for the RedisGraph API? Do you want me to expose such an API?

DvirDukhan commented 4 years ago

@BhanuPrakash531 In second thought, you can get a dedicated Jedis resource from the RedisGraphAPI by

boolean isConnected = false;
try(RedisGraphContext context = redisGraphApi.getContext()) {
    try (Jedis jedis =  context.getConnectionContext()) {
         if (jedis.ping().equalsIgnoreCase("PONG"))
            isConnected = true;
      }
}
return isConnected;

so you'll have full Jedis API as well as GraphAPI with this connection.

BhanuPrakash531 commented 4 years ago

Okay. Thanks for the above block of code, that helps. One last question, where does the connection opened for redisGraphApi closed here? Or that might not be closed? Or is that handled internally by JRedisGraph API?

DvirDukhan commented 4 years ago

you mean to

 JedisPool pool = new JedisPool(new JedisPoolConfig(), host, port, timeout, password);
 return new RedisGraph(pool);

? RedisGraphAPI is closable, once you close it, it will close the underlying pool.

BhanuPrakash531 commented 4 years ago

So that will be closed when the server is stopped? Or is there a best practice to close it?

DvirDukhan commented 4 years ago

@BhanuPrakash531 This is from Spring-RedisGraph repo, use the @Bean(destroyMethod = "close").

@Configuration
@ConfigurationProperties(prefix = "spring.redisgraph")
@Data
@ComponentScan
public class RedisGraphConfiguration {

    @Autowired
    @Getter(AccessLevel.NONE)
    @Setter(AccessLevel.NONE)
    private RedisProperties props;

    private String host;
    private Integer port;

    // Login to the database and set the password of redis graph with the below command
    // redis-cli$: CONFIG SET requirepass "Redis@password123"
    private String password;

    @Bean(destroyMethod = "close")
    public RedisGraph redisGraphConnection() {
        if (null == password) {
            return new com.redislabs.redisgraph.impl.api.RedisGraph();
        }
        else {
            JedisPool pool = new JedisPool(new JedisPoolConfig(), host, port, Protocol.DEFAULT_TIMEOUT, password);
            return new com.redislabs.redisgraph.impl.api.RedisGraph(pool);
        }
    }

    private String host() {
        if (host == null) {
            return props.getHost();
        }
        return host;
    }

    private int port() {
        if (port == null) {
            return props.getPort();
        }
        return port;
    }

    private String password() {
        if (password == null) {
            return props.getPassword();
        }
        return password;
    }
}
BhanuPrakash531 commented 4 years ago

Alright, thanks for the response. This helps. Appreciated.