Tuesday, April 16, 2013

Code Review for a String Lock Map

I recently needed a mechanism to lock on given Strings in a particular piece of code. As maybe a weak example, say I was doing disk file updates to files each with unique filenames (i.e. read, update, write-back). I could have synchronized the methods that modified the files, but I preferred to not lock at that level - in other words, only one file update could occur at a time then.

I'd prefer to simply lock on that file - or more specifically, that filename. Its a dubious proposition to lock on String objects in Java as different objects can represent the same set of characters. You can intern() all your strings (which makes sure the objects you have are the "one" system-wide representation of that String) but that is rather global. I wanted to do things locally - where I can control the scoping of the lock.

(The filename example is just an example, I've needed this structure in many different scenarios - so don't look to just solve that problem).

I might have missed it, but I didn't find any facilities in java.util.concurrent to help me (and to be fair, I haven't kept up with the happenings there. Long story short, I wrote my own.

Of course the best advice when writing concurrent locking datastructures is - DON'T.

But I did. And I probably plan on using it eventually, so I put it here for review. I'll update it as comments come in (so if you see a comment below that makes little sense it's probably because I heeded it and changed the code).

The use case I want:

public static final StringLock stringLock = new StringLock();

...

try {
     stringLock.lock(filename);

     // load disk file
     // change its contents a bit
     // rewrite it
     
} finally {
     stringLock.unlock(filename);
}

So.. find below the code I've come with so far. Its a bit heavy under contention but the normal case (no contention) it syncs 3 times (writes on ConcurrentHashMap being a sync). I could also use Cliff Click's NonBlockingHashMap to remove two of the syncs but and I'd be happy to be convinced that'd be superior for some reason.

public class StringLock {

 private final ConcurrentHashMap lockedStrings = new ConcurrentHashMap();

 public void lock(String s) { 
   Object lockObject = null; 
   while (true) { 
     lockObject = lockedStrings.get(s); 

     if (lockObject != null) { 
       synchronized (lockObject) { 
          Object lockObject2 = lockedStrings.get(s); 
          if (lockObject2 == lockObject) { 
            try { lockObject2.wait(); } catch (Exception e) { } 
          } 
        } 
     } else { 
        lockObject = new Object(); 
        Object lockObject1 = lockedStrings.putIfAbsent(s, lockObject); 
        if (lockObject1 == null) { 
          break; 
        } 
     } 
   } 
 } 

 public void unlock(String s) { 
    Object lockObject = lockedStrings.get(s); 
    synchronized (lockObject) { 
      lockedStrings.remove(s); 
      lockObject.notifyAll(); 
    } 
  }
}