Shared Locks in Multi Threading - Java

Question

I am new to multi threading so would appreciate anyone's advice on whether I have done the right thing.

I need to do the following:

Pre process an incoming message and push it on << a data structure. I have used a HashMap with the UUID of the message as the key and the message itself as the value>> for random access. Any further requests for the status of this message, would send in the UUID which can then be retrieved from the Map for random access.

At the same time I need another thread, which would sequentially access the values(or the messages) in the order of insertion, and process them

So here's the first bit for manipulating the Map in a class Store

public class Store {

static ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
static Map<String, MessageFileRead> fileGeneratorMap = Collections.synchronizedMap(new LinkedHashMap());


public static boolean insertRecord(MessageFileRead messageFileRead) {
    boolean processed = false;
    lock.writeLock().lock();
    try {

        if (!fileGeneratorMap.containsKey(messageFileRead.uuid)) {
            fileGeneratorMap.put(messageFileRead.uuid, messageFileRead);
            processed = true;
        }
    } finally {
        lock.writeLock().unlock();
    }
    return processed;
}

public static boolean updateRecord(String uuid, Status status) {
    boolean processed = false;
    lock.writeLock().lock();
    try {
        if (fileGeneratorMap.containsKey(uuid)) {
            fileGeneratorMap.get(uuid).status = status;
            processed = true;
        }
    } finally {
        lock.writeLock().unlock();
    }
    return processed;
}

public static boolean deleteRecord(String uuid) {
    boolean processed = false;
    lock.writeLock().lock();
    try {
        for (final Iterator<Map.Entry<String, MessageFileRead>> it = fileGeneratorMap.entrySet().iterator(); it.hasNext(); ) {
            Map.Entry<String, MessageFileRead> entry = it.next();
            if (entry.getKey().equals(uuid)) {
                it.remove();
                break;
            }
        }
    } finally {
        lock.writeLock().unlock();
    }
    return processed;
}

This is the code for Sequential access into the map using the Iterator which sits in another thread in another class in the same package. This is within a method which is part of the framework code to invoke a new worker Thread.

    while (!stopped) {
        try {
            Store.lock.writeLock().lock();
            for (final Iterator<Map.Entry<String, MessageFileRead>> it = Store.fileGeneratorMap.entrySet().iterator(); it.hasNext(); ) {
                Map.Entry<String, MessageFileRead> entry = it.next();
                MessageRead message = entry.getValue();

                if (message.status != COMPLETED) {
                    JSONObject response = DbService.process(message);
                    // And the above process method internally calls the updateRecord with a status of COMPLETED so that this message is marked as COMPLETED processing and is not picked up in the next run.
                }
            }
        } finally {
            Store.lock.writeLock().unlock();
        }
    }

I haven't encountered any issues so far, but its still initial stages and I want to know if I have done the right thing, so any advice/suggestions are welcome.

Your `deleteRecord` code doesn't need to use an iterative to remove by key. If you're just looking for a review, I'd suggest migrating to codereview.stackexchange.com — Krease, Jun 06 '16 at 15:59
If you are only reading then you should use a `readLock` in your sequential access process.They play nice with `writeLock` - see [ReentrantReadWriteLock,what's the difference between ReadLock and WriteLock](http://stackoverflow.com/a/18354623/823393). — OldCurmudgeon, Jun 06 '16 at 16:06
Thanks Chris, sorry for being naive. But does it mean I just copy paste it again or is there any other way to do this? — Vishal Jumani, Jun 06 '16 at 16:16
you can use fileGeneratorMap.remove(key) instead of iterating to find the correct key and then remove. https://docs.oracle.com/javase/7/docs/api/java/util/Map.html#remove(java.lang.Object) — Vijay, Jun 06 '16 at 16:23
Are you accessing fileGeneratorMap from outside the Store class, or could that variable be made private? — Warren Dew, Jun 06 '16 at 17:15
If you are using a `Lock`, you should do that consistently. In the code you have posted, you appear to do so. Therefore, there is no need to use a `synchronizedMap` and you shouldn’t use it as it’s a disguise. A reader seeing something like `synchronized…` will not look for a guarding `Lock`. Besides that, these `boolean processed` local variables are nonsense. At the place of the `return` statement, it’s impossible for them to be `false`, so they suggest a fundamental misunderstanding about the program flow… — Holger, Jun 06 '16 at 19:00
If you modify your record, do you expect your record to be treated in the same order/position as when it has been added? — Nicolas Filotto, Jun 06 '16 at 19:03
@WarrenDew: Yes the fileGeneratorMap is being used outside of the Store class. So basically if you look at the second bit of code above with the worker thread I do access it using Store.fileGeneratorMap — Vishal Jumani, Jun 07 '16 at 08:30
@Holger: Thank you for that. Like I said I was new to multi threading and I do appreciate any comments. I shall remove the syncronizedMap and try again. — Vishal Jumani, Jun 07 '16 at 08:32
@NicolasFilotto: As long as it doesnt affect the ordering of the incoming messages, it doesn't matter. I need to implement a FIFO for the incoming requests, so once they are processed the ordering doesnt matter. — Vishal Jumani, Jun 07 '16 at 08:36
Maybe [this book](http://www.amazon.com/dp/0321349601) is the right one for you. — Holger, Jun 07 '16 at 09:21

score 0 · Answer 1 · answered Jun 06 '16 at 16:01

0

I would question the use of Collections.synchronizedMap while ConcurrentHashMap is available, and you can avoid locking the map explicitly.

https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHashMap.html

answered Jun 06 '16 at 16:01

Vijay

542
4
15

1

Thanks Vijay, I read somewhere that it wouldn't give me sequential access to the elements added. I need to retrieve them in the order in which I inserted them as well. Hence the approach. – Vishal Jumani Jun 06 '16 at 16:12
1

yes, ConcurrentHashMap will not give you sequential access. – Vijay Jun 06 '16 at 16:17

score 0 · Answer 2 · answered Jun 06 '16 at 18:38

The code looks fine. Though, if you don't foresee needing read-only access, you can simplify it by using synchronized(lock) {...} instead of writeLock. Example:

public static boolean deleteRecord(String uuid) {
    boolean processed = false;
    synchronized(lock) {
        for (final Iterator<Map.Entry<String, MessageFileRead>> it = fileGeneratorMap.entrySet().iterator(); it.hasNext(); ) {
            Map.Entry<String, MessageFileRead> entry = it.next();
            if (entry.getKey().equals(uuid)) {
                it.remove();
                break;
            }
        }
    } 
    return processed;
}

Shared Locks in Multi Threading - Java

2 Answers2