Bug 17388 - Result set created in query tag is never released + update tag
Summary: Result set created in query tag is never released + update tag
Status: RESOLVED FIXED
Alias: None
Product: Taglibs
Classification: Unclassified
Component: Standard Taglib (show other bugs)
Version: 1.2.0
Hardware: All All
: P3 critical with 6 votes (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2003-02-25 18:40 UTC by camilo vasquez
Modified: 2007-03-01 14:03 UTC (History)
0 users



Attachments
Patch sent by Dhiru to the devs list (1.02 KB, patch)
2004-12-16 14:07 UTC, Felipe Leme
Details | Diff
patch for query tag (780 bytes, patch)
2005-04-19 16:59 UTC, Ivica Loncar
Details | Diff
Patch for 1.1 and 1.0 tags to close result set (2.56 KB, patch)
2007-01-23 12:08 UTC, Jimmy Mitchener
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description camilo vasquez 2003-02-25 18:40:45 UTC
Result set created in query tag is never released, resulting in rdbms error: ' 
maximum open cursors exceeded'.

The jsp code is very like this:

<c:set var="date_begin" value="${param.begin_date}"/>
<c:set var="date_end" value="${param.end_date}"/>

<sql:query var="manifs">
SELECT key_cuo, key_voy_nber, key_dep_date from car_gen
  where car_reg_date is not null and car_reg_date >= to_date(?,'YYYYMMDD') and 
car_reg_date <= to_date(?,'YYYYMMDD')
<sql:param value="${date_begin}"/>
<sql:param value="${date_end}"/>
</sql:query>

<c:forEach var="man" begin="0" items="${manifs.rows}">
  <sql:query var="ops">
 		SELECT key_cuo, key_voy_nber, key_dep_date, key_bol_ref, 
car_wgt_avl, car_pkg_avl,  max(car_ope_nbr)
    from car_bol_ope
    where key_cuo = ? and key_voy_nber = ? and 
    key_dep_date = ? and (car_pkg_avl    > 0 or car_wgt_avl > 10)
    group by key_cuo, key_voy_nber,key_dep_date, key_bol_ref, car_wgt_avl, 
car_pkg_avl
  	<sql:param value="${man.KEY_CUO}"/>
  	<sql:param value="${man.KEY_VOY_NBER}"/>
  	<sql:param value="${man.KEY_DEP_DATE}"/>
  </sql:query>
	<c:forEach var="op" begin="0" items="${ops.rows}">
	  <sql:query var="dts">
			SELECT a.key_cuo cuo, a.key_voy_nber voy, TO_CHAR
(a.key_dep_date, 'YYYY/MM/DD') dep, a.key_bol_ref bol, 
    	? wgt, ? pkg, a.carbol_cons_nam cons, TO_CHAR
(b.car_reg_date, 'YYYY/MM/DD') reg
  		from   car_bol_gen a, car_gen b
  		where (a.key_cuo = b.key_cuo and a.key_voy_nber = 
b.key_voy_nber and a.key_dep_date = b.key_dep_date) and 
			a.key_cuo = ? and a.key_voy_nber = ? and a.key_dep_date 
= ? and a.key_bol_ref = ? and 
     (carbol_status = 0 or carbol_status = 4)
  		<sql:param value="${op.CAR_WGT_AVL}"/>
  		<sql:param value="${op.CAR_PKG_AVL}"/>
  		<sql:param value="${op.KEY_CUO}"/>
  		<sql:param value="${op.KEY_VOY_NBER}"/>
  		<sql:param value="${op.KEY_DEP_DATE}"/>
  		<sql:param value="${op.KEY_BOL_REF}"/>
		</sql:query>
		<c:forEach var="dt" begin="0" items="${dts.rows}">
		
		<!-- <TR> row here using ${dt.<column_name} -->
		</c:forEach>
	</c:forEach>
</c:forEach>
Comment 1 Pierre Delisle 2003-02-26 23:14:41 UTC
Hummm, I just checked the code and we definitely close the connection in 
the doFinally() of the tag handler (you don't use transactions, right?)

    /**
     * Close the <code>Connection</code>, unless this action is used
     * as part of a transaction.
     */
    public void doFinally() {
	if (conn != null && !isPartOfTransaction) {
	    try {
		conn.close();
	    } catch (SQLException e) {} // Not much we can do
	}

	conn = null;
	parameters = null;
    }

I suggest you create a much simpler test case that reproduces the problem
you are describing and try it out again. 
If you still experiment the problem, send us the *simple* test case and 
we'll be happy to run it and figure out what the problem is.

Changing the state to "WORKSFORME" until more details can be provided.
Comment 2 camilo vasquez 2003-03-18 16:54:50 UTC
Apparently I'm using a pooled database connection, for that reason the result 
sets never becomes disconnected, although the connection has been closed 
several times (one for each </sql:query>). 

That fact, added to the fact that I use query tags inside <sql:forEach> loops, 
causes the rdbms to rise fault 'too many cursors open'.

I added a 'statement close' in the file QueryTagSupport.java, and my problem 
disappeared.

This is the code I left in the 
org/apache/taglibs/standard/tag/common/sql/QueryTagSupport.java file (line 
~249):

	try {
	    PreparedStatement ps = conn.prepareStatement(sqlStatement);
	    setParameters(ps, parameters);
	    ResultSet rs = ps.executeQuery();
	    result = new ResultImpl(rs, startRow, maxRows);
            ps.close(); // added 
	}
	catch (Throwable e) {
	    throw new JspException(sqlStatement + ": " + e.getMessage(), e);
	}
Comment 3 Pierre Delisle 2003-04-29 19:01:30 UTC
According to experts in the JDBC field, 
"all Statement objects (and their inherited statements PreparedStatement,
CallableStatement) will be closed when the connection that created them is closed."

So this should not be a problem.

However, experts will also say:
"It is however good coding practise for applications to close the statements as
soon as they have finished processing them."

In light of this, and if it can solve the reported problem, I have therefore
committed the suggested fix.
Comment 4 Mike Marrotte 2004-10-15 15:09:30 UTC
I found the same problem with the <sql:update> tag.  You might consider 
closing the statement after getting the result set in this case, too.
Comment 5 Mike Marrotte 2004-10-15 15:12:39 UTC
I found the same problem with the <sql:update> tag.  You might consider 
closing the statement after getting the result set in this case, too.
Comment 6 Justyna Horwat 2004-10-19 20:53:19 UTC
Closed statement object in update tag as soon as processing is finished instead of waiting for the 
connection close to clean up the statements.
Comment 7 Mike Marrotte 2004-10-25 19:33:08 UTC
<sql:transaction> provides nested database action elements with a shared 
Connection, set up to execute all statements as one transaction.  
Unfortunetly, each statement remains open until the entire transaction 
completes.  Leaving statements open usually mean leaving database resources 
open.  For example each <sql:update> opens an Oracle cursor.  And the maximum 
open cursors in Oracle is typically set to a relativley small number, e.g. 
500.  Why should a JSTL user have to worry about explicilty closing statements 
that <sql:update> tag opens?  In addition, what tag or object would be used to 
explicitly do this?  

It's always good programming practice to close a statemnt after it's used.  
The <sql:update> is an agent for the tag consumer that opens database 
statements and should finally close them after the body of the tag is executed.

Comment 8 Dhiru Pandey 2004-12-15 01:37:08 UTC
(In reply to comment #7)

Need some clarification on this comment.

> <sql:transaction> provides nested database action elements with a shared 
> Connection, set up to execute all statements as one transaction.  
> Unfortunetly, each statement remains open until the entire transaction 
> completes.  Leaving statements open usually mean leaving database resources 
> open.  For example each <sql:update> opens an Oracle cursor.  And the maximum 
> open cursors in Oracle is typically set to a relativley small number, e.g. 
> 500.  Why should a JSTL user have to worry about explicilty closing statements 
> that <sql:update> tag opens?  In addition, what tag or object would be used to 
> explicitly do this?  
> 
In looking thru the code, I did find that 2 places where PreparedStatement was
being used, and it was being closed right after. Is there something else that 
we should be doing here ? (Files were QueryTagSupport.java and 
UpdateTagSupport.java.)

I did submit a patch request for one issue that I did find in closing of the
prepared statement. Here is what I thought was a potential problem: 

A change is needed in the current implementation, if an error occurs
before the preparedStatement is closed, then the closing of the
preparedStatement would depend on the implementation of finalize() for
the PreparedStatement class which should be called when that object is
GCed (Garbage Collected). This is not a good practice since:

a) we should not depend on the implementation of the finalize() method
     in PreparedStatement
b) the invocation of the finalize() method is not guaranteed by the JVM.
     So even if finalize() does call close(), there is no guarantee
     that finalize() itself will be called when this object is GCed
     and we may continue to see memory leaks and resource leaks
     as reported in this Bug

