Re: svn commit: r1431191 - /ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java

[Jacopo Cappellato-4]

Jacques,

please see inline:

On Jan 10, 2013, at 6:32 AM, [hidden email] wrote:

> Author: jleroux
> Date: Thu Jan 10 05:32:48 2013
> New Revision: 1431191
>
> URL: http://svn.apache.org/viewvc?rev=1431191&view=rev
> Log:
> In "Memory leak due to transaction management using DBCP and MySQL" https://issues.apache.org/jira/browse/OFBIZ-5122 Jose Manuel Vivó Arnal reported that I missed to apply the patch from OFBIZ-2599, just handled the DBPC part, and Philippe Mouawad did not notice.
>
> OK, so it was just a matter of replacing
> PoolableConnectionFactory factory = new PoolableConnectionFactory(xacf, pool, null, null, true, true);
> by
> PoolableManagedConnectionFactory factory = new PoolableManagedConnectionFactory(xacf, pool, null, null, true, true);

This is minor but it would be better to write:

PoolableConnectionFactory factory = new PoolableManagedConnectionFactory(xacf, pool, null, null, true, true);

>
> The rest are only imports cleaning.
>
> Too bad we did not notice that for 3.5 years. It seems it does not affect servers with plenty of memory, or updated often enough. But I recommend to upgrade of course...

Are we sure we can backport this fix to all the release branches as you did? From the history of comments in OFBIZ-2599 I see that there are some prerequisites for the version of DBCP: did you check if they are satisfied by the jars in 10.04 and 11.04?

Regards,

Jacopo

>
> Modified:
>    ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java
>
> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java?rev=1431191&r1=1431190&r2=1431191&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java (original)
> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java Thu Jan 10 05:32:48 2013
> @@ -18,11 +18,22 @@
>  *******************************************************************************/
> package org.ofbiz.entity.connection;
>
> +import java.sql.Connection;
> +import java.sql.Driver;
> +import java.sql.SQLException;
> +import java.util.HashMap;
> +import java.util.Map;
> +import java.util.Properties;
> +
> +import javax.transaction.TransactionManager;
> +
> +import javolution.util.FastMap;
> +
> import org.apache.commons.dbcp.ConnectionFactory;
> import org.apache.commons.dbcp.DriverConnectionFactory;
> -import org.apache.commons.dbcp.PoolableConnectionFactory;
> import org.apache.commons.dbcp.managed.LocalXAConnectionFactory;
> import org.apache.commons.dbcp.managed.ManagedDataSource;
> +import org.apache.commons.dbcp.managed.PoolableManagedConnectionFactory;
> import org.apache.commons.dbcp.managed.XAConnectionFactory;
> import org.apache.commons.pool.impl.GenericObjectPool;
> import org.ofbiz.base.util.Debug;
> @@ -32,16 +43,6 @@ import org.ofbiz.entity.datasource.Gener
> import org.ofbiz.entity.transaction.TransactionFactory;
> import org.w3c.dom.Element;
>
> -import javax.transaction.TransactionManager;
> -import java.sql.Connection;
> -import java.sql.Driver;
> -import java.sql.SQLException;
> -import java.util.HashMap;
> -import java.util.Map;
> -import java.util.Properties;
> -
> -import javolution.util.FastMap;
> -
> /**
>  * DBCPConnectionFactory
>  */
> @@ -146,7 +147,7 @@ public class DBCPConnectionFactory imple
>
>
>             // create the pool object factory
> -            PoolableConnectionFactory factory = new PoolableConnectionFactory(xacf, pool, null, null, true, true);
> +            PoolableManagedConnectionFactory factory = new PoolableManagedConnectionFactory(xacf, pool, null, null, true, true);
>             factory.setValidationQuery("select 1 from entity_key_store where key_name = ''");
>             factory.setDefaultReadOnly(false);
>
>
>

[Jacques Le Roux]

Jacopo Cappellato wrote:

