What's the correct way of handling concurrency?

Question

I have a servlet which accesses DB through a singleton class named DBManager. This class contains a reference to EntityManager. This is my code:

package database;


import java.util.List;

import javax.persistence.EntityManager;
import javax.persistence.EntityTransaction;
import javax.persistence.Query;

import com.btc.EMF;

import database.entities.User;



public class DBManager {
    public class UserAlreadyExistsException extends Exception{
        private static final long serialVersionUID = 1L;}
    /* attributes */
    private static DBManager instance = null;

    private final EntityManager em = EMF.get().createEntityManager();
    private int tokens = 1;
    private final EntityTransaction transaction = em.getTransaction();


    private DBManager(){
    }

    public static DBManager getInstance(){
        if(instance == null){
            instance = new DBManager();
        }
        return instance;
    }

    @SuppressWarnings("unchecked")
    public synchronized boolean createUser(String username, String password) throws UserAlreadyExistsException {
        while(tokens == 0){
            try {
                wait();
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
        }
        tokens--;
        Query q = em.createQuery("select u from "+User.class.getName()+" u");
        List<User> list = q.getResultList();
        if(list != null){
            if(list.size()>0){
                throw new UserAlreadyExistsException();
            }
        }
        User u = new User();
        u.setUsername(username);
        u.setPassword(password);
        transaction.begin();
        em.persist(u);
        transaction.commit();
        tokens++;
        this.notifyAll();
        return true;
    }

    @SuppressWarnings("unchecked")
    public synchronized void eraseDB(){
        while(tokens == 0){
            try {
                wait();
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
        }
        tokens--;
        Query q = em.createQuery("select u from "+User.class.getName()+" u");
        List<User> list = q.getResultList();
        transaction.begin();
        for(User u : list)
            em.remove(u);
        transaction.commit();
        tokens++;
        this.notifyAll();
    }

    public EntityManager getEm() {
        return em;
    }

}

The serlvet simply calls eraseDB() and createUser() in sequence.

I added synchronized methods and wait() and notifyAll() in order to make concurrency work but I failed. If I keep pressing F5 in the browser some error occurs due to concurrency problems. And also this usage of wait and notify appears too complex.

What's the correct way to do this?

I think you should create transaction every time you update DB, not once per DBManager instance, also your singleton pattern implementation is not thread safe — hoaz, May 02 '14 at 19:48
@GoldenJam The first error is a logical error: two elements appear in the DB (this is not possible, see UserAlreadyExistsException line). The following errors are due to this error. — HAL9000, May 02 '14 at 19:49

score 1 · Accepted Answer · edited May 23 '17 at 10:32

1

Basically, every object that handles Connection and related resources must be initialized in the narrowest scope. Apart of this, you're naively and badly applying singleton for your DBManager which will be used among several threads because multiple requests are served by a single servlet instance (more info on this: How do servlets work? Instantiation, sessions, shared variables and multithreading).

The best option would be removing the singleton (anti) pattern from your DBManager class and make sure to initialize DBManager in the narrowest scope where the database resources should be handled.

edited May 23 '17 at 10:32

Community

1
1

answered May 02 '14 at 19:47

Luiggi Mendoza

85,076
16
154
332

Thank you for the answer. I removed the singleton pattern and now it seems to work better. However I still think that the usage of wait/notify in my code is wrong/ugly. And also there is still a problem: if the DB is empty and I call the servlet with two threads, two identical users are added to the DB. – HAL9000 May 02 '14 at 19:58
@HAL9000 you should handle those things at database level using an unique key, not in application. – Luiggi Mendoza May 02 '14 at 20:00
Maybe in this case I should handle it in DB. But in general this means that concurrency is not working properly, in my opinion. – HAL9000 May 02 '14 at 20:05
@HAL9000 that's not related to concurreny: two or more threads execute the same query at the same time (none of them have modified any data in database at all), database will return **the current database results** (in this case, none since the table is empty). All the threads will retrieve an empty list result from database (expected behavior), then every thread will, by its own, try to add a new user. When you need to avoid duplicate data in your database table, use an unique key. – Luiggi Mendoza May 02 '14 at 20:07

What's the correct way of handling concurrency?

1 Answers1