The code below is completely fine if running on single-thread 
environment. But it could break if running on multi-thread environment. 
I have added my analysis as comments in the code to show my 
understanding why it is not thread safe. My real code, with a pattern 
like the code shown here, indeed breaks once in a while with Null 
Pointer Exception in multi thread run. Do you think my analysis is 
correct? Can you help me on making this code thread safer?

Thank you very much.

public class MyClass
{
	private Person tim = new Person("Tim");
	private Person tom = new Person("Tom");

	public void doThis
	{
		this.checkSomething();

		if(tim != null) //thread "A" is running "doThis()" and tim is not null 
at this moment
		{
                         ... //other code
			tim.doA(); //tim is NULL all of sudden, and NPE is thrown. I think it 
is due to another thread running checkSomething()
			tim.doB();
			tim.doC();
	
		}

	}
	

	public void checkSomething() //thread "B" is running checkSomething()
	{
		if(tim.isAbsent())
		{
			tim = null;
		}

		if(tom.isAbsent())
		{
			tom = null;
		}

	}

}

www wrote:
> The code below is completely fine if running on single-thread 
> environment. But it could break if running on multi-thread environment. 
> I have added my analysis as comments in the code to show my 
> understanding why it is not thread safe. My real code, with a pattern 
> like the code shown here, indeed breaks once in a while with Null 
> Pointer Exception in multi thread run. Do you think my analysis is 
> correct? Can you help me on making this code thread safer?

The simplest fix is to just add "synchronized" to the method 
declarations.  This will cause each method to acquire the instance's 
monitor during their execution, ensuring that only one thread can be 
executing either method at a time.

Another alternative would be to use the "synchronized" statement to the 
code inside each method.  Assuming the "isAbsent()" method isn't 
affected by the "other code" or the "doA()", "doB()", or "doC()" 
methods, that could look like this:

public class MyClass
{
   private Person tim = new Person("Tim");
   private Person tom = new Person("Tom");
   private final Object lock = new Object();

   public void doThis
   {
     this.checkSomething();

     synchronized (lock)
     {
       if(tim != null) //thread "A" is running "doThis()" and tim is not 
null at this moment
       {
                   ... //other code
         tim.doA(); //tim is NULL all of sudden, and NPE is thrown. I 
think it is due to another thread running checkSomething()
         tim.doB();
         tim.doC();
       }
     }
   }

   public void checkSomething() //thread "B" is running checkSomething()
   {
     if(tim.isAbsent())
     {
       synchronized (lock)
       {
         tim = null;
       }
     }

     if(tom.isAbsent())
     {
       tom = null;
     }
   }
}

That would have a similar effect, but would only synchronize in thread 
"B" when the instance field actually has to be changed.  It would still 
ensure that the field cannot change between the time thread "A" checks 
for the "null" value and when it tries to use it, but has the potential 
for being more efficient because in theory, less contention would occur 
("contention" being when two different threads actually do need to 
acquire the same lock at the same time).

Note the use of a dedicated object reference for synchronization.  In 
general, it is poor practice to use the "this" reference for 
synchronization, though that's what declaring a method as "synchronized" 
does.  It's not the end of the world to use "this", but doing so "leaks" 
some of your implementation, and introduces the possibility of some 
other code not related to your own taking the same monitor, increasing 
contention as well as the complexity of the locking scenarios.

Those are the simplest techniques available in Java, and for the example 
you've given are probably the most appropriate anyway.  As you learn 
more about concurrent programming, you'll probably want to look at the 
other synchronization features in Java, found mainly in the 
java.util.concurrent package.

Pete

www <www@nospam.com> writes:

> The code below is completely fine if running on single-thread
> environment. But it could break if running on multi-thread
> environment. I have added my analysis as comments in the code to show
> my understanding why it is not thread safe. My real code, with a
> pattern like the code shown here, indeed breaks once in a while with
> Null Pointer Exception in multi thread run. Do you think my analysis
> is correct? Can you help me on making this code thread safer?
>
> Thank you very much.
>
> public class MyClass
> {
>   private Person tim = new Person("Tim");
>   private Person tom = new Person("Tom");
>
>   public void doThis
>   {
>       this.checkSomething();
>
>       if(tim != null) //thread "A" is running "doThis()" and
> tim is not null at this moment
>       {
>                         ... //other code
>           tim.doA(); //tim is NULL all of sudden, and
> NPE is thrown. I think it is due to another thread running
> checkSomething()
>           tim.doB();
>           tim.doC();
>   
>       }
>
>   }
>   
>
>   public void checkSomething() //thread "B" is running checkSomething()
>   {
>       if(tim.isAbsent())
>       {
>           tim = null;
>       }
>
>       if(tom.isAbsent())
>       {
>           tom = null;
>       }
>
>   }
>
> }

The simplest and least error prone approach is to declare both methods
as synchronized, e.g.

    synchronized public void doThis()

    synchronized public void checkSomething()

I wouldn't consider any other approach else unless there are known,
specific performance requirements that this does not meet.

And making a class "thread safer" is not a goal you should aim for.
If a class is not completely thread safe and it's used in a threaded
environment, threading errors *will* eventually occur.  You probably
won't see them, but your users will.

-- 
Jim Janney

Jim Janney wrote:

> 
> The simplest and least error prone approach is to declare both methods
> as synchronized, e.g.
> 
>     synchronized public void doThis()
> 
>     synchronized public void checkSomething()
> 

This is equal to running my program in single thread. Am I correct?

Jim Janney wrote:

> 
> The simplest and least error prone approach is to declare both methods
> as synchronized, e.g.
> 
>     synchronized public void doThis()
> 
>     synchronized public void checkSomething()
> 

This is equal to running my program in single thread. Am I correct?

More specifically, how can I make sure that checkSomething() is finished 
at the top of doThis()?

www wrote:

> This is equal to running my program in single thread. Am I correct?


Not really, no.  The synchronized keyword provides different semantics 
than just "single threaded behavior," whatever that might mean.


> More specifically, how can I make sure that checkSomething() is finished 
> at the top of doThis()?


