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?

Answer 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.