Can you lock on a local object in Java?

Question

I have this snippet of code

private Templates retrieveFromCache(String name) {
        TemplatesWrapper t = xlCache.get(name);
        synchronized(t){
            if (!t.isValid()) {
                xlCache.remove(name);
                return null;
            }
        }
        return t.getTemplate();
    }

xlCache is a ConcurrentHashMap; my reason for synchronizing on t is that 2 threads could interleave where by the time Thread 1 verifies the predicate Thread 2 has already removed the object from the map and then a NullPointerException would be thrown. Is my assumption correct as I know concurrency is one of the more difficult things to reason about. And then to my original question, can I lock on t even if it's local?

And this is private method as well which gets called from a public method, does it make a diff?

EDIT: MY original premise that a NullPointerException is thrown was incorrect as remove() returns boolean making synchronization moot; however, my question was of locking on a local object was answered.

Answer 1

ConcurrentHashMap (and Map/ConcurrentMap in general) won't throw an exception if the specified key doesn't exist. That's why the remove method returns a boolean, to indicate whether or not anything was actually removed.

But yes, you can lock on the local variable. After all, you're really locking via a reference (and the monitor associated with the referenced object), not a variable - and the other concurrently running method would have the same reference.

Answer 2

Yep, that will work just fine.

You can synchronize on any object in java so you code will work and will be thread safe.

Appart from the fact that you aren't checking for t being null. I'm guessing you have just missed that out of your code example?

Answer 3

If remove(null) would call a null pointer exception, this seems reasonable. If you don't expect collision to be a regular problem, you could also implement a possibly-faster version of the code, by just wrapping a try/catch around that instead of a synchronized.

In either case, I'd add a comment there to explain why you did what you did, so that a month from now, it still makes sense.

Answer 4

You can lock on any object you want. However, in your case, it looks like you could solve it in a clearer and safer.

Synchronization should be as localized as possible. Since you're getting the TemplatesWrapper from some unknown location, its possible that anyone can synchronize on it making it really hard to control the concurrency. It should also be as obvious as possible just by looking at the code why something gets locked.

It would be better off to put the synchronization inside xlCache with something like removeIfInvalid()

Answer 5

A better way to do this would be to use the 2 arg remove method from ConcurrentMap (assuming t has a reasonable equals implementation). then you don't need any synchronization:

private Templates retrieveFromCache(String name) {
    TemplatesWrapper t = xlCache.get(name);

    if (!t.isValid()) {
        xlCache.remove(name, t);
        return null;
    }
    return t.getTemplate();
}