Java Synchronizing for Integer values. Cleaning up map that tracks synchronize objects

Question

I have a portion of code that needs to be thread safe. It is code that loads and modifies an object from the database based on its ID. I want to avoid synchronizing on just the Integer ID variable, so I am attempting to implement the solution offered in this thread: https://stackoverflow.com/a/659939/3561422

However, I am not creating a cache so I have nothing in place to manage the objects added to the map. I want to avoid a memory leak situation. I have looked into using a WeakHashMap, but that is apparently not thread-safe. I have created a map as follows, but the GC does not appear to be cleaning up the references I create.

 private static Map<Integer, Object> locks = Collections.synchronizedMap(new WeakHashMap<Integer, Object>())

Is there something I am missing here that would make this solution work? Is WeakHashMap actually safe for me to use here?

Some example code:

    public static void mainMethod(Integer id){
        Object lockObject = getMapObject(id);
        synchronized (lockObject) {
            Object dbObj = loadDBObjFromDB(id);
            //Do pre execution checks
            if (dbObj.isInUse()) {
                //fail here
            }
            dbObj.setAsInUseAndCommitToDB();
        }
        actOnObj(dbObj);
}

private static Object getMapObject(final Integer id) {
    locks.putIfAbsent(id, new Object);
    return locks.get(id);
}

Basically, I need to mark something in the database as in use. If another thread comes in and wants to do something on it, it needs to see if it is already in use. If it is, I fail and give the user feedback. I need to lock around loading, checking if it is in use, and updating that it is in use. I would like to use the map to avoid locking on an Integer object

We need to see more of what you are doing. Please provide an example that demonstrates the intent (even if just pseudocode). — Chris Shain, Nov 08 '18 at 17:41
How can this even work? `private static WeakHashMap locks = Collections.synchronizedMap(new WeakHashMap())` — tsolakp, Nov 08 '18 at 17:53
Is your intent to call `mainMethod` with multiple threads, potentially with the same `id`? And the idea is that if one thread is operating on, say, `id == 6`, another thread calling `mainMethod(6)` would return immediately? — Chris Shain, Nov 08 '18 at 17:55
@ChrisShain Pretty much. The other caveat I'd add is that when an object is marked as "in use" there are other, separate operations that are unavailable. (This is why I store it in the db) — scott13, Nov 08 '18 at 18:50
You have a database - why not just use it for locking? Are there performance considerations here? What are your throughput/latency constraints? — Boris the Spider, Nov 08 '18 at 18:57

score 0 · Answer 1 · answered Nov 08 '18 at 18:06

0

I think that what you are looking for here is an implementation of ConcurrentHashSet (there are several out there, I'd look at Guava's). It is the same idea as a ConcurrentHashMap without needing a value (in fact, Guava's is based on ConcurrentHashSet per the documentation). Another alternative is simply doing what you are doing, and only using a single, statically created object as the value (since the value here is irrelevant):

private static final MAP_VALUE = new Object();
private static Object getMapObject(final Integer id) {
    locks.putIfAbsent(id, MAP_VALUE);
    return locks.get(id);
}

For the map, just make it a regular ConcurrentHashMap. No need to worry about weak references or weak hashmaps.

answered Nov 08 '18 at 18:06

Chris Shain

50,833
6
93
125

I wouldn't recommend Guava when this is already in the JDK - `Collections.newSetFromMap(new ConcurrentHashMap<>())`. – Boris the Spider Nov 08 '18 at 18:16
@BoristheSpider I don't know that that method will create a *concurrent set*, just because the underlying map is concurrent. – Chris Shain Nov 08 '18 at 18:33
Docs are very clear. "_The resulting set displays the same ordering, concurrency, and performance characteristics as the backing map_" – Boris the Spider Nov 08 '18 at 18:35
Wouldn't this mean it would lock independent of the supplied ID since the same object is simply reused? What is the point of the map at this point? Also, if I do not use weak references, aren't I subjecting myself to the possibility of a memory leak as the map would grow unbounded? – scott13 Nov 08 '18 at 18:48
Locking the ID is what you want, isn't it? If the ID is locked, then mainMethod will bail out for all concurrent access. You don't care about the value in the map, thus the suggestion to use a set instead. – Chris Shain Nov 08 '18 at 19:08
I was only using the map because I was under the impression that locking on an Integer isn't really advised. So, I was mapping the integer value to a Java object and then using that object to lock. – scott13 Nov 08 '18 at 19:48
Locking on an Integer isn't advised... What you are doing here is using an alternative to actual locking (as in, the concurrency concept), and instead using a data structure to indicate which IDs are in use. I used the word locking in my previous comment as a convenience. – Chris Shain Nov 08 '18 at 20:06

Java Synchronizing for Integer values. Cleaning up map that tracks synchronize objects

1 Answers1