Bug 37262

Created attachment 16814 [details]
NamingContextListener.java patch

Created attachment 16815 [details]
LocalStrings.java patch

No, we will not add proprietary code to release any resource provider resources
(obviously, almost every resource provider will need something ...), but rather
it is up to it to have its cleanup code. DBCP can do this by closing inactive
connections, and everything should eventually be GCed.

Obviously, feel free to use your own custom code as needed.

While it might not seem elegant to introduce DBCP specific code, it is the
responsibility of the container to close its MANAGED resources. DBCP has no way
of knowing that the tomcat context is  stopped and the pooled connections must
be closed.

An elegant solution would be (as JBoss does) to wrap the resources in a tomcat
class that listens to lifecycle events of the context and calls DBCP to close
the pooled connections when the context is stopped.

I've reopened the bug just so that others can see my comment. Feel free to close
it again, but I think DB connections leaks are a pretty serious.

You don't have to reopen and waste my time for others to see your comment.

(In reply to comment #3)
> No, we will not add proprietary code to release any resource provider resources
> (obviously, almost every resource provider will need something ...), but rather
> it is up to it to have its cleanup code. DBCP can do this by closing inactive
> connections, and everything should eventually be GCed.
> 
> Obviously, feel free to use your own custom code as needed.

Remy, 

I would agree that there would be a big can of worms here if Tomcat needed to
import:
+import com.mycompany.MyResource;
+import com.acme.MyBasicDataSource;

etc., etc., non-integrated resources, just to keep manually closing each
resource--there would be no end to it.

But looking at the import statement needed for this patch:
+import org.apache.--->tomcat<---.dbcp.dbcp.BasicDataSource;

It seems that DBCP has already been specifically integrated within
Tomcat[1]--i.e., the proprietary code is already there--and for that reason,
Tomcat should perhaps take care of DBCP cleanup needs.

OTOH, is there a bug with the implementation of DBCP that is not causing these
connections to close on its own--i.e., insufficient timeouts or other
enhancements that DBCP can (rightfully?) provide that will not require any
changes to Tomcat?  Maybe a patch should be submitted to DBCP instead.

Also, slightly different topic, needing to modify and rebuild Tomcat in order to
close proprietary resource providers seems nonideal.  Is there a cleaner way
within the servlet specification that this can be done without needing to recode
& rebuild Tomcat?  Just asking as a newbie here--I'm hardly a Tomcat/servlet expert.

Thanks,
Glen

[1] http://tinyurl.com/7ebqn

If the Tomcat team decides that DBCP is a "proprietary" external library that
does not deserve proper integration and furthermore connections leaks are
something totally acceptable, than the documentation at
http://tomcat.apache.org/tomcat-5.5-doc/jndi-datasource-examples-howto.html
should be updated to reflect this position. Funny enough, the doc guides the
users how not to leak connections, but when tomcat does it, it's OK :-(

(In reply to comment #7)
> If the Tomcat team decides that DBCP is a "proprietary" external library that
> does not deserve proper integration and furthermore connections leaks are
> something totally acceptable, than the documentation at
> http://tomcat.apache.org/tomcat-5.5-doc/jndi-datasource-examples-howto.html
> should be updated to reflect this position. Funny enough, the doc guides the
> users how not to leak connections, but when tomcat does it, it's OK :-(

Hmmm...is the responsibility of Tomcat to just release DBCP when it is no longer
needed (by any webapp it is hosting), or DBCP's connections as well when just
one webapp no longer needs it?  Interesting question--the latter may be too
granular for it.

At any rate, please check the Servlet 2.4 Specification, the very small section
SRV.10.2.2 -- for the example given, it appears to indicate that the Developer
(that's you ;) should create a servlet context listener class in order to close
database connection(s).  If so, the only issue appears to be then, does Tomcat
give you access to the DBCP object(s) necessary to do such connection closing?

Glen

Glen, of course I close my connections. But the datasource uses a connection
pool and the underlying JDBC connection in not really closed but just returned
to the pool. It is the responsibility of the container who manages the
datasource to call close() (on the datasource, not the connection) when the
context is stopped.

Part of the problem is that javax.sql.DataSource does not provide a close()
method, only the DBCP DataSource does. But this makes sense to me, because
javax.sql.DataSource is an interface for the users, and the appserver should use
its own means to cleanup the datasource. The users should not be able to close a
datasource, because it is meant to be managed by the appserver.

Here is a quote from the JDBC 3.0 spec the mentions that an appserver
implementation closes a datasource.

"A connection pool manager shuts down a physical connection by calling the
method PooledConnection.close. This method is typically called only in
certain circumstances: when the application server is undergoing an orderly
shutdown, when the connection cache is being reinitialized, or when the
application server receives an event indicating that an unrecoverable error has
occurred on the connection." 

The connection manager is DBCP but it has no way of knowing that the tomcat
context is stopped. That's why, for a proper integration there needs to be some
glueing code that listens to tomcat lifecycle events and notifies DBCP. It
doesn't strictly belong to Tomcat, it doesn't strictly belong to DBCP, so that's
why it does not exist.

(In reply to comment #9)
> Glen, of course I close my connections. But the datasource uses a connection
> pool and the underlying JDBC connection in not really closed but just returned
> to the pool. 

I know -- SRV10.2.2 was just a "for example" -- it implies the types of things
the Developer can/should do, such as create a listener to close the DBCP when
the webapp is shut down.

However, I can't see how this would be doable within Tomcat, given that the
object is created within server.xml.  The Developer does not have access to the
Java object external to the Tomcat source code; they have to recode Tomcat as
you've done here.

> It is the responsibility of the container who manages the
> datasource to call close() (on the datasource, not the connection) when the
> context is stopped.

Well, this seems to be too specific--the Servlet 2.4 spec doesn't mention data
sources and close() methods specifically.  So I'm not so certain how much of a
container requirement this is.


> 
> Part of the problem is that javax.sql.DataSource does not provide a close()
> method, only the DBCP DataSource does. But this makes sense to me, because
> javax.sql.DataSource is an interface for the users, and the appserver should use
> its own means to cleanup the datasource. 

If that were the case, I think there would have to be a standard definition for
a data source, as well as a "close()" method that it must implement within the
Servlet spec.

> The users should not be able to close a
> datasource, because it is meant to be managed by the appserver.

OK--but what if a datasource is shared by multiple applications/instances of
Tomcat, etc.?  (possible?) Then we wouldn't want it to be closed automatically
whenever a webapp is shut down.


> 
> Here is a quote from the JDBC 3.0 spec the mentions that an appserver
> implementation closes a datasource.
> 
> "A connection pool manager shuts down a physical connection by calling the
> method PooledConnection.close. This method is typically called only in
> certain circumstances: when the application server is undergoing an orderly
> shutdown, when the connection cache is being reinitialized, or when the
> application server receives an event indicating that an unrecoverable error has
> occurred on the connection." 
> 

But this doesn't seem to rule out that the developer won't be creating an
application event listener that is activated to specifically shut down the data
source (data source API's vary, so I don't see how the app server can do this in
a common manner for all data sources.)  But again, I don't see how this is
doable within Tomcat in an server.xml-created connection pool.

> The connection manager is DBCP but it has no way of knowing that the tomcat
> context is stopped. That's why, for a proper integration there needs to be some
> glueing code that listens to tomcat lifecycle events and notifies DBCP. It
> doesn't strictly belong to Tomcat, it doesn't strictly belong to DBCP, so that's
> why it does not exist.

I think there are two problems here (again, newbie speaking here):
1.)  The application developer does not have programmatic access to the
connection pool created in server.xml, so he/she cannot call close() on it from
a developer-created application event listener even if he/she wanted to.  (Your
opinion is that this should not need to be done anyway, it should be automatic.)

2.)  Tomcat integrates DBCP enough to allow for creation of a connection pool
from server.xml, but is not calling close() on those connection pools after the
webapp is shut down.  

I don't have an opinion either way on this; I'm just trying to understand the
issue, although I wonder if other users have complained about this as well, and
if not, why not.

Glen

I'm not talking about resources from server.xml but rather context.xml which are
not shared with other webapps.

*** Bug 29497 has been marked as a duplicate of this bug. ***