synchronization issue

[Parvinder]

public Object lookup(String jndiName) throws NamingException {
Object obj = homeInterfaces.get(jndiName);
if (obj == null)
{
synchronized (lock)
{
if ((obj = homeInterfaces.get(jndiName)) == null)
{
Context ctx = this.getInitialContext();
obj = ctx.lookup(jndiName);
homeInterfaces.put(jndiName,obj);
}
}
}
return obj;
}




The writer wanted to synchronize the entire block of the code.The
"lock" object is the object which is a member of the class. Getting a
lock on the object "lock" achieves the motive of synchronizing the
entire block but in my opinion it leaves a window to write some other
method which will can modify the homeInterfaces member (hash map) and
thus have a concurrency here.

According to me the code should be modified to
synchronized(homeInterfaces) which will essentially synchronize the
entire block of code as well as it will not leave any window to modify
the homeInterfaces hash map through some other function.
Any particular comments ?

~Parvinder

 

[Steve Horsley]

public Object lookup(String jndiName) throws NamingException {
Object obj = homeInterfaces.get(jndiName);
if (obj == null)
{
synchronized (lock)
{
if ((obj = homeInterfaces.get(jndiName)) == null)
{
Context ctx = this.getInitialContext();
obj = ctx.lookup(jndiName);
homeInterfaces.put(jndiName,obj);
}
}
}
return obj;
}




The writer wanted to synchronize the entire block of the code.The
"lock" object is the object which is a member of the class. Getting a
lock on the object "lock" achieves the motive of synchronizing the
entire block but in my opinion it leaves a window to write some other
method which will can modify the homeInterfaces member (hash map) and
thus have a concurrency here.

According to me the code should be modified to
synchronized(homeInterfaces) which will essentially synchronize the
entire block of code as well as it will not leave any window to modify
the homeInterfaces hash map through some other function.
Any particular comments ?

~Parvinder

It doesn't really matter too much which object they lock on, provided
that everyone (all methods) do the same. It is probably easier to lock
on the homeInterfaces object, especially if homeInterfaces is referenced
from more than one other object - you don't want to end up with object A
using its own lock and object B using its own lock, and then both trying
to access homeInterfaces at the same time.

Another point - there is nothing in the above code to prevent one thread
from calling homeInterfaces.get() concurrently with another thread (inside
the sync block) calliong homeInterfaces.put(). This is potentially not
thread-safe at all - the un-synchronized get() could read a partially
modified structure. You must also perform the get() from inside a
synchronized block to be safe.

Steve

 

[John C. Bollinger]

public Object lookup(String jndiName) throws NamingException {
Object obj = homeInterfaces.get(jndiName);
if (obj == null)
{
synchronized (lock)
{
if ((obj = homeInterfaces.get(jndiName)) == null)
{
Context ctx = this.getInitialContext();
obj = ctx.lookup(jndiName);
homeInterfaces.put(jndiName,obj);
}
}
}
return obj;
}




The writer wanted to synchronize the entire block of the code.The
"lock" object is the object which is a member of the class. Getting a
lock on the object "lock" achieves the motive of synchronizing the
entire block but in my opinion it leaves a window to write some other
method which will can modify the homeInterfaces member (hash map) and
thus have a concurrency here.

You are correct.

According to me the code should be modified to
synchronized(homeInterfaces) which will essentially synchronize the
entire block of code as well as it will not leave any window to modify
the homeInterfaces hash map through some other function.
Any particular comments ?

You are incorrect. Correct synchronization requires that all operations
that read or write the protected shared state synchronize on the same
object, but _which_ object they synchronize on doesn't make a bit of
difference. In particular, synchronizing on an object that is part (or
all) of the protected shared state is not any more special than
synchronizing on any other object, and does not solve the problem you
remarked upon.

What does make a difference is when the same synchronization object is
used for unrelated purposes. This can cause unnecessary blocking and
may contribute to deadlock. It is a good reason to use of an internal
synchronization object as the code fragment you show does, as opposed to
using a synchronized method or synchronizing on "this". This is where
choosing part of the shared state to synchronize on makes some sense: it
both documents what is supposed to be protected and reduces the
likelihood of unrelated code accidentally selecting the same
synchronization object. That advantage is greatly diminished when the
shared state is spread over several objects.

Somewhat related, I observe that the code uses a variation on the
"Double Checked Locking" pattern. You will find voluminous material, in
the archives of this newsgroup and elsewhere, describing how and why
double checked locking does not ensure thread safety in Java. With that
being the case, the extra complexity is simply not worth it, so put the
entire method body inside the synchronized block and remove the
duplicate "if" and homeInterfaces.get().


John Bollinger
(e-mail address removed)