Database connection - should they be passed as parameter?

Question

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.

• Greg Hewgill: What sort of problems are you referring to? Who has these doubts? (Not you, I assume.)
• ipohfly: Problems like causing developer forgetting to close the connection, generally i'm just trying to see whether is it a good practice to pass around the database connection to various methods as a parameter. Ya the doubts comes from another developer.

Answer 1

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.

• ipohfly: marking this as the answer as it gives me more idea on how the situation is

Answer 2

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

• ipohfly: Emm not really looking at dependency injection. Just trying to find out whether generally is it a good practice to do that, and if it's not, what is the better way of managing the database connection (DI is one way of doing it though).
• mike30: -1. One connection does not fit a multi-user system. It may appear to work due to low user volume and fast execution. With pooling, it is better to instantiate a connection object per-action even in a single user system.

Answer 3

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.

• jqa: +1 db connections should have the shortest timespan possible. Open it, use it, close it as fast as possible. Nowadays there a plenty of connection pool implementations so using a connection for multiple operations a false economy. And an invitation for bugs or performance problems (holding locks on tables, using connection resources)
• Daniel Kaplan: What are the names of some of these existing patterns and structures?
• user40980: @tieTYT The primary ones that comes to the forefront is the Data access object which serves to hide the database from the rest of the application. See also Data Access Layer and Object-Relational Mapping
• Daniel Kaplan: When I think of those patterns I feel they're at a higher level of abstraction than what he's asking about. Let's say you've got a way to get a Root from a Dao. But then you realize you also want a way to get a Node without pulling out the whole Root object with it. How do you make the Root Dao calls the Node Dao code (ie: reuse), but make sure the Node Dao only closes the connection when the Node Dao is directly called and keeps the connection open when the Root Dao is called?
• TMN: Just wanted to add that if you're not in auto-commit mode, passing a connection around could lead to a situation where one object updates the database, then another (possibly unrelated) object gets the connection, has an error, and winds up rolling back the first object's changes. These types of errors can be very difficult to debug.

Answer 4

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
}

• user470365: I use something similar. I have function doInTransaction(DbTask task), where DbTask is my interface with method with connection parameter. doInTransaction obtains connection, calls task and commit (or rollback if there was exception) and close that connection.
• ipohfly: judging from your example, it would mean that the DataSource object is a singleton?
• KevinLH: @ipohfly Actually I should have named that object dataSource rather than DataSource (I'll fix my answer regarding that point). The exact type of that object would be javax.sql.DataSource. In old code I used to have a singleton manage all the available data sources within my applications. My DAOs did not have to know about that through, as the DataSource instance is provided through dependency injection.
• user1156544: If you use this schema, use try-with-resources better
• KevinLH: Back when I answered, I wasn't yet using Java 7. But you are right that this should be the preferred way these days. I'll update my answer to include your suggestion.

Answer 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.