Using synchronized as shown will do that.  This is called MUTEX ("mutual 
exclusion") and is one of the semantics "synchronized" provides, but not 
the only one.

Hint: Google MUTEX.

Jim Janney wrote:
>> The simplest and least error prone approach is to declare both methods
>> as synchronized, e.g.
>
>> =A0 =A0 synchronized public void doThis()
>
>> =A0 =A0 synchronized public void checkSomething()
>

www wrote:
> This is equal to running my program in single thread. Am I correct?
>

What do you mean by "equal"?  It's not the same as running in a single
thread, no, and nor is it always going to be enough for every
concurrent situation.  Whether it's as safe as running in a single
thread depends on the details of the situation, but for this simple
situation it seems to be.  Pete alluded to risks with this approach
upthread.

> More specifically, how can I make sure that checkSomething() is finished
> at the top of doThis()?
>

With the simple approach shown, you can be sure that
'checkSomething()' and 'doThis()' are not executing simultaneously
within the same instance.

You need to learn to think in terms of concurrent programming, which
takes a bit of time.  One trick is to think in terms of "critical
sections".  Those are parts of code that access the same data, so in a
concurrent environment they can interfere with each other unless
you're careful.  To be careful, you apply 'synchronized' or 'volatile'
or a raft of other techniques.  A critical section is a shortest piece
of code that operates on the shared data as a unitary operation.  The
collection of critical sections is the collection of pieces of code
that have to honor the same synchronization mechanism.

By the way, this is for read or write.  You cannot synchronize on,
say, just the write sections and expect unsynchronize read sections to
work correctly.

Another trick is to master the Java concept of /happens-before/.  This
is the set of rules about what changes threads can see that were made
by other threads.  It's a good trick because once you get the concept,
you can reason about whether changes will be visible or conflict
between threads.

--
Lew

www wrote:
> The code below is completely fine if running on single-thread 
> environment. But it could break if running on multi-thread environment. 

> public class MyClass
> {
>     private Person tim = new Person("Tim");
>     private Person tom = new Person("Tom");
> 


Here's a weird question for this list at large.  Are the above lines 
even thread safe?  Objects are guaranteed to be correctly constructed, 
but these objects might be mutable.  And the fields tim and tom are not 
final, so they aren't guaranteed to be published correctly either.

So one thread has to construct this "MyClass" and then some other thread 
might consume it (note that the methods of MyClass are not static, so an 
instance does have to be constructed):

public class Main {
   public static void main( String... args ) {
     MyClass test = new MyClass();
     ...  // now what?
   }
}

So here if one then passed the "test" object to, for example, the EDT 
for use in a GUI, one still needs synchronization/happens-before to make 
the "test" object correctly visible to the EDT, or any other thread 
besides main, yes?

I could look this up but I'm feeling a bit lazy right now, and I thought 
the OP could use some discussion of other thread-safe semantics besides 
mutex, which is the only one he seems to understand right now.

www <www@nospam.com> writes:

> Jim Janney wrote:
>
>>
>> The simplest and least error prone approach is to declare both methods
>> as synchronized, e.g.
>>
>>     synchronized public void doThis()
>>
>>     synchronized public void checkSomething()
>>
>
> This is equal to running my program in single thread. Am I correct?
>
> More specifically, how can I make sure that checkSomething() is
> finished at the top of doThis()?

Assume that there is an instance of MyClass and that some thread is
executing in either doThis() or checkSomething() on that instance.
The synchronized declaration accomplishes two things:

1) If any other thread tries to execute doThis() or checkSomething()
   on the same instance, it will block until the first thread exits
   the synchronized method it's in, either by returning normally or by
   throwing an exception.

2) By the time the blocked thread is allowed to enter, any changes to
   the object made by the previous thread will be visible in memory to
   the second thread.

In other words, it guarantees both exclusion and memory consistency.

-- 
Jim Janney

