Here is some idea for you:
First, you have two static methods to get and return the connection inside a
listener class. Those methods do not have anything to do with the listener
(which is an instance), and also and also do not need synchronization.
Synchronizing on getConnection can be a big bottleneck, especially if you
are doing a validation query onBorrow. Having said that, it sounds like this
is the least of your problems now.
I think you can have a solution without changing your code.
Try something like this:
getConnection() static method should get the connection, and add it to a
list that you keep in threadlocal.
recycleConnection() should close the connection and remove the connection
object from thread local.
Add a servlet filter that closes all connections in thread local. The filter
calls next filter, and in a finally block get the connections from thread
local, close all of them, and clear the list in thread local.
This will allow you to keep your legacy code. As far as I remember DBCP has
an option to close the result sets and statements when you close the
connection. If not this will partly work.
This is as close as you can get to having the connections reclaimed
immediately without rewriting your application.
Something like:
public static /* NOT synchronized */ Connection getConnection(){
Connection c = ...// get the connection from JNDI/DBCP
List<Connection> connections = // get or create from thread local
if(connections.contains(c) == false)
// make sure c.equals() works as object reference equality, eg ==
// You may have to read DBCP code for this or add a break point and trace
through the code until you get to equals()
connections.add(c);
if(myLogger.isDebug())
myLogger.debug(“Request consumed “+ list.size() + “ connection(s)”);
return c;
}
public void doFilter(ServletRequest req, ServletResponse res, FilterChain
chain) throws IOException, ServletException {
try{
// handle request
chain.doFilter(req, res);
}finally{
List<Connection> connections = ...// get from thread local
if (connections != null){
for(Connection c: connections){
try{
c.close();
}catch(Throwable t){
myLogger.error(“Error on connection close”, t);
}
}
connections.clear();
}
}
}
public static synchronized void recycleConnection(Connection c) {
List<Connection> connections = ...// get from thread local
connections.remove(c);
try {
c.close();
} catch (Throwable t) {
// DO NOT eat the exception, especially when there are known issues in the
connection management code
myLogger.warn(“Error closing connection”, t);
}
}
I also think that I have seen somewhere a configuration that does the above
for you, but if not its not a lot of code to do yourself. Map the filter to
all the URL mappings that can possibly can open connection, which in the
worse case is /*.
Notice that with the code sample you wrote, a typical application will
consume many connections for one request (possible one per statement), and
worse than that, will not be able to perform transactional writes (since
each write is on a separate connection).
Version 2: Advanced
Keep the actual connection in thread local. You will have one connection per
HTTP request. getConnection() should be something like
public static /* NOT synchronized */ Connection getConnection(){
Connection c = ...// get the connection from thread local
if (c != null)
return c;
Connection c = ...// get the connection from JNDI/DBCP
// put connection in thread local
return c;
}
recycleConnection(){
// empty, connection will be recycled by filter.
}
public void doFilter(ServletRequest req, ServletResponse res, FilterChain
chain) throws IOException, ServletException {
try{
// handle request
chain.doFilter(req, res);
}finally{
List<Connection> connections = ...// get from thread local
if (connections != null){
for(Connection c: connections){
try{
c.close();
}catch(Throwable t){
myLogger.error(“Error on connection
close”, t);
}
}
connections.clear();
}
}
}
public void doFilter(ServletRequest req, ServletResponse res, FilterChain
chain) throws IOException, ServletException {
try{
// handle request
chain.doFilter(req, res);
}finally{
Connection c = ...// get and REMOVE from thread local
if (c != null){
try{
c.close();
}catch(Throwable t){
myLogger.error(“Error on connection close”, t);
}
}
}
}
The filter is the same, but not using lists. Version 2 will give you one
connection per servlet request, which normally I would not do for a well
written DAO code, but in your case you are not really dealing with such
code. You can get large performance boost by having one connection per
request and closing it immediately. Your connection usage will be close to
optimal. It may not be 100% in the transactional sense, but it does not look
like your legacy code is.
E
On Fri, Oct 30, 2009 at 8:38 PM, Josh Gooding <josh.good...@gmail.com>wrote:
> Chris,
>
> I was looking at that earlier, wondering why it was put in there in the
> first place. It just doesn't fit in. Sometimes you just hate to inherit
> someone else's mess.
>
> While there is another school of thought telling me to re-write the entire
> DAO (which I could be willing to later on) for right now, I want to just
> tweak and get it to work more efficiently on the server. I think this is a
> HUGE improvement over what was there and what actually was going on. Thank
> you sir!
