DAO methods and synchronized

Question

The following are methods I currently have in an Abstract DAO class. If there are concurrent calls, are they safe as they are or should synchronization be used? I know synchronization should be used if there is a reference to an attribute outside the scope of a method but it's unclear to me how should things be handled with an outside ressource.

public Connection getConnection() {
    // Call to singleton handling JDBC stuff
    return Database.getInstance().getCon();
}

public boolean isConnectionAvailable(){     
    if( getConnection() != null ){
        return true;
    }

    return false;
}

public PreparedStatement getPreparedStatement( String sqlStatement ){
    Connection connection = getConnection();
    PreparedStatement pS = null;

    if( connection != null ){
        try {
            pS = connection.prepareStatement( sqlStatement );
        } catch (SQLException e) {
            return null;
        }
    }

    return pS;
}

Edit: I might reformulate this question to include information on writing DAOs as it's what's important here.

Piccoloist

do not do not do not do not use singletons for database connection access – Roughhew

I often hear that. But what is the alternative when you're working with plain Java? – Piccoloist

Answer 1

I don't agree with this implementation at all.

First, DAOs should be given their connection information by the services that own units of work and transactions.

Second, I don't see an interface.

Third, I don't see model or domain objects.

Fourth, prepared statements should only be part of the internal implementation. If they're leaking out of your DAO, you're doing it wrong.

Fifth, passing a prepared statement out of the object makes responsibility for closing it and cleaning up far less clear. Your DAO will die a resource leak death in no time.

Here's an interface for a generic DAO. You'll notice that it's all CRUD operations, with no mention of connnections or any interfaces from java.sql package:

package persistence;

import java.io.Serializable;
import java.util.List;

public interface GenericDao<T, K extends Serializable>
{
    T find(K id);
    List<T> find();
    List<T> find(T example);
    List<T> find(String queryName, String [] paramNames, Object [] bindValues);

    K save(T instance);
    void update(T instance);
    void delete(T instance);
}

You can go a long way with this. It's a better abstraction. The T is your business object type, and K is the primary key.

Mccaffrey

Spring has a decent DAO framework: static.springsource.org/spring/docs/2.5.x/reference/dao.html – Adlay

More than decent. This is the model to follow. – Mccaffrey

Thanks for the answer duffymo. I have to admit that I am struggling a bit as I've been asked to not use a framework such as Spring. if you have an example that could be used as a template, it would be more than welcome :) . – Piccoloist

@James: that is really unfortunate, I have a hard time understanding the motivation behind requests like that (that you not use a framework). Spring in particular has been a huge time-saver for me. – Playwriting

Answer 2

If getCon() returns a new Connection each time it is called, or returns a ThreadLocal connection, then you are safe and there is no need to use synchronized

If you return the same connection to everyone you might still save in terms of synchronization, because there is no state in the connection that gets changed (in your current code). But you should avoid this practice. Consider a connection pool instead.

And a few notes on general design principles. DAOs form a separate layer. Each layer exists for a reason, not juts for the sake of having cool names. The DAO layer exists in order to abstract, or in other words - hide the database access from the services that use the DAO objects. In order to imagine it more clearly - the DAO has to be written in a way that if tomorrow you decide to switch from RDBMD storage (via JDBC) to XML storage, you should be able to do that by changing only the DAO objects and nothing else.

Ijssel

The concrete DAO classes work as you've explained. The methods above were just an idea to add some utility methods in the Asbtract class but perhaps this is a bad approach. What I probably haven't understood yet is how it should interact with JDBC and a database. – Piccoloist

Answer 3

The JDBC Connection class is not guaranteed to be thread safe. If your Database.getInstance().getCon() method is always returning the same connection, then you will run into problems. If, however, it is using a pool such that each call to getInstance().getCon() returns a different connection you will be fine.

That said, if you are returning a different connection with each call to getCon() then the getPreparedStatement() won't work if you want two Prepared Statement calls to use the same connection (and same transaction).

I like Spring's JDBCTemplate class as a basis for my DAO classes.

Adlay

Connection is just an interface, whether or not the implementations are thread-safe are not are completely up to the JDBC driver being used – Roughhew

Answer 4

You shouldn't use the same connection in every thread. JDBC drivers aren't supposed to be thread safe. If you want a thread safe code, you should create one connection for every thread.

The rest of your code seems safe.

Defect

Do you think I should add some sort of threading mecanism inside the DAO? – Piccoloist

It's not your DAO which isn't thread safe, it's the connection part. Like @Ijssel said if getCon() return each time a specific connection for each thread then there is no race condition. – Defect

Ok, so what you're saying is that I should isolate any accesses to a connection. I've had a look in the singleton and it returns the same connection each time so that might be an issue. – Piccoloist

No, the connection should be passed into the DAO. Let the object that knows about units of work - the service - instantiate the Connection, give it to the DAO, and then clean it up in method scope. – Mccaffrey

Answer 5

Look at how Spring does it, they have already figured out all this stuff and there is no need to re-invent it. Check out the petclinic sample code that is bundled with the full distribution of Spring, or (for a non-Spring approach) read the DAO chapter of the Bauer/King Hibernate book.

Definitely the DAO shouldn't be in charge of getting the database connection, because you will want to group multiple DAO calls in the same transaction. The way Spring does it is there's a service layer that gets its transactional methods wrapped in an interceptor that pulls the connection from the datasource and puts it in a threadlocal variable where the DAOs can find it.

Eating your SQLException and returning null is bad. As duffymo points out, letting the PreparedStatement get passed around with no assurance that it will ever get closed is very bad. Also, nobody should use raw JDBC anymore, Ibatis or spring-jdbc are better alternatives.

Playwriting

the -1 seems a bit harsh. Have you ever used spring jdbc ? It is nearly raw jdbc, where you will still manipulate ps and rs (if you wish) but with all the exception handling (and resource management) and interaction with tx taken care of. So you have the advantage of raw access to data with less risk of leak. – Cassareep

I think it's a defendable position, GreenieMeanie. I'd prefer that people use Spring just for JDBC instead of the heinous JDBC code I see on SO. I voted Nathan Hughes up. – Mccaffrey

@stepanian: why? my position is JDBC is too painful to do right, even for experts. the result is either cut-n-paste code or an in-house framework that is probably inferior to spring-jdbc or Mybatis (both of which have had a lot of work put into them). what is so wrong with that? – Playwriting

Nathan Hughes: It's one thing to say that you prefer to use spring-jdbc, mybaits, ibatis, unnecessarybatis, or whatever framework you love. It's another thing to say something absolute like "nobody should use raw JDBC anymore". Java development is all about choice. I don't want to debate with you on the merits and shortcomings of frameworks and how they relate to various categories of projects, there is a lot of that on the web. But I hope you realize that your preferences of coding techniques are not always shared by everyone. – Cymbiform

Answer 6

Connection is just an interface. It is not thread safe. JDBC drivers are not thread safe either.

Swahili

Answer 7

Looks fine to me. The functions are Thread safe.

Coop