markspace wrote:
> www wrote:
>> The code below is completely fine if running on single-thread 
>> environment. But it could break if running on multi-thread environment. 
> 
>> public class MyClass
>> {
>>     private Person tim = new Person("Tim");
>>     private Person tom = new Person("Tom");
>>
> 
> 
> Here's a weird question for this list at large.  Are the above lines 
> even thread safe?  Objects are guaranteed to be correctly constructed, 
> but these objects might be mutable.  And the fields tim and tom are not 
> final, so they aren't guaranteed to be published correctly either.
> 
> So one thread has to construct this "MyClass" and then some other thread 
> might consume it (note that the methods of MyClass are not static, so an 
> instance does have to be constructed):
> 
> public class Main {
>   public static void main( String... args ) {
>     MyClass test = new MyClass();
>     ...  // now what?
>   }
> }
> 
> So here if one then passed the "test" object to, for example, the EDT 
> for use in a GUI, one still needs synchronization/happens-before to make 
> the "test" object correctly visible to the EDT, or any other thread 
> besides main, yes?

For the EDT example, there are actually two different mechanisms that 
should ensure thread safety.  One of them, I would have to look up to 
make sure is supported in the JVM (but I'm pretty sure it is).  The 
other is inherent in your statement "passed the 'test' object to�the EDT".

The first mechanism has to do with semantics surrounding object 
construction.  In .NET, the specification ensures that whatever happens 
in the constructor "happens before" any other code that uses the 
reference after the constructor completes (.NET doesn't actually use the 
term "happens-before", but it's the same idea).  I would expect Java to 
be the same in this respect.

Note that this is a good reason for not letting the "this" reference 
leak out of the constructor, because doing so would circumvent whatever 
assurances the object construction semantics would normally provide.

The second mechanism has to do with how object references get from one 
thread to another.  In particular, keep in mind that "happens 
before"/"happens after" has a sort of transitive nature to it. 
Everything that happens before something synchronized in one thread is 
visible to everything that happens after the inspection of that 
synchronized something in a different thread.

In the case of the EDT, the typical way an object reference would get 
from another thread to the EDT is via the invokeLater() or 
invokeAndWait() methods.  These methods are inherently synchronized 
themselves; it's a natural consequence of what must happen for the data 
passed to the methods to get to the EDT.

This means that everything that happened in that other thread before the 
call to invokeLater() or invokeAndWait() is necessarily going to be 
visible to the EDT, as long as it only inspects that data as a result of 
whatever Runnable was passed to the invoke�() method.  Likewise if the 
data was passed not via an invoke�() method but rather via, for example, 
a synchronized queue monitored by a Swing timer that inspects it 
periodically.  All bets are off, of course, if the data is transferred 
via some unsynchronized means.

This applies more generally to any cross-thread communication mechanism. 
  Even if not using the EDT, there is likely some synchronized 
data-transfer mechanism that acts as the go-between, allowing that newly 
constructed reference to safely move from one thread to the other.  So 
long as the reference is not inserted into that synchronized mechanism 
until the constructor returns, then the other thread has the full 
guarantee of the object being fully initialized that the original thread 
where the object was created has.

In other words, even if it turns out that I'm mistaken and the JVM 
doesn't provide any specific synchronization guarantees with respect to 
object construction, code that is otherwise thread-safe anyway will 
automatically ensure that the initialization of the object is itself 
thread-safe.

Note also that this second mechanism applies not just to construction, 
but to _any_ mutation that may occur in one thread prior to the object 
being delivered to another thread.

Finally, part of your question brings up the possibility of the "Person" 
object being mutable.  And well, yes.  If those classes are mutable, 
there could yet still be a thread-safety problem if they mutate 
post-construction without synchronization being used between threads 
handling those objects.  But that's not related to the question of the 
initialization of the object.  That would be just the usual "synchronize 
mutations" problem that generally comes up with concurrent code.

Pete

Peter Duniho wrote:

> The first mechanism has to do with semantics surrounding object 
> construction.  In .NET, the specification ensures that whatever happens 
> in the constructor "happens before" any other code that uses the 
> reference after the constructor completes (.NET doesn't actually use the 
> term "happens-before", but it's the same idea).  I would expect Java to 
> be the same in this respect.


I'm pretty sure it's not the same in Java.  The semantics of the 
constructor only guarantee that an object will be visible to the calling 
thread.  All other threads may not see a completely and correctly 
constructed object.  And I'd be surprised if it's any different in C#, 
because that would imply a memory barrier at every object construction, 
although I guess C# could indeed be doing such a thing.


> In the case of the EDT, the typical way an object reference would get 
> from another thread to the EDT is via the invokeLater() or 
> invokeAndWait() methods.  These methods are inherently synchronized 
> themselves; it's a natural consequence of what must happen for the data 
> passed to the methods to get to the EDT.


I agree that the only practical way to pass an object to the EDT 
involves a shared lock.  However, invokeLater() makes no such guarantee, 
and they'd be within their rights to change their implementation of 
invokeLater() and remove any shared locks, thus removing any 
synchronization and any happens-before semantics.  Contrast this with 
the docs of some of the concurrent classes like Executor, which does 
provide an explicit memory consistency guarantee.

Thus, I think the minimum you can get away with is a volatile field:

public class Main {
   public static void main( String... args ) {
     final MyClass test = new MyClass();
     SwingUtilities.invokeLater( new Runnable() {
       private volatile MyClass temp = test;
       public void run() {
          MyClass model = temp;
          ...  // ok to use model now...
       }
     } );
   }
}

> This applies more generally to any cross-thread communication mechanism. 
>  Even if not using the EDT, there is likely some synchronized 
> data-transfer mechanism that acts as the go-between, allowing that newly 
> constructed reference to safely move from one thread to the other.


This is an even scarier assertion, imo.  I don't agree that we should 
rely on what's "likely" to happen in concurrency, unless you only want 
it "likely" that your program will run correctly.

markspace wrote:
> [...]
>> In the case of the EDT, the typical way an object reference would get 
>> from another thread to the EDT is via the invokeLater() or 
>> invokeAndWait() methods.  These methods are inherently synchronized 
>> themselves; it's a natural consequence of what must happen for the 
>> data passed to the methods to get to the EDT.
> 
> 
> I agree that the only practical way to pass an object to the EDT 
> involves a shared lock.  However, invokeLater() makes no such guarantee, 
> and they'd be within their rights to change their implementation of 
> invokeLater() and remove any shared locks, thus removing any 
> synchronization and any happens-before semantics.  

I disagree.  Java concurrency and especially that related to the EDT 
(that is, in light of the fact that EDT-related issues are almost always 
about concurrency) is poorly documented, I will grant.

But it is simply not possible for the invoke methods to not provide the 
necessary synchronization.  Whatever has happened in one thread prior to 
the call to an invoke method _must_ be visible to the code executing as 
a result of that call to the invoke method.  The invoke methods would be 
useless otherwise.

> Contrast this with 
> the docs of some of the concurrent classes like Executor, which does 
> provide an explicit memory consistency guarantee.
> 
> Thus, I think the minimum you can get away with is a volatile field:
> 
> public class Main {
>   public static void main( String... args ) {
>     final MyClass test = new MyClass();
>     SwingUtilities.invokeLater( new Runnable() {
>       private volatile MyClass temp = test;
>       public void run() {
>          MyClass model = temp;
>          ...  // ok to use model now...
>       }
>     } );
>   }
> }

If what you say were true, then you could not even count on the Runnable 
instance that you created to be valid during the invoked execution, 
since that's something that happened before the call to invokeLater(), 
but which wasn't synchronized.  The "temp" field itself would be safe, 
but nothing else in the Runnable instance would be (including, for 
example, a v-table as might be used to dispatch the call to run()).

>> This applies more generally to any cross-thread communication 
>> mechanism.  Even if not using the EDT, there is likely some 
>> synchronized data-transfer mechanism that acts as the go-between, 
>> allowing that newly constructed reference to safely move from one 
>> thread to the other.
> 
> This is an even scarier assertion, imo.  I don't agree that we should 
> rely on what's "likely" to happen in concurrency, unless you only want 
> it "likely" that your program will run correctly.

You are misreading my statement.  It's not about relying on "what's 
likely".  It's that it is "likely" there is a synchronization mechanism 
in place, and if so one can rely on it.

You can certainly move data from one thread to another unsafely.  For 
example, through use of a non-volatile field.  And yes, even if the data 
managed to get from one thread to the other via that field, there would 
be no guarantees about other data that may have been updated in one 
thread before the data in question, being visible in the other thread 
even though the data in question is visible.

But, assuming the data itself was moved safely (e.g. through use of a 
volatile field), then all of the things that happened in one thread 
before that data was moved from that thread will then be visible in 
another thread after the data is moved to that thread.  And since for 
the code to be thread-safe, the data _must_ be moved safely, it is 
"likely" that the data was moved safely (inasmuch as one hopes it is 
"likely" that the code is thread-safe).

The likeliness depends of course on how much you trust the person who 
wrote the code moving the data.  But for mechanisms found in the JDK, I 
find the level of likelihood quite high.  For a random Java programmer I 
stumble across on the street, not so much.

Pete

Peter Duniho wrote:
> [...]
>> Thus, I think the minimum you can get away with is a volatile field:
>>
>> public class Main {
>>   public static void main( String... args ) {
>>     final MyClass test = new MyClass();
>>     SwingUtilities.invokeLater( new Runnable() {
>>       private volatile MyClass temp = test;
>>       public void run() {
>>          MyClass model = temp;
>>          ...  // ok to use model now...
>>       }
>>     } );
>>   }
>> }
> 
> If what you say were true, then you could not even count on the Runnable 
> instance that you created to be valid during the invoked execution, 
> since that's something that happened before the call to invokeLater(), 
> but which wasn't synchronized.  The "temp" field itself would be safe, 
> but nothing else in the Runnable instance would be (including, for 
> example, a v-table as might be used to dispatch the call to run()).

Sorry, just to be clear: I overstate when I use the word "nothing". 
But, only those things that occurred prior to the initialization of the 
"volatile" field would be guaranteed to be visible in the EDT.  Other 
initialization of the object would not be.  I would guess that things 
like a v-table would be initialized before the field, but since you 
can't reference "this" in an initializer, even that may be open to question.

Pete

On 7/19/2010 9:33 AM, www wrote:
> The code below is completely fine if running on single-thread
> environment. But it could break if running on multi-thread environment.
> I have added my analysis as comments in the code to show my
> understanding why it is not thread safe. My real code, with a pattern
> like the code shown here, indeed breaks once in a while with Null
> Pointer Exception in multi thread run. Do you think my analysis is
> correct? Can you help me on making this code thread safer?
There are a few ways to handle this, the most obvious approach would be 
to synchronize the whole thing, which in essences prevents the code from 
running in a multi-threaded manor.

I suggest reading the book Java Concurrency in Practice.
<http://www.amazon.com/dp/0321349601?tag=virtuinfinnet-20&camp=14573&creative=327641&linkCode=as1&creativeASIN=0321349601&adid=1Z4QATYV394TGJNXFGXC&>

"This book is a must read for anyone who works in a multi-threaded 
application, which turns out is a lot more frequently than you might 
expect. If you work in a J2EE container, Swing GUI application, or any 
kind of concurrent system, this book will give you the know-how to 
create efficient, scalable, and correct programs.

It helps dispel common misconceptions about concurrency, and showcases 
the improvements to concurrent programming in Java 5."
-- 
Daniel Pitts' Tech Blog: <http://virtualinfinity.net/wordpress/>

On 07/19/2010 02:51 PM, Peter Duniho wrote:
> The first mechanism has to do with semantics surrounding object
> construction. In .NET, the specification ensures that whatever happens
> in the constructor "happens before" any other code that uses the
> reference after the constructor completes (.NET doesn't actually use the
> term "happens-before", but it's the same idea). I would expect Java to
> be the same in this respect.

This is only true for final fields. Regular fields do not have the same 
luxury--hence why it is often preferred to have immutable objects in Java.

-- 
Beware of bugs in the above code; I have only proved it correct, not 
tried it. -- Donald E. Knuth

Peter Duniho wrote:

> If what you say were true, then you could not even count on the Runnable 
> instance that you created to be valid during the invoked execution, 
> since that's something that happened before the call to invokeLater(), 
> but which wasn't synchronized.  The "temp" field itself would be safe, 


Correct.  The Runnable here is not inherently thread safe, hence the 
need for the volatile keyword.


> but nothing else in the Runnable instance would be (including, for 
> example, a v-table as might be used to dispatch the call to run()).


There's nothing in the JLS that describes a need to make *methods* safe 
for concurrency, or for the programmer to worry about internal 
structures such as a hypothetical v-table.  The JVM is guaranteed to 
construct a class correctly.  The fields used by the programmer must be 
made thread safe somehow by the programmer.

Right now you are just makin' stuff up.


Weird side note:  I was just reading through the JLS to make sure I had 
a couple of things right.  You know what else is guaranteed to be 
visible and thread safe?  Objects referred to by final fields, up to the 
point where said objects are assigned to the final field.  Thus this 
works as good as a volatile:

public class Main {
   public static void main( String... args ) {
     final MyClass test = new MyClass();
     SwingUtilities.invokeLater( new Runnable() {
       private final MyClass model = test;
       public void run() {
          ...  // ok to use model now...
       }
     } );
   }
}

markspace wrote:
> Peter Duniho wrote:
> 
>> If what you say were true, then you could not even count on the 
>> Runnable instance that you created to be valid during the invoked 
>> execution, since that's something that happened before the call to 
>> invokeLater(), but which wasn't synchronized.  The "temp" field itself 
>> would be safe, 
> 
> Correct.  The Runnable here is not inherently thread safe, hence the 
> need for the volatile keyword.

It is quite common to pass a Runnable to invoke�() without any use of 
"volatile" or other synchronization in the implementation.

None of those examples would be correct if your assertion was.

>> but nothing else in the Runnable instance would be (including, for 
>> example, a v-table as might be used to dispatch the call to run()).
> 
> There's nothing in the JLS that describes a need to make *methods* safe 
> for concurrency, or for the programmer to worry about internal 
> structures such as a hypothetical v-table.  The JVM is guaranteed to 
> construct a class correctly.  The fields used by the programmer must be 
> made thread safe somehow by the programmer.
> 
> Right now you are just makin' stuff up.

I'm simply pointing out that there are implicit order-of-operation 
guarantees that necessarily come along with something like the invoke�() 
methods that, without such guarantees, would break all sorts of code, 
including that implicitly executed.

I'm only "makin' stuff up" inasmuch as I haven't bothered to read the 
JLS for the purpose of this discussion.  But I don't need to.

It's patently obvious that the invoke�() methods couldn't possibly be 
useful unless they included synchronization between the thread that 
calls them and the thread where the Runnable is actually invoked.  The 
only way to safely get the Runnable reference from one thread to the 
other is for there to be some synchronization, and such synchronization 
will necessarily take care of ensuring everything up to the invoke�() 
call is seen in the other thread after the invoke�() call.

I did mention other aspects of initialization in my discussion, but you 
seem to be getting too distracted by the specifics of those comments, 
and I now regret introducing those comments.  I should have simply 
omitted them, because they aren't really needed in order to understand 
why invoke�() must have synchronization.  The EDT can't even reliably 
get the reference to the invoked Runnable unless there's some 
synchronization.

Pete

In article <i22ll3$hdt$1@news.eternal-september.org>,
 markspace <nospam@nowhere.com> wrote:

> Weird side note:  I was just reading through the JLS to make sure I 
> had a couple of things right.  You know what else is guaranteed to be 
> visible and thread safe?  Objects referred to by final fields, up to 
> the point where said objects are assigned to the final field.  Thus 
> this works as good as a volatile:
> 
> public class Main {
>    public static void main( String... args ) {
>      final MyClass test = new MyClass();
>      SwingUtilities.invokeLater( new Runnable() {
>        private final MyClass model = test;
>        public void run() {
>           ...  // ok to use model now...
>        }
>      } );
>    }
> }

IIUC, this is foundational to the lazy initialization holder 
class idiom [1], also known as the initialize-on-demand holder class 
idiom [2].

[1] Joshua Bloch, Effective Java, second edition, item 71.
[2]<http://en.wikipedia.org/wiki/Initialization_on_demand_holder_idiom>

-- 
John B. Matthews
trashgod at gmail dot com
<http://sites.google.com/site/drjohnbmatthews>

markspace wrote:
>> Weird side note:  I was just reading through the JLS to make sure I
>> had a couple of things right.  You know what else is guaranteed to be
>> visible and thread safe?  Objects referred to by final fields, up to
>> the point where said objects are assigned to the final field.  Thus
>> this works as good as a volatile:
>>
>> public class Main {
>>     public static void main( String... args ) {
>>       final MyClass test = new MyClass();
>>       SwingUtilities.invokeLater( new Runnable() {
>>         private final MyClass model = test;
>>         public void run() {
>>            ...  // ok to use model now...

Or you could use 'test' directly.

>>         }
>>       } );
>>     }
>> }

John B. Matthews wrote:
> IIUC, this is foundational to the lazy initialization holder
> class idiom [1], also known as the initialize-on-demand holder class
> idiom [2].
>
> [1] Joshua Bloch, Effective Java, second edition, item 71.
> [2]<http://en.wikipedia.org/wiki/Initialization_on_demand_holder_idiom>

Actually what's foundational to that is not so much final variables as final 
static fields.

-- 
Lew

On 19-07-2010 14:51, Peter Duniho wrote:
> The first mechanism has to do with semantics surrounding object
> construction. In .NET, the specification ensures that whatever happens
> in the constructor "happens before" any other code that uses the
> reference after the constructor completes (.NET doesn't actually use the
> term "happens-before", but it's the same idea). I would expect Java to
> be the same in this respect.

Where does it say that?

The only thing I can find seems to say the exact opposite:

<quote>
It is explicitly not a requirement that a conforming implementation of 
the CLI guarantee that all state updates
performed within a constructor be uniformly visible before the 
constructor completes.
</quote>

Arne

On 19-07-2010 13:08, Peter Duniho wrote:
> Note the use of a dedicated object reference for synchronization. In
> general, it is poor practice to use the "this" reference for
> synchronization, though that's what declaring a method as "synchronized"
> does. It's not the end of the world to use "this", but doing so "leaks"
> some of your implementation, and introduces the possibility of some
> other code not related to your own taking the same monitor, increasing
> contention as well as the complexity of the locking scenarios.

Someone wrote this many years ago for C#/.NET.

Everybody in the C#/.NET world has repeated it since then.

But no one seems to have ever seen a real world case where
it was a problem.

It is not best practice to avoid it.

Best practices is about avoiding real problems.

It just an urban legend type of thing.

Arne

Joshua Cranmer wrote:

> This is only true for final fields. Regular fields do not have the same 
> luxury--hence why it is often preferred to have immutable objects in Java.


One small addendum:  any fields (not just final) that are not 
initialized but left at their default (0 or null) state are also thread 
safe.  The JLS guarantees that the default values for all objects are 
visible to all threads.

Once you start assigning stuff though, all bets are off.

Arne Vajh�j wrote:
> On 19-07-2010 14:51, Peter Duniho wrote:
>> The first mechanism has to do with semantics surrounding object
>> construction. In .NET, the specification ensures that whatever happens
>> in the constructor "happens before" any other code that uses the
>> reference after the constructor completes (.NET doesn't actually use the
>> term "happens-before", but it's the same idea). I would expect Java to
>> be the same in this respect.
> 
> Where does it say that?
> 
> The only thing I can find seems to say the exact opposite:
> 
> <quote>
> It is explicitly not a requirement that a conforming implementation of 
> the CLI guarantee that all state updates
> performed within a constructor be uniformly visible before the 
> constructor completes.
> </quote>

Hmm.  Well, the above quote is inapplicable, because it describes what 
is true _before_ the constructor completes, not _after_.  My comment was 
with respect to _after_.

That said, it is possible I am misremembering, and only "readonly" 
fields (similar to "final" fields in Java) are guaranteed to be visible. 
  I was pretty sure the spec declared that for all fields, but I haven't 
actually looked through the spec recently myself.  And you appear to 
have, so if you couldn't find it, maybe it's not there.

But, if so, I wonder why they would bother to write the statement you 
quoted.  After all, if there's no guarantee that state updates are 
visible after the constructor completes, why would anyone think they 
would be guaranteed to be visible before the constructor completes?  And 
thus why would there be a need to state explicitly there's no such 
guarantee?

Pete

Arne Vajh�j wrote:
> On 19-07-2010 13:08, Peter Duniho wrote:
>> Note the use of a dedicated object reference for synchronization. In
>> general, it is poor practice to use the "this" reference for
>> synchronization, though that's what declaring a method as "synchronized"
>> does. It's not the end of the world to use "this", but doing so "leaks"
>> some of your implementation, and introduces the possibility of some
>> other code not related to your own taking the same monitor, increasing
>> contention as well as the complexity of the locking scenarios.
> 
> Someone wrote this many years ago for C#/.NET.
> 
> Everybody in the C#/.NET world has repeated it since then.
> 
> But no one seems to have ever seen a real world case where
> it was a problem.
> 
> It is not best practice to avoid it.
> 
> Best practices is about avoiding real problems.
> 
> It just an urban legend type of thing.

Yes, you've said all that before (in the C# newsgroup).  You're still wrong.

If you like, we can just copy/paste the back-and-forth we had there to 
this newsgroup.

Pete

Hi www,

I read your code. i would like to ask some questions regarding the
code? have you tried the code and is it giving NullPointerException or
it is ur assumption? b'se as the class or method or objects are not
'static', every instance of the class will have its own variables and
so it should be thread safe. 'this.checkSomething();' will make tim/
tom null but only of that object/thread and not of other objects/
threads. I dont find any problem in the code. I am not very
experienced at multi threaded programming but this is my primary and
logical thinking says. Please check and let me know.

Regards,
Chintan

Chintan(Neo) wrote:
> Hi www,
> 
> I read your code. i would like to ask some questions regarding the
> code? have you tried the code and is it giving NullPointerException or
> it is ur assumption? b'se as the class or method or objects are not
> 'static', every instance of the class will have its own variables and
> so it should be thread safe. 'this.checkSomething();' will make tim/
> tom null but only of that object/thread and not of other objects/
> threads. I dont find any problem in the code. I am not very
> experienced at multi threaded programming but this is my primary and
> logical thinking says. Please check and let me know.

There's not enough context in the question to say for sure.  But given 
the description in the original question, it seems safe to assume that 
both threads are using the same instance of the MyClass class.  Thus, 
while it's true that "every instance of the class will have its own 
variables", that fact in no way helps avoid thread safety problems.

Instances are not tied to threads.  Any given instance can have code 
executing in any given thread.

Pete

Hi Peter,

As you said, instances are not tied to threads which is correct.
however it was my assumption that instance of MyClass was created
inside the thread after the thread started and i gave my view
according to that. But ya, if the instance of MyClass is created
before the thread started and passed to the threads, it is definately
not thread safe and it seems that 'synchronized' is the option which
should be used.

On Jul 20, 1:25=A0pm, Peter Duniho <NpOeStPe...@NnOwSlPiAnMk.com> wrote:
>
> There's not enough context in the question to say for sure. =A0But given
> the description in the original question, it seems safe to assume that
> both threads are using the same instance of the MyClass class. =A0Thus,
> while it's true that "every instance of the class will have its own
> variables", that fact in no way helps avoid thread safety problems.
>
> Instances are not tied to threads. =A0Any given instance can have code
> executing in any given thread.
>
> Pete

On Mon, 19 Jul 2010, www wrote:

> public class MyClass
> {
> 	private Person tim = new Person("Tim");
> 	private Person tom = new Person("Tom");
>
> 	public void doThis
> 	{
> 		this.checkSomething();
>

Insert this here:

Person tim = this.tim;

> 		if(tim != null) //thread "A" is running "doThis()" and tim is not null at this moment
> 		{
>                        ... //other code
> 			tim.doA(); //tim is NULL all of sudden, and NPE is 
> thrown. I think it is due to another thread running checkSomething()
> 			tim.doB();
> 			tim.doC();
>
> 		}
>
> 	}
>
> 	public void checkSomething() //thread "B" is running checkSomething()
> 	{
> 		if(tim.isAbsent())
> 		{
> 			tim = null;
> 		}
>
> 		if(tom.isAbsent())
> 		{
> 			tom = null;
> 		}
>
> 	}
>
> }

That change will prevent NullPointerExceptions emanating from doThis. The 
point is that by copying the instance field into a local each thread gets 
its own copy, which is insulated from suprising effects.

You still have a risk of NullPointerExceptions from checkSomething - what 
if a thread enters that while either tim or tom is already null? I assume 
your code is actually doing null checks on the variables as well as 
calling isAbsent(). You should protect those in the same way as the code 
in doThis.

However! None of this will actually make the code threadsafe, probably, 
because it would still be possible for A to come in and set tim to null, 
then for B to come in and see an old value for tim and do something with 
it. To avoid that, i think it's sufficient to declare tim volatile.

tom

-- 
isn't it about time we had some new label for people interested in
technology who also have an interest in drinking binges, womanising and
occasional bouts of ultra violence? -- D

Tom Anderson wrote:
> On Mon, 19 Jul 2010, www wrote:
> 
>> public class MyClass
>> {
>>     private Person tim = new Person("Tim");
>>     private Person tom = new Person("Tom");
>>
>>     public void doThis
>>     {
>>         this.checkSomething();
>>
> 
> Insert this here:
> 
> Person tim = this.tim;
> 
>>         if(tim != null) //thread "A" is running "doThis()" and tim is 
>> not null at this moment
>> [...]
 >
> That change will prevent NullPointerExceptions emanating from doThis. 
> The point is that by copying the instance field into a local each thread 
> gets its own copy, which is insulated from suprising effects.

That assumes, however, that "tim == null" is not also an implicit flag 
indicating that the code should not be calling methods on the object 
previously referenced by "tim".

It avoids the NPE for sure.  But whether it makes the code thread-safe 
is an entirely different question.

> You still have a risk of NullPointerExceptions from checkSomething - 
> what if a thread enters that while either tim or tom is already null? I 
> assume your code is actually doing null checks on the variables as well 
> as calling isAbsent(). You should protect those in the same way as the 
> code in doThis.

Noting, of course, that issue isn't related to thread-safety.  Rather, 
even in a single-threaded program the original code could potentially 
have a problem with that.

> However! None of this will actually make the code threadsafe, probably, 
> because it would still be possible for A to come in and set tim to null, 
> then for B to come in and see an old value for tim and do something with 
> it. To avoid that, i think it's sufficient to declare tim volatile.

Again, it depends on what the code is supposed to do and what's legal. 
Using "volatile" will ensure that the most up-to-date value for the 
variable "tim" is retrieved by the thread calling "doThis()".  But, if 
it's okay to store the value to a local and then use the local, it may 
well be okay to get an out-of-date version of the instance field too.

On the other hand, if it's not okay to get an out-of-date version of the 
instance field, then it very well may not be okay to use the Person 
object after the up-to-date version of the instance field has been set 
to null, in which case full synchronization is required.

Of course, the third possibility is that it's not okay to get an 
out-of-date version, but it is okay to operate on an out-of-date 
version, in which case the modification you suggest works fine.

Unfortunately, the OP didn't provide enough context for anyone to know 
what's really safe, legal, or correct with regards to his program.  Any 
of those three possibilities are valid.

Pete

Peter Duniho wrote:
> Arne Vajh�j wrote:
>> On 19-07-2010 13:08, Peter Duniho wrote:
>>> Note the use of a dedicated object reference for synchronization. In
>>> general, it is poor practice to use the "this" reference for
>>> synchronization, though that's what declaring a method as "synchronized"
>>> does. It's not the end of the world to use "this", but doing so "leaks"
>>> some of your implementation, and introduces the possibility of some
>>> other code not related to your own taking the same monitor, increasing
>>> contention as well as the complexity of the locking scenarios.
>>
>> Someone wrote this many years ago for C#/.NET.
>>
>> Everybody in the C#/.NET world has repeated it since then.
>>
>> But no one seems to have ever seen a real world case where
>> it was a problem.
>>
>> It is not best practice to avoid it.
>>
>> Best practices is about avoiding real problems.
>>
>> It just an urban legend type of thing.
> 
> Yes, you've said all that before (in the C# newsgroup).  You're still 
> wrong.
> 
> If you like, we can just copy/paste the back-and-forth we had there to 
> this newsgroup.

I'd be interested to see that. I'd tend to agree with Arne. I generally 
avoid holding the monitor on an object whose class is not part of the 
code base, that is, I know that there is a monitor on every object, so I 
don't try to hold the monitor on an instance someone else's class, 
because that there for them to use.

The encapsulation as proscribed by the C# "best-practice" makes it 
impossible, I understand, but I really cotton to Arne's comment on 
"best-practices".

So, other than the ounce of prevention, what makes Arne wrong? (Or, link 
to the previous discussion if you're not interested.)

-- 
Alan Gutierrez - alan@blogometer.com - http://twitter.com/bigeasy

Alan Gutierrez wrote:
> [...]
>> If you like, we can just copy/paste the back-and-forth we had there to 
>> this newsgroup.
> 
> I'd be interested to see that. I'd tend to agree with Arne. I generally 
> avoid holding the monitor on an object whose class is not part of the 
> code base, that is, I know that there is a monitor on every object, so I 
> don't try to hold the monitor on an instance someone else's class, 
> because that there for them to use.
> 
> The encapsulation as proscribed by the C# "best-practice" makes it 
> impossible, I understand, but I really cotton to Arne's comment on 
> "best-practices".

The C# "best-practice" recommendation is to not lock on "this". 
Following that recommendation in no way prevents _other_ code from 
locking on the object reference itself.

In fact, there is no way to prevent other code from locking on the 
object reference itself, short of acquiring that lock and never 
releasing it.  Hence the recommendation for a class to not lock on its 
own "this" reference.  There's no way to know what other code might be 
using that reference for a lock.

> So, other than the ounce of prevention, what makes Arne wrong? (Or, link 
> to the previous discussion if you're not interested.)

Here's the link:
http://groups.google.com/group/microsoft.public.dotnet.languages.csharp/browse_thread/thread/95fa9dbd85a37227/dd03736b7038541c

As far as my point of view goes, I find the advice useful for the same 
reason I find the general OOP philosophy of information hiding and 
encapsulation useful.  There are a number of mistakes I have 
successfully avoided even before I was using OOP languages and which are 
prevented or at least minimized through the use of good OOP practices. 
But the fact that I've never actually had or seen those problems doesn't 
mean I should eschew OOP best-practices.

Should I insist that I make and/or observe every possible mistake 
possible before I incorporate policies in my own code to avoid those 
mistakes?

You may find these discussions interesting as well:
http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java
http://www.yoda.arachsys.com/csharp/threads/lockchoice.shtml

The former is not in any way 100% pro-"don't lock on this", but it has a 
bit of reasonable back-and-forth.  It also includes a (perhaps 
contrived) code example of how using "this" for synchronization not only 
can lead to problems, it can expose code to an intentional 
denial-of-service attack (not necessarily something that is even 
possible in all scenarios, but certainly worth thinking about IMHO).

Pete

Peter Duniho wrote:
> Alan Gutierrez wrote:
>> [...]
>>> If you like, we can just copy/paste the back-and-forth we had there 
>>> to this newsgroup.
>>
>> I'd be interested to see that. I'd tend to agree with Arne. I 
>> generally avoid holding the monitor on an object whose class is not 
>> part of the code base, that is, I know that there is a monitor on 
>> every object, so I don't try to hold the monitor on an instance 
>> someone else's class, because that there for them to use.
>>
>> The encapsulation as proscribed by the C# "best-practice" makes it 
>> impossible, I understand, but I really cotton to Arne's comment on 
>> "best-practices".
> 
> The C# "best-practice" recommendation is to not lock on "this". 
> Following that recommendation in no way prevents _other_ code from 
> locking on the object reference itself.
> 
> In fact, there is no way to prevent other code from locking on the 
> object reference itself, short of acquiring that lock and never 
> releasing it.  Hence the recommendation for a class to not lock on its 
> own "this" reference.  There's no way to know what other code might be 
> using that reference for a lock.
> 
>> So, other than the ounce of prevention, what makes Arne wrong? (Or, 
>> link to the previous discussion if you're not interested.)
> 
> Here's the link:
> http://groups.google.com/group/microsoft.public.dotnet.languages.csharp/browse_thread/thread/95fa9dbd85a37227/dd03736b7038541c 
> 
> 
> As far as my point of view goes, I find the advice useful for the same 
> reason I find the general OOP philosophy of information hiding and 
> encapsulation useful.  There are a number of mistakes I have 
> successfully avoided even before I was using OOP languages and which are 
> prevented or at least minimized through the use of good OOP practices. 
> But the fact that I've never actually had or seen those problems doesn't 
> mean I should eschew OOP best-practices.

Thank you for taking the time to layout a lesson plan. I read it all.

Yes, it is truly an ounce of prevention, and an appropriate application 
of encapsulation. Somewhere else in this list, someone wrote, objects 
are cheap, defects are expensive. Create a mutex Object makes sense.

The Collections.concurrentMap implementation is probably considered by 
C# programmers to be too clever.

-- 
Alan Gutierrez - alan@blogometer.com - http://twitter.com/bigeasy

markspace wrote:
> I agree that the only practical way to pass an object to the EDT
> involves a shared lock. However, invokeLater() makes no such guarantee,

Are you kidding?  The whole documented point of 'invokeLater()' and 
'invokeAndWait()' is to push things to the EDT in a thread-safe manner.

> and they'd be within their rights to change their implementation of
> invokeLater() and remove any shared locks, thus removing any
> synchronization and any happens-before semantics. Contrast this with the

They totally would not be within their rights to do that, as it would violate 
the entire point of the method.  When you look at the source and see all the 
work they did to build synchronization into those methods, you will understand 
that it's there for a reason - to do what the methods freaking promise to do!

> docs of some of the concurrent classes like Executor, which does provide
> an explicit memory consistency guarantee.

I agree that the promise should be repeated in the Javadocs for the methods 
themselves, but I'm not going to bet that anyone's going to remove 
synchronization from the implementation of the 'invoke...()' methods.

-- 
Lew

Lew wrote:

> I agree that the promise should be repeated in the Javadocs for the 
> methods themselves,

This is my main point.  If that is indeed their intention, they should 
document it.  The Swing team needs a once-over with a wet noodle for 
some of their concurrency documentation.

Lew wrote:

> markspace wrote:
>> I agree that the only practical way to pass an object to the EDT
>> involves a shared lock. However, invokeLater() makes no such guarantee,
> 
> Are you kidding?  The whole documented point of 'invokeLater()' and 
> 'invokeAndWait()' is to push things to the EDT in a thread-safe manner.


Addendum: no, I'm not kidding.  Where do you see it documented that the 
purpose of invokeLater() is to "push things" to the EDT?  I only see it 
documented that invokeLater() executes code, so that already existing 
objects, created on the EDT, can be accessed in a thread safe manner.

All the examples of using invokeLater() I can find carefully avoid any 
access of objects created off the EDT, unless that object is made thread 
safe some other way (e.g., use of final keyword).

If you can show documentation that even partially supports your claim, 
I'd love to see it.

markspace wrote:
> Lew wrote:
> 
>> markspace wrote:
>>> I agree that the only practical way to pass an object to the EDT
>>> involves a shared lock. However, invokeLater() makes no such guarantee,
>>
>> Are you kidding?  The whole documented point of 'invokeLater()' and 
>> 'invokeAndWait()' is to push things to the EDT in a thread-safe manner.
> 
> 
> Addendum: no, I'm not kidding.  Where do you see it documented that the 
> purpose of invokeLater() is to "push things" to the EDT?  I only see it 
> documented that invokeLater() executes code, so that already existing 
> objects, created on the EDT, can be accessed in a thread safe manner.
> 
> All the examples of using invokeLater() I can find carefully avoid any 
> access of objects created off the EDT, unless that object is made thread 
> safe some other way (e.g., use of final keyword).

In general, the examples I've seen use "final" only because that's the 
only way to refer to a local variable in an anonymous class.  It has 
nothing to do with "carefully avoiding any access of objects created off 
the EDT".

The example that IMHO is _definitive_ would be the discussion found here:
http://java.sun.com/products/jfc/tsc/articles/threads/threads1.html

And there's no example of any code there where any specific attempt is 
made to synchronize access to data initialized for use in the Runnable. 
  Here's an excerpt that specifically _doesn't_ do synchronization:

   void printTextField() throws Exception {
     final String[] myStrings =
       new String[2];

     Runnable getTextFieldText =
       new Runnable() {
         public void run() {
             myStrings[0] =
                textField0.getText();
             myStrings[1] =
                textField1.getText();
         }
     };
     SwingUtilities.invokeAndWait
       (getTextFieldText);

     System.out.println(myStrings[0]
                        + " " + myStrings[1]);
   }

Now, the above is actually (according to you) "broken" in the other 
direction.  That is, data is initialized in the EDT, and then used after 
the invokeAndWait() method returns, but no explicit synchronization is 
present to ensure that the data is visible in the original thread.  But 
it's exactly the same issue you're talking about.  If it's broken going 
from the original thread to the EDT (as you claim), it's also broken 
going from the EDT to the original thread.

> If you can show documentation that even partially supports your claim, 
> I'd love to see it.

If you can show documentation that even partially supports your claim, 
I'd love to see it.

You'd think that if it were important to explicitly synchronize data 
initialized prior to calling one of the invoke…() methods when that data 
will be used in the Runnable, the documentation would actually mention 
that fact.

The documentation that supports my claim (and Lew's, which is identical) 
is the JLS, where it describes the "happens-before" guarantees that are 
related to the use of the various synchronization techniques.  Along 
with, of course, the documentation for the invoke…() methods, which 
promise that those methods will actually work.

It would be nice if the JDK docs themselves distilled the admittedly 
more complicated presentation found in the JLS, to make it more clear to 
those less familiar with the JLS and concurrency issues in general.  But 
the fact that it doesn't isn't proof of the invoke…() methods being broken.

Regardless of the state of the documentation, as I have pointed out a 
number of times already, it is simply not possible for the invoke…() 
methods to work correctly and yet not ensure the necessary order of 
operations:

   • The invoke…() methods both take as an argument a reference to a 
Runnable instance

   • This reference _must_ be passed to the EDT somehow (after all, 
that's where the Runnable's run() method is going to be called), and the 
only way to achieve that in a thread-safe way is to synchronize some 
shared data between the two threads

   • The act of synchronizing shared data, even if simply by use of 
"volatile", _guarantees_ that any data written before the synchronized, 
shared data is written will be visible to any other thread that reads 
the data after the shared data is read

Thus, everything that your non-EDT thread that calls the invoke…() 
method writes before that call is visible in the EDT from the moment the 
Runnable's run() method starts executing (actually, a bit before that, 
when the Runnable's reference was actually read, but there's no 
practical way to take advantage of that…for all intents and purposes, 
it's the stuff in and after the run() method that counts).

There is simply no other way that it can work.  The entire invoke…() 
paradigm would be fundamentally broken otherwise.

Pete