Anurag Anurag - 5 months ago 31
MySQL Question

Multithreaded write on MySQL DB with JDBC and c3p0

I have written a DAO class which allows several threads invoked by

ExecutorServices
to write to MySQL DB.

EDIT: I am using c3p0 to create a JDBC ConnectionPool. So every new thread will get a new
JDBC Connection
by calling

DataBaseManager.getInstance().getConnection()


There seems to be random concurrency issue while executing, e.g:

java.sql.SQLException: No value specified for parameter 1
at com.eanurag.dao.DataBaseManager.writeData(DataBaseManager.java:102)


I am not able to understand all the issues with the code. Should I just synchronize entire
writeData()
?

public class DataBaseManager {

private final static Logger logger = Logger.getLogger(DataBaseManager.class);

private static volatile DataBaseManager dbInstance = null;

private DataBaseManager() {
cpds = new ComboPooledDataSource();
try {
cpds.setDriverClass("com.mysql.jdbc.Driver");
} catch (PropertyVetoException e) {
logger.error("Error in Initializing DB Driver class", e);
}
cpds.setJdbcUrl("jdbc:mysql://" + DB_HOST + "/" + DB_NAME);
cpds.setUser(DB_USER);
cpds.setPassword(DB_PASS);

cpds.setMinPoolSize(MINIMUM_POOL_SIZE);
cpds.setAcquireIncrement(INCREMENT_SIZE);
cpds.setMaxPoolSize(MAXIMUM_POOL_SIZE);
cpds.setMaxStatements(MAX_STATEMENTS);
}

public static DataBaseManager getInstance() {
if (dbInstance == null) {
synchronized (WorkerManager.class) {
if (dbInstance == null) {
dbInstance = new DataBaseManager();
}
}
}

return dbInstance;
}

private ComboPooledDataSource cpds;

private static final Integer MINIMUM_POOL_SIZE = 10;
private static final Integer MAXIMUM_POOL_SIZE = 1000;
private static final Integer INCREMENT_SIZE = 5;
private static final Integer MAX_STATEMENTS = 200;

private volatile Connection connection = null;
private volatile Statement statement = null;
private volatile PreparedStatement preparedStatement = null;

private static final String DB_HOST = "localhost";
private static final String DB_PORT = "3306";
private static final String DB_USER = "root";
private static final String DB_PASS = "";
private static final String DB_NAME = "crawly";
private static final String URL_TABLE = "url";


public Connection getConnection() throws SQLException {
logger.info("Creating connection to DB!");
return this.cpds.getConnection();
}

public Boolean writeData(URL url) {
StringBuffer writeDBStatement = new StringBuffer();
writeDBStatement.append("insert into");
writeDBStatement.append(" ");
writeDBStatement.append(DB_NAME);
writeDBStatement.append(".");
writeDBStatement.append(URL_TABLE);
writeDBStatement.append(" ");
writeDBStatement.append("values (?,?,default)");

Boolean dbWriteResult = false;

try {
connection = DataBaseManager.getInstance().getConnection();

preparedStatement = connection.prepareStatement(writeDBStatement.toString());
preparedStatement.setString(1, url.getURL());
preparedStatement.setString(2, String.valueOf(url.hashCode()));
dbWriteResult = (preparedStatement.executeUpdate() == 1) ? true : false;


if(dbWriteResult){
logger.info("Successfully written to DB!");
}
} catch (SQLException e) {
logger.error("Error in writing to DB", e);
} finally {
try {
preparedStatement.close();
connection.close();
} catch (SQLException e) {
e.printStackTrace();
}
}
return dbWriteResult;
}


}

AWT AWT
Answer

What is happening here?

public Connection getConnection() throws SQLException {
    logger.info("Creating connection to DB!");
    return this.cpds.getConnection();
}

Namely, what does cpds.getConnection() do? When you call:

connection = DataBaseManager.getInstance().getConnection();

Your connection object is a member of what's supposed to be a singleton class here but every call to writeData() overwrites it it with a new getConnection() call. Is the getConnection() call thread unsafe as well?

Also, why is the connection object declared as a class member and then overwritten each time writeData() is called? In a multi-threaded environment, the code as it exists allows for the connection object to be overwritten by another getConnection() call immediately before prepareStatement() is called, since access to writeData() is not locked. Same for preparedStatement. Move those into the writeData() method.