Is this ok? Synchronized( thread ), then thread=null in the synch block

Question

I see this:

// thread is a member of this class

synchronized( this.thread )
{
  this.thread.running = false;
  this.thread.notifyAll(); // Wake up anything that was .waiting() on
  // the thread
  this.thread = null;  // kill this thread reference.
  // can you do that in a synchronized block?
}

Is it ok to set the thread=null while still keeping a lock on it?

I found this nugget in a bit of BB code.

Sprag

Is there any reason you are not using Thread.interrupt() as this is supported by the underlying libraries? – Stellite

Answer 1

Yes, that's fine. The synchronized statement will take a copy of the reference that it's locking on, and use the copy to work out what to unlock at the end.

Section 14.19 of the Java Language Specification isn't actually clear about this, but it does state that the expression is evaluated at the start - and doesn't mention evaluating it again later on.

Gametophyte

It's fine, but it's questionable as to whether setting the thread reference to null is a good idea. – Yuki

@Adamski: I tend to take a reasonably strong view on what I synchronize on in the first place to be honest - I thought I'd avoid going into that :) – Gametophyte

Agreed - In fact IntelliJ warns me in this situation about synchronizing on a non-final variable. – Yuki

Answer 2

There's a difference:

synchronized( this.thread )

You are synchronizing on the Object the field this.thread points to

this.thread = null;

You are reassigning the field. You are not doing anything with the object you referenced above, so the lock is still valid.

Uvula

Answer 3

The synchronized expression is dereferenced on entry, so any later users of this lock will get a NullPointerException. You can work around that by putting a null check ahead of the synchronized block, but then you've introduced a race condition.

Olga

@EJP more than evaluated - an expression that evaluates to null does not cause a NullPointerException. – Olga

Answer 4

You can do it, but it's almost certain that the code is wrong for whatever it is trying to achieve. Post the entire code, and I guarantee that it is obviously the programmer doesn't understand concurrency.

Do not reassign a variable which is used for synchronization.

Upmost

Answer 5

You only have a problem if you also have a block which assigns a new value to thread. In that case you have a race condition as the two blocks don't lock on the same object, but will update the same field and it will be random as to which block assigns the value last.

Stellite