Java DAO pattern: singleton and threadsafe?

K

koenxjans

Hello all,

Here's my problem:
I'm working on a legacy J2EE application that uses the DAO pattern
and implements all DAO's as singletons. There's one abstract DAO
superclass.

But this class holds the connection to the database as an instance
variable.
Which, in my opinion, is not thread safe.

If multiple clients would make a call simultaneously, the very same
connection
would be used for the different calls, resulting in an exception on
the app server.


I think a different connection should be gotten from the datasource
connection
pool on everycall? And that the datasource itself should be an
instance variable.



All opinons are appreciated!

Thanks,
Koen
 
K

koenxjans

Hello all,
Here's my problem:
I'm working on a legacy J2EE application that uses the DAO pattern
and implements all DAO's [sic] as singletons. There's one abstract DAO
superclass.

That the DAOs are singletons and that there is one abstract superclass are
orthogonal.
But this class holds the connection to the database as an instance
variable.
Which, in my opinion, is not thread safe.

It's a lot easier to make an instance variable thread safe than a static
variable. The problem isn't that there's an instance variable, the problem is
that you're using a singleton. Stop using the class as a singleton.
If multiple clients would make a call simultaneously, the very same
connection
would be used for the different calls, resulting in an exception on
the app server.
I think a different connection should be gotten from the datasource
connection
pool on everycall? And that the datasource itself should be an
instance variable.

I suggest that you provide an SSCCE - a simple, self-contained compilable
example that illustrates the points you're making. All this vague hand-waving
doesn't admit of any useful comment. The example should elucidate the
"singleton-ness" of the approach.



Hey Lew,

Thanks for your comment. To provide you with some (simplified) code:

The superclass looks like this:
(this can be abstract, although subclasses are singletons ;) )

-------------------------------------


package nl.nedcar.apollo.server.dao;

import java.sql.Connection;
import java.sql.SQLException;
import java.sql.Statement;

import javax.naming.InitialContext;
import javax.naming.NamingException;
import javax.sql.DataSource;


public abstract class AbstractDAO {

private DataSource dataSource;
private Connection connection;
private final static String JNDI_NAME = "yourJndiName";

protected AbstractDAO() {
InitialContext initial;
try {
initial = new InitialContext();
DataSource dataSource = (DataSource) initial.lookup(JNDI_NAME);
Logger.debug(this, "Obtained ref to DataSource " + JNDI_NAME);
this.dataSource = dataSource;
}
catch (NamingException e) {
e.printStackTrace();
}
}

private Connection getConnection() throws SQLException {
return dataSource.getConnection();
}

protected void releaseConnection() throws SQLException {
if(connection != null) {
connection.close();
}
connection = null;
}

protected Statement getStatement() throws SQLException {
if(connection != null) {
RuntimeException rt = new
RuntimeException(this.getClass().getName() + ": Creating new statement
while previous query was not " + "properly closed. \nClosing previous
connection...");
releaseConnection();
throw rt;
}
connection = getConnection();
return connection.createStatement();
}

}






The singletons subclasses look like:


-------------------------------------


package nl.nedcar.apollo.server.dao;

import java.sql.ResultSet;

public class FirstDAO extends AbstractDAO {

private static FirstDAO theInstance;


public static synchronized FirstDAO getInstance() {
if(theInstance == null) {
theInstance = new FirstDAO();
}
return theInstance;
}


public String getSomethingFromDatabase() throws Exception {
try {
ResultSet s = getStatement().executeQuery("select something from
users");
if(s.next()) {
return s.getString("username");
}
return null;
}
finally {
releaseConnection();
}
}

}



-------------------------------------

calls are made like:

String somethingFromDb =
FirstDAO.getInstance().getSomethingFromDatabase();



So, the problem occurs when multiple clients are performing a call at
the moment:
client1 holds the connection, while client2 is attempting to use the
very same connection.
 
K

koenxjans

package nl.nedcar.apollo.server.dao;
import java.sql.ResultSet;
public class FirstDAO extends AbstractDAO {
private static FirstDAO theInstance;

Here's your trouble. You will also note than none of the methods or other
accesses to shared state are synchronized. This code was designed to fail.
public static synchronized FirstDAO getInstance() {
if(theInstance == null) {
theInstance = new FirstDAO();
}
return theInstance;
}
public String getSomethingFromDatabase() throws Exception {
try {
ResultSet s = getStatement().executeQuery("select something from
users");

Note that
[a] ResultSet object is automatically closed when the Statement object that
generated it is closed, re-executed, or used to retrieve the next result from
a sequence of multiple results.
if(s.next()) {
return s.getString("username");
}
return null;
}
finally {
releaseConnection();
}
}

Yep. Designed to fail.


Thanks for your reply.
One more question to clarify things for me..


In my opinion, this legacy code is indeed designed to fail.
Because the superclass holds an instance of the connection.
But, what if, like I mentionned in my first post, an instance
of the datasource is kept in the superclass, not the connection
itself.


With the superclass code like:


protected Statement getStatement() throws SQLException {
return dataSource.getConnection().getStatement();
}

protected void releaseResources(ResultSet resultSet) throws
SQLException {
...
}



If 2 threads would access the getSomethingFromDatabase() method
simultaneously,
would..

a) both threads use a difference connection?
So operations on the resultset of one thread, would not interfere
with
the resultSet of the other thread.
b) one thread still take the resultSet of the other thread, breaking
the design?
This would mean Singleton DAO's are always a no go.



Thanks!
Koen
 
K

koenxjans

Here's your trouble. You will also note than none of the methods or other
accesses to shared state are synchronized. This code was designed to fail.
Note that
[a] ResultSet object is automatically closed when the Statement object that
generated it is closed, re-executed, or used to retrieve the next result from
a sequence of multiple results.
if(s.next()) {
return s.getString("username");
}
return null;
}
finally {
releaseConnection();
}
}
}
Yep. Designed to fail.

Thanks for your reply.
One more question to clarify things for me..

In my opinion, this legacy code is indeed designed to fail.
Because the superclass holds an instance of the connection.
But, what if, like I mentionned in my first post, an instance
of the datasource is kept in the superclass, not the connection
itself.

With the superclass code like:

protected Statement getStatement() throws SQLException {
return dataSource.getConnection().getStatement();

}

protected void releaseResources(ResultSet resultSet) throws
SQLException {
...

}

If 2 threads would access the getSomethingFromDatabase() method
simultaneously,
would..

a) both threads use a difference connection?
So operations on the resultset of one thread, would not interfere
with
the resultSet of the other thread.
b) one thread still take the resultSet of the other thread, breaking
the design?
This would meanSingletonDAO'sare always a no go.

Thanks!
Koen



Oh,

I think it's thread safe and the answer is a).
Because only local variables are used.
And every caller gets their own copy of local variables in a method.

Koen
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,755
Messages
2,569,535
Members
45,007
Latest member
obedient dusk

Latest Threads

Top