I need to insert into database which has two columns-
ID PrimaryKey String
ACCOUNT String
So that means each thread should be using unique id always and I need to store the same ID
in Account
column also. So suppose if ID is 1
then in Database it should be stored as
ID Account
1 SomeString+1
2 SomeString+2
3 SomeString+3
....
..
100 SomeString+100
I am concatenating that userID with that String always in Account column.
Below is my multithreaded code which will spawn multiple threads- And each thread will be getting a new unique ID everytime as I am using AtomicInteger
for that. And it will insert that ID
to ID column
and also append that ID
to Account
column
But somehow in my below program what I have seen in that database is-
ID Account
1 String+2
2 String+1
3 String+3
Which is not right. It should be something like this-
ID Account
1 String+1
2 String+2
3 String+3
Below is the code
public static void main(String[] args) {
final int noOfThreads = 4;
final int noOfTasks = 10;
final AtomicInteger id = new AtomicInteger(1);
ExecutorService service = Executors.newFixedThreadPool(noOfThreads);
for (int i = 0; i < noOfTasks * noOfThreads; i++) {
service.submit(new Task(id));
}
}
class Task implements Runnable {
private final AtomicInteger id;
private volatile int userId;
public Task(AtomicInteger id) {
this.id = id;
}
@Override
public void run() {
dbConnection = getDBConnection();
preparedStatement = dbConnection.prepareStatement(Constants.INSERT_ORACLE_SQL);
userId = id.getAndIncrement();
preparedStatement.setString(1, String.valueOf(userId));
preparedStatement.setString(2, Constants.getaAccount(userId));
preparedStatement.executeUpdate();
}
}
And below is my Constants class
which I have made immutable.
public final class Constants {
public static String A_ACCOUNT;
public final static String INSERT_ORACLE_SQL = "INSERT INTO XMP_TEST"
+ "("
+ "ID, A_ACCOUNT) VALUES"
+ "(?, ?)";
public static String getaAccount(int userId) {
A_ACCOUNT = "{\"lv\":[{\"v\":{\"userId\":"+userId+"},\"cn\":1}]}";
return A_ACCOUNT;
}
}
Can anyone tell me what wrong I am doing here? I believe it's happening because of thread safety issue. Multiple threads modifying the userID
integer I guess and that's why it is getting written wrongly to database.
How can I fix this problem?
The major problem I see is not with Task.userId
, but rather with Constants.A_ACCOUNT
: if two separate threads call getaAccount
at the same time, then they'll both set Constants.A_ACCOUNT
and both read it, so they can end up both having the same value, or each having the other's value, or whatnot. To fix this, you can use a local variable instead of a static field:
public static String getaAccount(int userId) {
final String ret = "{\"lv\":[{\"v\":{\"userId\":"+userId+"},\"cn\":1}]}";
return ret;
}
or just dispense with the variable:
public static String getaAccount(int userId) {
return "{\"lv\":[{\"v\":{\"userId\":"+userId+"},\"cn\":1}]}";
}
(You say that you've made Constants
immutable, but that's not really true. Instances of Constants
will be immutable, because they have no fields at all; but Constants
itself has a publically modifiable field, so it's very mutable!)
More generally, you shouldn't be using fields for temporary values only needed within a specific method, and only during a single call to it. Even when it's not a synchronization problem, it's a maintenance problem. For example, Task
does not need a volatile int userId
; userId
should just be a local variable inside its run
method.
Also, I'd recommend wrapping your AtomicInteger
in its own class, IncrementingCounter
or something, that offers just one method, called (say) getNewId
. Then getNewId
will be the only class that has to deal with coordination between threads. All other classes can be made threadsafe by regular techniques (immutability, only existing within a single thread, etc.).