This is true for both the files (QueryTagSupport.java and 
UpdateTagSupport.java.)

> It's always good programming practice to close a statemnt after it's used.  
> The <sql:update> is an agent for the tag consumer that opens database 
> statements and should finally close them after the body of the tag is executed.
> 
> 
Agreed

In light of the above facts, shouldn't this bug be closed. Please clarify if
anything more is needed here
Comment 9 Felipe Leme 2004-12-16 14:07:05 UTC
Created attachment 13765 [details]
Patch sent by Dhiru to the devs list
Comment 10 Dhiru Pandey 2005-02-17 02:20:31 UTC
The patch submitted by me was applied by Pierre Delisle (thanks Pierre)

  This is needed because in the current implementation, if an error occurs
  before the preparedStatement is closed, then the closing of the
  preparedStatement would depend on the implementation of finalize() for
  the PreparedStatement class which should be called when that object is
  GCed (Garbage Collected). This is not a good practice since:
  
  a) we should not depend on the implementation of the finalize() method
       in PreparedStatement
  b) the invocation of the finalize() method is not guaranteed by the JVM.
       So even if finalize() does call close(), there is no guarantee
       that finalize() itself will be called when this object is GCed
       and we may continue to see memory leaks and resource leaks
       as reported here.

This should fix the memory and resource leaks observed. Closing this bug.

