tamalsen / vibur-dbcp Goto Github PK

Originally pointed out here:

https://github.com/brettwooldridge/HikariCP/wiki/Pool-Analysis#vibur-dbcp

The JDBC contract specifies that when a Connection is closed, any open 
Statements that have not been closed yet should automatically be closed, 
otherwise a resource leak on the database server-side could occur. Only in the 
case of cached PreparedStatements or CallableStatement does Vibur perform such 
resource cleanup. Even when caching is enabled ordinary Statement objects are 
not tracked for cleanup. When caching is disabled no Statements are tracked at 
all.

========================================================
Further comments:

The best suggested practice to close JDBC resources should use idiom similar to 
the below:

Connection conn = null;
PreparedStatement stmt = null;
ResultSet rs = null;
try {
    conn = // Retrieve connection
    stmt = conn.prepareStatement(// Some SQL);
    rs = stmt.executeQuery();
} catch (...) {
    // Error Handling
} finally {
    try { if (rs != null) rs.close(); } catch (Exception e) {};
    try { if (stmt != null) stmt.close(); } catch (Exception e) {};
    try { if (conn != null) conn.close(); } catch (Exception e) {};
}

If this idiom is used no resource leak on database server side or on client 
application side could occur, and if one is not working directly with JDBC API 
but say via something like Spring JDBC, this idiom will be always and properly 
used. 

The Javadoc for JDBC Connection says: "Releases this Connection object's 
database and JDBC resources immediately instead of waiting for them to be 
automatically released." which doesn't explicitly clarify whether the JDBC 
Statements created from this Connection have to be explicitly closed or not. 
For comparison, the Javadoc for closing JDBC Statements says explicitly: 
"Note:When a Statement object is closed, its current ResultSet object, if one 
exists, is also closed."

Additional judgement on this issue is required in order to determine whether 
this is a genuine issue which needs to be addressed, as well as whether the 
Statements closure has to be always enabled or configurable.

Different usernames and passwords are not supported yet and the call to 
`DataSource.getConnection(username, password)` will simply log a warning 
message and then will create and return a connection using the configured 
default username and password.

Using different usernames and passwords for creating connections to a database 
(from one and the same application) is a rarely used feature, and implementing 
support for it will be considered based on users requests and demand. 

Until then an obvious workaround will be to use different connection pools for 
the different usernames and passwords required, i.e. if the application needs a 
pair of different usernames and passwords for accessing the database, 2 
different connection pools can be created and used by the application.

slf4j-log4j12 is not nessesary. according slf4j philosofy backing log 
implementation must define library consumer, not developer...

Originally pointed out here:

https://github.com/brettwooldridge/HikariCP/wiki/Pool-Analysis#exception-detecti
on

Vibur, like HikariCP and BoneCP, can detect when a SQLException indicates that 
a Connection has been severed. This is a good thing, but there are two issues. 
Vibur's list of detected SQL error codes is more limited than either HikariCP 
or BoneCP. 

Surprisingly, when Vibur detects that an exception has occurred on one 
connection, it ejects all connections from the pool, not just the connection 
that encountered the exception.

===================================================================
Further comments:

The first part of this issue most likely refers to the handling of 
criticalSQLStates "08001", "08007", "08S01", "57P01", although this has to be 
confirmed. It has occurred to me that the list of the criticalSQLStates after 
which all JDBC Connections to the SQL database server have to be closed, may 
slightly vary from one SQL database server to another and likely the best 
programming practice will be to make this list of criticalSQLStates 
configurable.

As to the second part of the original issue report, Vibur DBCP ejects all 
connections from the pool only if the current SQL Exception has one of the 
criticalSQLStates (and this is intentional), but not otherwise.

Originally pointed out here:

https://github.com/brettwooldridge/HikariCP/wiki/Pool-Analysis#auto-commit-handl
ing-2


When the connection is configured for autocommit=false a connection pool should 
perform the test query or isValid() test within its own transaction, otherwise 
there is a transaction in progress when the connection is handed to the 
consumer. Calling a method like setReadOnly() on a "fresh" connection from 
Vibur will throw an exception. Vibur does not encapsulate connection tests.

Originally pointed out here:

https://github.com/brettwooldridge/HikariCP/wiki/Pool-Analysis#sql-warnings-2

A connection pool should clear SQL warnings via Connection.clearWarnings() 
either when the Connection is returned to the pool or before it is taken from 
the pool. Vibur does not do this.

==============================================================
Further comments:

Determine whether calling Connection.clearWarnings() is necessary, as well as 
whether it has to happen always or to be configurable.

Originally pointed out here:

https://github.com/brettwooldridge/HikariCP/wiki/Pool-Analysis#unsafe-defaults-3

By default does not reset connection auto-commit and transaction isolation 
levels.

=============================================================
Further comments:

This default setting was intentionally done as is. May need to document it a 
bit more clearly, as well as to explain why it was decided this way.

What steps will reproduce the problem?
1. Using Vibur in an OSGi container (in my case Equinox)

What do you see instead?
org.vibur.dbcp.ViburDBCPException: java.sql.SQLException: No suitable driver 
found for XXX

What version of the product are you using? On what operating system?
vibur-dbcp 1.0.0

Please provide any additional information below.
This is a common problem with OSGi and JDBC drivers. This blog post [1] 
contains a few solutions/workarounds. HikariCp faced the same problem and chose 
the DynamicImport-Package route [2].

[1] http://freemanfang.blogspot.de/2012/03/how-to-use-jdbc-driver-in-osgi.html
[2]