Originally Posted by
Snakeday
Nice catch!
It's not such a great idea to rely on the garbage collector to close your sql connections though (but it's ok). What should be done is configure the threadpool(s) to close the connections when they destroy a thread.
But then again - it's much better to do it this way than the way it was before (but I'm unsure if it was like this in clean OdinMS)
I agree; however, if the autoReconnect parameter is not included in the URL, the connection would timeout regardless of the object being collected sooner versus later (this was the main goal). This is especially true since ThreadLocal may or may not do anything directly when the thread dies from what I've found at least. This is still not the greatest implementation ever; and, I would personally lean towards just having a set pool for connections, but that would not really fit well in a release thread considering one modification needing to be made for everywhere a Connection object is required can get really bad for the purposes of a small release.
Default OdinMS did not necessarily have the "Too Many Connections" problems since they did not use autoReconnect in the URL by default. My guess is that the one issue it resolved is that the change was made later down the line to combat connections timing out. Also, I think I may be wrong about ThreadLocal removing the object when the thread dies. Not very helpful when online resources completely vary for what exactly constitutes thread death (making this a gray area and I would rather not try figuring out how exactly the JVM handles ending threads for something as trivial as this).
I guess at the end of the day this release is meant to dissuade people from using autoReconnect versus just refreshing the connection in the source when you need it since it causes a major problem in the long run. The root of the problem was primarily people having bad ideas on how to fix connections timing out before thread death (though I kind of want to blame Frz for using ThreadLocal to begin with).
Here's an example of something using pooled connections for anyone interested. This requires more work to manage since you have to release your connection after each usage (pulled from Invictus by Zygon, I just made javadocs for it):
Code:
public final class Database {
/**
* The connection URL for the database. Connections will be attempted to be
* made to this URL.
*/
private String URL;
/**
* Manages the amount of sessions that can exist within a service/process
* rather than using something like ThreadLocal which can leak.
*/
private final Semaphore s;
/**
* Connection properties that are used for the database credentials.
*/
private final Properties cProp;
/**
* Safety variable for preventing new connections after closing.
*/
private boolean closed = false;
/**
* Queue that contains all the connections (references to this collection
* are thread-safe).
*/
private final ConcurrentLinkedQueue<Connection> queue;
static {
try {
Class.forName("com.mysql.jdbc.Driver"); // touch the driver first
} catch (Exception e) {
System.err.println("SEVERE : SQL Driver has not been located during runtime. Please make sure it is in the correct path.");
System.exit(0);
}
}
/**
* Creates a new Database object using specific credentials and information
* for maintaining the server.
*
* @Param url the URL for the database.
* @Param user the user credential for the database.
* @Param pass the password credential for the database.
* @Param connections the amount of connections that can exist in this
* service at one time.
* @return a new Database object for usage in a service.
*/
public static Database newDatabase(String url, String user, String pass, int connections) {
Database db = new Database(connections);
db.URL = url;
db.cProp.setProperty("user", user);
db.cProp.setProperty("password", pass);
return db;
}
/**
* @See Database#newDatabase(java.lang.String, java.lang.String,
* java.lang.String, int)
*/
private Database(int connections) {
cProp = new Properties();
s = new Semaphore(connections, true);
queue = new ConcurrentLinkedQueue();
}
/**
* Closes this database and closes all connections.
*/
public final void close() {
try {
s.tryAcquire(10, 1, TimeUnit.HOURS);
} catch (Exception e) {
return;
}
for (Connection c : queue) {
try {
c.close();
} catch (Exception e) {
e.printStackTrace();
}
}
closed = true;
}
/**
* Acquires a new connection using a default timeout for the Semaphore to
* acquire a connection. If the time has passed, an exception will be thrown
* and no connection will be given.
*
* @return ideally a connection to the desired database; if nothing pans out
* and the Semaphore has to wait too long, no connection will be returned.
*/
public final Connection getConnection() {
return getConnection(60000);
}
/**
*
* @Param timeout time-out for the Semaphore.
* @return a connection to the database.
* @See Database#getConnection()
*/
public final Connection getConnection(long timeout) {
if (URL == null || cProp.isEmpty() || closed) {
return null;
}
try {
s.tryAcquire(timeout, TimeUnit.MILLISECONDS);
} catch (Exception e) {
return null;
}
Connection c = queue.poll();
if (c == null) {
try {
c = DriverManager.getConnection(URL, cProp);
} catch (Exception e) {
e.printStackTrace();
s.release();
return null;
}
} else { // validates connection is still active
try {
c.getMetaData();
} catch (Exception e) {
try {
c = DriverManager.getConnection(URL, cProp);
} catch (Exception ex) {
ex.printStackTrace();
s.release();
return null;
}
}
}
return c;
}
/**
* Releases a connection back into the queue for usage later. This is a
* critical step in this model since if the semaphore is blocked forever,
* then the server cannot possibly function properly.
*
* @Param c the connection to be released back into the queue.
*/
public final void release(Connection c) {
try {
if (!c.isClosed()) {
queue.add(c);
}
} catch (Exception e) {
} finally {
s.release();
}
}
}
Reply With Quote
