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.

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

How to close the redisgraph connections #59

Open
BhanuPrakash531 opened this issue Feb 12, 2020 · 20 comments
Open

How to close the redisgraph connections #59

BhanuPrakash531 opened this issue Feb 12, 2020 · 20 comments

Comments

@BhanuPrakash531
Copy link

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
Copy link
Contributor

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

@BhanuPrakash531
Copy link
Author

What kind of leak log are you referring here?

@DvirDukhan
Copy link
Contributor

@BhanuPrakash531
where did you see the resource leakage?

@BhanuPrakash531
Copy link
Author

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

@DvirDukhan
Copy link
Contributor

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

@BhanuPrakash531
Copy link
Author

BhanuPrakash531 commented Feb 19, 2020

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

gkorland commented Feb 19, 2020

@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
Copy link
Contributor

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

@BhanuPrakash531
Copy link
Author

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

@gkorland
Copy link
Contributor

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

@BhanuPrakash531
Copy link
Author

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
Copy link
Contributor

DvirDukhan commented Feb 20, 2020

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

@DvirDukhan
Copy link
Contributor

@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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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

@DvirDukhan
Copy link
Contributor

DvirDukhan commented Feb 20, 2020

@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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants