Multiple Threads trying to access a Map.

use java.util.concurrent.ConcurrentHashMap instead of your Map

private ConcurrentHashMap map = new ConcurrentHashMap()

Also remove 'final' modifier from orderId... however I'm not sure about that whether it makes problem or not but I think you can remove that final modifier.

if you want to synchronize threads per order IDs it's a little complex. You need to use java.util.concurrent.Lock object to lock a specific lines. But I think that ConcurrentHashMap works fine

That is an ObjectMap from ObjectGrid caching technology. I can't change that.
I am thinking more like the one in attachment.
What do you think of that?

private final static Map<String, String> concurrentHashMap = new ConcurrentHashMap<String, String>();

@Override
    public List<FModel> getModels(final String orderId) {
        List<FModel> result = new ArrayList<FModel>();
        Session session = txManager.getSession();
        ObjectQuery query = session.createObjectQuery("SELECT f " +
                                                      "FROM Models f " +
                                                      "WHERE f.orderId=?1");
        query.setParameter(1, orderId);
        Iterator iter = query.getResultIterator();
        while (iter.hasNext()) {
            result.add((FModel) iter.next());
        }
        return result;
    }

    @Override
    public void removeModelsForOrder(final String orderId) {
        while(concurrentHashMap.containsKey(orderId));
    		concurrentHashMap.put(orderId, orderId);
    		try {
                List<FModel> modelList = getModels(orderId);
                ObjectMap modelsMap = getModelsMap();
                for (FModel model : modelList) {
                    try {
                        ModelsMap.remove(model.getCorrelationId());
                    } catch (ObjectGridException e) {
                        throw new DaoException("Could not remove model for order " + model.getOrderId() +
                                ", correlation id: " + model.getCorrelationId(), e);
                    }
                }
               } finally {
            	   concurrentHashMap.remove(orderId);
    	       }
    }

Select allOpen in new window

However because the getModel() method only reads data I don't think it has problem but what I doubt is about that Session and the createQuery() method... so if you used ConcurrentHashMap and still you get the same models from getModel() you need to use lock as following codes and you can't lock it per user ID

private final Lock lock = new ReentrantLock();

@Override
    public List<FModel> getModels(final String orderId) {
        List<FModel> result = new ArrayList<FModel>();
        lock.lock();
        try{
        Session session = txManager.getSession();
        ObjectQuery query = session.createObjectQuery("SELECT f " +
                                                      "FROM Models f " +
                                                      "WHERE f.orderId=?1");
        query.setParameter(1, orderId);
        Iterator iter = query.getResultIterator();
        }finally {
          lock.unlock();
        }
        while (iter.hasNext()) {
            result.add((FModel) iter.next());
        }
        return result;
    }

    @Override
    public void removeModelsForOrder(final String orderId) {
        
                List<FModel> modelList = getModels(orderId);
                ObjectMap modelsMap = getModelsMap();
                for (FModel model : modelList) {
                    try {
                        ModelsMap.remove(model.getCorrelationId());
                    } catch (ObjectGridException e) {
                        throw new DaoException("Could not remove model for order " + model.getOrderId() +
                                ", correlation id: " + model.getCorrelationId(), e);
                    }
                }
    }

Select allOpen in new window

Can you please let me know how to use Lock in my specific scenario? Thanks.

No the code you attached is not correct and the problem still exists... if you can't change that ObjectMap you can use the another Lock for that section at the time of remove.

private final Lock removelock = new ReentrantLock();

  @Override
    public void removeModelsForOrder(final String orderId) {
        
                List<FModel> modelList = getModels(orderId);
                ObjectMap modelsMap = getModelsMap();
                removelock.lock();
                try {
                for (FModel model : modelList) {
                    try {
                        ModelsMap.remove(model.getCorrelationId());
                    } catch (ObjectGridException e) {
                        throw new DaoException("Could not remove model for order " + model.getOrderId() +
                                ", correlation id: " + model.getCorrelationId(), e);
                    }
                }
                }finally {
                   removelocl.unlock
                }
    }

Select allOpen in new window

the form of using lock is as follow:

private final Lock lock = new ReentrantLock();
.
.
.
  lock.lock();
  try {
     //code shared among threads
  }finally {
    lock.unlock();
  } 

Select allOpen in new window

