search

pingidentity / ldapsdk

UnboundID LDAP SDK for Java
Other
331 stars 79 forks source link

Memory leak when no servers available #73

Open MichalCho opened 4 years ago

MichalCho commented 4 years ago

Hi,

I think I've found a potential issue with write timeout timer thread. In LDAPConnectionPool we create a connection like this: ` try

{
  c = serverSet.getConnection(healthCheck);
}
catch (final LDAPException le)
{
  Debug.debugException(le);
  poolStatistics.incrementNumFailedConnectionAttempts();
  throw le;
}
c.setConnectionPool(this);

`

That internally creates a timer task. I've debugged a litte and I think that if an exception is thrown (ex. error 91 - when I kill my all ldap servers) during c = serverSet.getConnection(healthCheck); the setClosed() method is never called, further LDAPConnectionInternals never close the timer task.

If the pool object is alive for a long time it will slowly create more of these objects. Do you agree?

Thanks for help

dirmgr commented 4 years ago

You are correct that there is an issue in which a timer task could be leaked, but it's not exactly what you suspected. It doesn't have anything to do with the use of a connection pool (in the code snippet that you've pointed out, the call to serverSet.getConnection(healthCheck) does properly ensure that the connection gets closed if the health check fails), and the issue can arise with non-pooled connections as well.

The real problem is in the LDAPConnectionInternals constructor, which creates a WriteTimeoutHandler before attempting to actually establish the connection. A WriteTimeoutHandler is a TimerTask, and creating a WriteTimeoutHandler instance causes that task to be scheduled in a Timer. The issue is that if there was a problem that prevented the connection from being established, then the WriteTimeoutHandler wasn't being properly destroyed, but was instead allowed to stay registered with the timer.

I've just committed a fix for this issue (https://github.com/pingidentity/ldapsdk/commit/1c6638ce78c227ee5533c833cebf3091a246266c). Note that I had already also fixed a related issue (https://github.com/pingidentity/ldapsdk/commit/7880f6811ebbbc7c4bc1d1ccd64de2e0c3e2e512) in which the timer itself was never cleaned up. The next release will include both of these fixes (among other things), but you can get them now if you check out and build the LDAP SDK for yourself.

MichalCho commented 4 years ago

Hi! You are right, I've overlooked this. I've build the sdk and verified that it works. Thank you again for a quick fix.

dirmgr commented 4 years ago

FYI, LDAP SDK version 4.0.12 has been released and includes the fix for this problem. Thanks again for reporting it, and let us know if you run into any more issues.