We have a system whereby the database connection is get once using a common method, and being pass throughout the relevant class to be used. There are doubts that passing the database connection as a parameter to different classes would cause problem, so i’m checking here to see whether this is actually viable, and are there any better patterns to do it?
I know there are some ORM tools to do the persistence but we can’t go into that, yet..
Any feedback is welcomed, thanks.
2
Yes it is safe to pass around a connection. You handle the connection in an outer controlling block. There is nothing unsafe about it.
What is unsafe is writing code that does not guarantee the connection is properly disposed in a timely manner. Forgetting to clean up a resource is unrelated to passing it around. You could just as easily write code that leaves a hanging connection without passing it anywhere.
In C++, you are protected by RAII if you allocate on the stack or use smart pointers. In C# make a hard rule that all disposable objects (such as connections) be declared in a “using” block. In Java clean up with try-finally logic. Have code reviews on all data layer code to ensure this.
The most common use-case is when you have several operations that can be combined in many permutations. And each of these permutations need to be an atomic-transaction (all succeed or rollback). then you must pass the transaction (and therefore the corresponding connection) around to all the methods.
Suppose we have many foobar() actions that can be combined in various ways as atomic-transactions.
//example in C#
//outer controlling block handles clean up via scoping with "using" blocks.
using (IDbConnection conn = getConn())
{
conn.Open();
using (IDbTransaction tran = conn.BeginTransaction())
{
try
{//inner foobar actions just do their thing. They don't need to clean up.
foobar1(tran);
foobar2(tran);
foobar3(tran);
tran.Commit();
}
catch (Exception ex)
{ tran.Rollback(); }
}
}//connection is returned to the pool automatically
BTW you will want to open connections as late as possible, dispose them as soon as possible. Your teammates could be right if you are treating connections as object members, introducing them as unnecessary state, and leaving connections open much longer than necessary. But the act of passing a connection or transaction as a parameter is not inherently wrong.
BTW. Depending on your language’s support for first class functions you may take in a list of foobar() actions. So one function could handle all permutations of the actions. Eliminating duplication of the outer controlling block for each permutation.
1
It sounds like you’re after Dependency Injection. That is, the pooled connection gets created once and injected whereever it’s needed. Certainly passing in the connection via a method parameter is one way to dependency inject, but an IoC container such as Guice, PicoContainer or Spring is another (safer) way you can do this.
Using DI means you can neatly wrap up the logic around the creation, opening, usage and closing of the connection – away from your core business logic.
Spring JDBC et al are otehr examples of performing this sort of behaviour for you
2
Passing around database things rather than data things can lead to problems. To that extent, whenever it practical, don’t pass a database thing unless one can guarantee proper database hygiene.
The problem with passing around database things is that it can be sloppy. I have seen more than one bug in code with someone passing around a database connection, that someone then grabs a result set to and stashes in a local object (the result set, still connected to the database) and then ties up a cursor in the database for a significant time. Another instance someone passed a result set to someone else (which was then stashed) and then method that passed the result set closed it (and the statement) leading to errors when other methods tried to work with the result set that wasn’t anymore.
All of this stems from not respecting the database, the connection, the statement, the result set, and their lifecycles.
To avoid this, there are existing patterns and structures that play more nicely with databases and don’t have database things need to get out of the classes they are confined in. Data goes in, data goes out, the database stays put.
5
Passing Connection
instances around is not usually a problem, even though in most situation only the DAO implementations should have anything to do with them. Now, with your problem being about connections not being closed after used, it is actually easy to fix: the Connection
object needs to be closed at the same level it is opened, i.e. in the same method. I personally use the following code pattern :
final Connection cnx = dataSource.getConnection();
try {
// Operations using the instance
} finally {
cnx.close();
}
That way I ensure all connections are always closed, even if an exception is thrown within the block. I actually go as long as using the exact same pattern for Statement
and ResultSet
instances, and everything has been smooth sailing so far.
Edit 2018-03-29: As indicated by user1156544 in the comments below, starting with Java 7 the use of the try-with-resources construct should be favoured. Using it, the code pattern I provided in my initial answer can be simplified like so:
try (final Connection cnx = dataSource.getConnection()) {
// Operations using the instance
}
5
there’s a tradeoff to doing things this way rather than using a singleton which you can get as needed. I have done things both ways in the past.
In general, you need to think about the consequences of database connection management, and this may or may not be orthogonal to database query usage. For example, if you have one db connection for a given application instance and it gets closed when not in use, that would be orthogonal. Put the management in a singleton class and don’t pass it around. This allows you to manage the db connection as you need. For example, if you want to close a connection on every commit (and re-open on the next call) this is easier to do on a singleton because the API for this can be centralized.
On the other hand, suppose you need to manage a pool of connections where a given call may need to use any arbitrary connection. This might happen when doing distributed transactions across multiple servers, for example. In this case you are usually far better off passing the db connection object than you are working with singletons. I think this is usually the rarer case, but there isn’t anything wrong with doing it when you need to.