I am aware of using Central Locking mechanism that you are doing.
lock.lock();
But, in ObjectGrid the problem is when two threads have the same data before it goes to remove from Map, it is bad, really bad.
1) ModelsMap.remove(model.getCorrelationId());
2) If it doesn't find inside the Map, it queries the database which we don't want at all.

The only way two threads come to this point is when

One thread is coming from Application1 which says mydataSize == App2dataSize. So, go ahead and remove the data from the Map.

Other thread is coming from Application2 which says mydataSize == App1dataSize. So, go ahead and remove the data from the Map.

I can use the synchronized method, but I don't want to synchronize access to the method which affects other orders which does not have the scenario mentioned above.

So, when one thread is coming to the removeModelsForOrder(12345), other threads should wait until the thread that is working on removeModels is done. after which it can resume working again.

When it resumes, it queries the objectMap and it finds no data, so it won't go to the remove() of the Map.

So, in the scenario you mentioned above.

List<FModel> modelList = getModels(orderId);
                ObjectMap modelsMap = getModelsMap();
                removelock.lock();
                try {
                for (FModel model : modelList) {
                    try {
                        ModelsMap.remove(model.getCorrelationId());
                    } catch (ObjectGridException e) {
                        throw new DaoException("Could not remove model for order " + model.getOrderId() +
                                ", correlation id: " + model.getCorrelationId(), e);
                    }
                }
                }finally {
                   removelocl.unlock
                }

It calls the getModelsMap(orderId) by all the threads. So, all the threads might have similar data/ some data if first thread has already removed some data for that particular order.

So, each and every thread is waiting to remove the particular CorrId from the Map apart from the first thread. When first thread actually completes processing, other threads are going to do stuff which are already done(removed). So, it results in an exception trying to delete something that is already deleted.

well that is why I used two lock... check this snippet:

the problem you have stated for ObjectMap
2) If it doesn't find inside the Map, it queries the database which we  don't want at all.

I think before calling remove method you have to check if ID is not exists then remove it...
 map.containsKey(...)

By the way, can you instantiate ObjectMap using ConcurrentHashMap as follow?

ObjectMap map = new ConcurrentHashMap()

private final Lock lock = new ReentrantLock();
private final Lock removelock = new ReentrantLock();

@Override
    public List<FModel> getModels(final String orderId) {
        List<FModel> result = new ArrayList<FModel>();
        lock.lock();
        try{
        Session session = txManager.getSession();
        ObjectQuery query = session.createObjectQuery("SELECT f " +
                                                      "FROM Models f " +
                                                      "WHERE f.orderId=?1");
        query.setParameter(1, orderId);
        Iterator iter = query.getResultIterator();
        }finally {
          lock.unlock();
        }
        while (iter.hasNext()) {
            result.add((FModel) iter.next());
        }
        return result;
    }




  @Override
    public void removeModelsForOrder(final String orderId) {
        
                List<FModel> modelList = getModels(orderId);
                ObjectMap modelsMap = getModelsMap();
                removelock.lock();
                try {
                for (FModel model : modelList) {
                    try {
                        ModelsMap.remove(model.getCorrelationId());
                    } catch (ObjectGridException e) {
                        throw new DaoException("Could not remove model for order " + model.getOrderId() +
                                ", correlation id: " + model.getCorrelationId(), e);
                    }
                }
                }finally {
                   removelocl.unlock
                }
    }

Select allOpen in new window

I think I figured out your problem.... now check this snippet code:

Hope this works

 @Override
    public void removeModelsForOrder(final String orderId) {
        
                List<FModel> modelList = getModels(orderId);
                
                ObjectMap modelsMap = getModelsMap();
                try {
                for (FModel model : modelList) {
                    removelock.lock();
                    try {
   		      if(modelsMap.containsKey(model.getCorrelationId()) {	
                          ModelsMap.remove(model.getCorrelationId());
                       }
                    }finally {
                      removelocl.unlock
                    }


                }
    }

Select allOpen in new window

sorry I removed your try-catch block only to clear the code... and there is a inappropriate try { mistakenly I put

Your containsKey makes sense. But,

if(modelsMap.containsKey(model.getCorrelationId()) hits the database.
I made this mistake long back according to cache it represents database.
So, if the key is not present in the Map, it will go and check the database and fetch the entry from there.

The Map is an ObjectMap not an ordinary Map. ObjectMap

 private ObjectMap getModelsMap() {
        try {
            return txManager.getSession().getMap(MAP_NAME);
        } catch (UndefinedMapException e) {
            throw new DaoException(MAP_NAME + "is not defined!", e);
        }
    }


and also why my code is not working(sometimes)?

Code is attached.

private final static Map<String, String> concurrentHashMap = new ConcurrentHashMap<String, String>();

    private synchronized void containsKey(String orderId) {
    	while(concurrentHashMap.containsKey(orderId));
    	concurrentHashMap.put(orderId, orderId);
    }

@Override
    public List<FModel> getModels(final String orderId) {
        List<FModel> result = new ArrayList<FModel>();
        Session session = txManager.getSession();
        ObjectQuery query = session.createObjectQuery("SELECT f " +
                                                      "FROM Models f " +
                                                      "WHERE f.orderId=?1");
        query.setParameter(1, orderId);
        Iterator iter = query.getResultIterator();
        while (iter.hasNext()) {
            result.add((FModel) iter.next());
        }
        return result;
    }

    @Override
    public void removeModelsForOrder(final String orderId) {
        containsKey(orderId);
                try {
                List<FModel> modelList = getModels(orderId);
                ObjectMap modelsMap = getModelsMap();
                for (FModel model : modelList) {
                    try {
                        ModelsMap.remove(model.getCorrelationId());
                    } catch (ObjectGridException e) {
                        throw new DaoException("Could not remove model for order " + model.getOrderId() +
                                ", correlation id: " + model.getCorrelationId(), e);
                    }
                }
               } finally {
                   concurrentHashMap.remove(orderId);
               }
    }

Select allOpen in new window

The way I am testing this is using ExecutorService.

@setUp() 
{
        modelDao.persistModel(model5);
        modelDao.persistModel(model6);
        modelDao.persistModel(model7);
        modelDao.persistModel(model8);
}

    class Threads2RemoveOrder implements Callable<Integer>{
    	public Integer call() {
    		modelsDao.removeModelForOrder(orderId3);
    		return modelsDao.getModel(orderId3).size();
    	}
    }

    @Test
    public void threadToRemoveOrderTest() throws InterruptedException, ExecutionException, TimeoutException {
    	
    	ExecutorService service = Executors.newFixedThreadPool(5);
    	
    	Future<Integer> retVal1 = service.submit(new Threads2RemoveOrder());
    	Future<Integer> retVal2 = service.submit(new Threads2RemoveOrder());
    	Future<Integer> retVal3 = service.submit(new Threads2RemoveOrder());
    	Future<Integer> retVal4 = service.submit(new Threads2RemoveOrder());
    	Future<Integer> retVal5 = service.submit(new Threads2RemoveOrder());
    	
    	long timeout = 1000;
    	
        assertEquals(0, retVal1.get(timeout, TimeUnit.MILLISECONDS).intValue());
        assertEquals(0, retVal2.get(timeout, TimeUnit.MILLISECONDS).intValue());
        assertEquals(0, retVal3.get(timeout, TimeUnit.MILLISECONDS).intValue());
        assertEquals(0, retVal4.get(timeout, TimeUnit.MILLISECONDS).intValue());
        assertEquals(0, retVal5.get(timeout, TimeUnit.MILLISECONDS).intValue());
    }

Select allOpen in new window

if this map always contact the database to load data then you don't have to use that in this case... You need to have the same data in another map which has nothing to do with databases but always you must synchronize those data in your map to those in your ObjectMap  and only if  it is necessary, you can simply remove it from the ObjectMap

How about this:
consider you have defined a ConcurretHashMap and each time you keep it up-to-date with ObjectMap

 @Override
    public void removeModelsForOrder(final String orderId) {
        
                List<FModel> modelList = getModels(orderId);
                
                ObjectMap modelsMap = getModelsMap();
 		 
                for (FModel model : modelList) {
                    removelock.lock();
                    try {
   		      if(concurrentMap.containsKey(model.getCorrelationId()) {	
                          ModelsMap.remove(model.getCorrelationId());
			  concurrentMap.remove(model.getCorrelationId());
                       }
                    }finally {
                      removelocl.unlock
                    }


                }
    }

Select allOpen in new window

Thanks.