> Jacques,
>
> please see inline:
>
> On Jan 10, 2013, at 6:32 AM, [hidden email] wrote:
>
>> Author: jleroux
>> Date: Thu Jan 10 05:32:48 2013
>> New Revision: 1431191
>>
>> URL: http://svn.apache.org/viewvc?rev=1431191&view=rev
>> Log:
>> In "Memory leak due to transaction management using DBCP and MySQL" https://issues.apache.org/jira/browse/OFBIZ-5122 Jose Manuel
>> Vivó Arnal reported that I missed to apply the patch from OFBIZ-2599, just handled the DBPC part, and Philippe Mouawad did not
>> notice.  
>>
>> OK, so it was just a matter of replacing
>> PoolableConnectionFactory factory = new PoolableConnectionFactory(xacf, pool, null, null, true, true);
>> by
>> PoolableManagedConnectionFactory factory = new PoolableManagedConnectionFactory(xacf, pool, null, null, true, true);
>
> This is minor but it would be better to write:
>
> PoolableConnectionFactory factory = new PoolableManagedConnectionFactory(xacf, pool, null, null, true, true);

OK, I can change that in trunk
 
>>
>> The rest are only imports cleaning.
>>
>> Too bad we did not notice that for 3.5 years. It seems it does not affect servers with plenty of memory, or updated often
>> enough. But I recommend to upgrade of course...
>
> Are we sure we can backport this fix to all the release branches as you did? From the history of comments in OFBIZ-2599 I see
> that there are some prerequisites for the version of DBCP: did you check if they are satisfied by the jars in 10.04 and 11.04?

Yes, commons-dbcp-1.4.jar is present since R10.04 (also else it would not have compiled ;o)
The change has been introduced since 1.3: https://issues.apache.org/jira/browse/DBCP-294

Jacques

> Regards,
>
> Jacopo
>
>>
>> Modified:
>>    ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java
>>
>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java?rev=1431191&r1=1431190&r2=1431191&view=diff
>> ============================================================================== ---
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java (original) +++
>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java Thu Jan 10 05:32:48 2013 @@ -18,11
>>  +18,22 @@ *******************************************************************************/
>> package org.ofbiz.entity.connection;
>>
>> +import java.sql.Connection;
>> +import java.sql.Driver;
>> +import java.sql.SQLException;
>> +import java.util.HashMap;
>> +import java.util.Map;
>> +import java.util.Properties;
>> +
>> +import javax.transaction.TransactionManager;
>> +
>> +import javolution.util.FastMap;
>> +
>> import org.apache.commons.dbcp.ConnectionFactory;
>> import org.apache.commons.dbcp.DriverConnectionFactory;
>> -import org.apache.commons.dbcp.PoolableConnectionFactory;
>> import org.apache.commons.dbcp.managed.LocalXAConnectionFactory;
>> import org.apache.commons.dbcp.managed.ManagedDataSource;
>> +import org.apache.commons.dbcp.managed.PoolableManagedConnectionFactory;
>> import org.apache.commons.dbcp.managed.XAConnectionFactory;
>> import org.apache.commons.pool.impl.GenericObjectPool;
>> import org.ofbiz.base.util.Debug;
>> @@ -32,16 +43,6 @@ import org.ofbiz.entity.datasource.Gener
>> import org.ofbiz.entity.transaction.TransactionFactory;
>> import org.w3c.dom.Element;
>>
>> -import javax.transaction.TransactionManager;
>> -import java.sql.Connection;
>> -import java.sql.Driver;
>> -import java.sql.SQLException;
>> -import java.util.HashMap;
>> -import java.util.Map;
>> -import java.util.Properties;
>> -
>> -import javolution.util.FastMap;
>> -
>> /**
>>  * DBCPConnectionFactory
>>  */
>> @@ -146,7 +147,7 @@ public class DBCPConnectionFactory imple
>>
>>
>>             // create the pool object factory
>> -            PoolableConnectionFactory factory = new PoolableConnectionFactory(xacf, pool, null, null, true, true);
>> +            PoolableManagedConnectionFactory factory = new PoolableManagedConnectionFactory(xacf, pool, null, null, true, true);
>>             factory.setValidationQuery("select 1 from entity_key_store where key_name = ''");
>>             factory.setDefaultReadOnly(false);

[Jacques Le Roux]

Jacques Le Roux wrote:

> Jacopo Cappellato wrote:
>> Jacques,
>>
>> please see inline:
>>
>> On Jan 10, 2013, at 6:32 AM, [hidden email] wrote:
>>
>>> Author: jleroux
>>> Date: Thu Jan 10 05:32:48 2013
>>> New Revision: 1431191
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1431191&view=rev
>>> Log:
>>> In "Memory leak due to transaction management using DBCP and MySQL" https://issues.apache.org/jira/browse/OFBIZ-5122 Jose Manuel
>>> Vivó Arnal reported that I missed to apply the patch from OFBIZ-2599, just handled the DBPC part, and Philippe Mouawad did not
>>> notice.
>>>
>>> OK, so it was just a matter of replacing
>>> PoolableConnectionFactory factory = new PoolableConnectionFactory(xacf, pool, null, null, true, true);
>>> by
>>> PoolableManagedConnectionFactory factory = new PoolableManagedConnectionFactory(xacf, pool, null, null, true, true);
>>
>> This is minor but it would be better to write:
>>
>> PoolableConnectionFactory factory = new PoolableManagedConnectionFactory(xacf, pool, null, null, true, true);
>
> OK, I can change that in trunk
>
>>>
>>> The rest are only imports cleaning.
>>>
>>> Too bad we did not notice that for 3.5 years. It seems it does not affect servers with plenty of memory, or updated often
>>> enough. But I recommend to upgrade of course...
>>
>> Are we sure we can backport this fix to all the release branches as you did? From the history of comments in OFBIZ-2599 I see
>> that there are some prerequisites for the version of DBCP: did you check if they are satisfied by the jars in 10.04 and 11.04?
>
> Yes, commons-dbcp-1.4.jar is present since R10.04 (also else it would not have compiled ;o)
> The change has been introduced since 1.3: https://issues.apache.org/jira/browse/DBCP-294
>
> Jacques

BTW even R09.04 would work because of http://svn.apache.org/viewvc?view=revision&revision=r836015
And it was my bad then to not commit the OFBiz patch (I only handled the DBCP side)

Jacques
 

>
>> Regards,
>>
>> Jacopo
>>
>>>
>>> Modified:
>>>    ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java
>>>
>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java?rev=1431191&r1=1431190&r2=1431191&view=diff
>>> ============================================================================== ---
>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java (original) +++
>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/connection/DBCPConnectionFactory.java Thu Jan 10 05:32:48 2013 @@ -18,11
>>>  +18,22 @@ *******************************************************************************/
>>> package org.ofbiz.entity.connection;
>>>
>>> +import java.sql.Connection;
>>> +import java.sql.Driver;
>>> +import java.sql.SQLException;
>>> +import java.util.HashMap;
>>> +import java.util.Map;
>>> +import java.util.Properties;
>>> +
>>> +import javax.transaction.TransactionManager;
>>> +
>>> +import javolution.util.FastMap;
>>> +
>>> import org.apache.commons.dbcp.ConnectionFactory;
>>> import org.apache.commons.dbcp.DriverConnectionFactory;
>>> -import org.apache.commons.dbcp.PoolableConnectionFactory;
>>> import org.apache.commons.dbcp.managed.LocalXAConnectionFactory;
>>> import org.apache.commons.dbcp.managed.ManagedDataSource;
>>> +import org.apache.commons.dbcp.managed.PoolableManagedConnectionFactory;
>>> import org.apache.commons.dbcp.managed.XAConnectionFactory;
>>> import org.apache.commons.pool.impl.GenericObjectPool;
>>> import org.ofbiz.base.util.Debug;
>>> @@ -32,16 +43,6 @@ import org.ofbiz.entity.datasource.Gener
>>> import org.ofbiz.entity.transaction.TransactionFactory;
>>> import org.w3c.dom.Element;
>>>
>>> -import javax.transaction.TransactionManager;
>>> -import java.sql.Connection;
>>> -import java.sql.Driver;
>>> -import java.sql.SQLException;
>>> -import java.util.HashMap;
>>> -import java.util.Map;
>>> -import java.util.Properties;
>>> -
>>> -import javolution.util.FastMap;
>>> -
>>> /**
>>>  * DBCPConnectionFactory
>>>  */
>>> @@ -146,7 +147,7 @@ public class DBCPConnectionFactory imple
>>>
>>>
>>>             // create the pool object factory
>>> -            PoolableConnectionFactory factory = new PoolableConnectionFactory(xacf, pool, null, null, true, true);
>>> +            PoolableManagedConnectionFactory factory = new PoolableManagedConnectionFactory(xacf, pool, null, null, true,
>>>             true); factory.setValidationQuery("select 1 from entity_key_store where key_name = ''");
>>>             factory.setDefaultReadOnly(false);