-Dhiru
Comment 11 Ivica Loncar 2005-04-19 16:18:44 UTC
This will not work with Oracle database. Oracle JDBC documentation states
(oracle.jdbc.driver.OracleResultSet):

Note: Because finalization is problematic for many Java VMs, Oracle JDBC does
not have finalizer methods on OracleConnection, OracleStatement and its
subclasses, nor OracleResultSet and its subclasses. User code must explicitly
close these by sending the close method.
Comment 12 Ivica Loncar 2005-04-19 16:59:55 UTC
Created attachment 14757 [details]
patch for query tag

I have created a patch for query tag. Please validate.
Comment 13 Jimmy Mitchener 2007-01-23 12:08:37 UTC
Created attachment 19444 [details]
Patch for 1.1 and 1.0 tags to close result set
Comment 14 Kris Schneider 2007-01-23 13:15:47 UTC
I think the fix is actually a little messier than that ;-). I'm in the process
of putting something together and I'll check it in shortly.
Comment 15 Kris Schneider 2007-01-29 11:04:20 UTC
Just a quick note that I checked-in a fix last week (r499150):

http://svn.apache.org/viewvc/jakarta/taglibs/proper/standard/trunk/src/org/apache/taglibs/standard/tag/common/sql/QueryTagSupport.java?view=diff&rev=499150&r1=499149&r2=499150

I guess I won't close this until it can be tested properly...
Comment 16 Henri Yandell 2007-03-01 14:03:26 UTC
The commit looks good to me. Given that it's for when there are problems with
the database, it's not the easiest thing to test.

QueryTagSupport does get tested by the 33054 test. Marking this as fixed.