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.

Is there any reason you are not using Thread.interrupt() as this is supported by the underlying libraries? — Peter Lawrey, Nov 19 '10 at 17:51

score 7 · Accepted Answer · answered Nov 19 '10 at 16:07

7

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.

answered Nov 19 '10 at 16:07

Jon Skeet

1,421,763
867
9,128
9,194

It's fine, but it's questionable as to whether setting the thread reference to null is a good idea. – Adamski Nov 19 '10 at 16:08
1

@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 :) – Jon Skeet Nov 19 '10 at 16:10
Agreed - In fact IntelliJ warns me in this situation about synchronizing on a non-final variable. – Adamski Nov 21 '10 at 22:15

score 3 · Answer 2 · answered Nov 19 '10 at 16:08

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.

score 1 · Answer 3 · answered Nov 19 '10 at 17:57

1

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.

answered Nov 19 '10 at 17:57

Michael Donohue

11,776
5
31
44

3

@EJP more than evaluated - an expression that evaluates to null does not cause a NullPointerException. – Michael Donohue Nov 20 '10 at 07:52

score 0 · Answer 4 · answered Nov 19 '10 at 17:15

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.

score 0 · Answer 5 · answered Nov 19 '10 at 17:53

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.

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

5 Answers5