opentracing-contrib / java-jdbc

OpenTracing Instrumentation for JDBC
Apache License 2.0
82 stars 56 forks source link

memory leak using c3p0 pooling library #83

Open randallt opened 4 years ago

randallt commented 4 years ago

I am using a custom fork of the Java Special Agent. We have a service that relies on the Quartz scheduler, which uses c3p0 (v0.9.1.1). With the tracing agent applied, the service leaks memory until it throws an OOM error.

A heap dump showed that TracingPreparedStatements were being leaked. I tracked this to the use of the WrapperProxy and the fact that, given a Proxy instance of TracingConnection or TracingStatement, a call to obj.equals(obj) will always return false. This is because the WrapperProxy is delegating all method calls to the wrapper or the original object, but these don't know how to handle the added Proxy/WrapperProxy layers for the equals method. This leads to c3p0 not being able to clean up its collections for the connections/statements.

I was able to fix this by adding a custom equals method to TracingStatement:

  @Override
  public boolean equals(Object obj) {
    if (this == obj) {
        return true;
    } else if (obj == null) {
        return false;
    } else if (WrapperProxy.isWrapper(obj, getClass())) {
        return obj.equals(this); // trick to get through the WrapperProxy/Proxy instances
    } else if (getClass() != obj.getClass()) {
        return false;
    } else {
        TracingStatement other = (TracingStatement) obj;
        if (statement == null) {
            if (other.statement != null) {
                return false;
            }
        } else if (statement != other.statement && !statement.equals(other.statement)) {
            return false;
        }
    }
    return true;
  }

and to TracingConnection:

  @Override
  public boolean equals(Object obj) {
    if (this == obj) {
        return true;
    } else if (obj == null) {
        return false;
    } else if (WrapperProxy.isWrapper(obj, getClass())) {
        return obj.equals(this); // trick to get through the WrapperProxy/Proxy instances
    } else if (getClass() != obj.getClass()) {
        return false;
    } else {
        TracingConnection other = (TracingConnection) obj;
        if (connection == null) {
            if (other.connection != null) {
                return false;
            }
        } else if (connection != other.connection && !connection.equals(other.connection)) {
            return false;
        }
    }
    return true;
  }

These may not be 100% hardened, but they got me past the memory leak with c3p0. I thought I should be a good citizen and open an issue regarding this.

malafeev commented 4 years ago

@randallt would you like to submit PR? please add comments to the code.

randallt commented 4 years ago

Can someone please submit the PR for me? The only non-standard thing about the equals method is the trick to call the equals method on 'obj', which gets through the Proxy and WrapperProxy layers so that one of the other comparisons can function.

quaff commented 4 years ago

@randallt could you test https://github.com/opentracing-contrib/java-jdbc/pull/97