aspose file tools*
The moose likes Developer Certification (SCJD/OCMJD) and the fly likes IceCreamMan.java threading issue (Monkhouse and Camerlengo) Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login
JavaRanch » Java Forums » Certification » Developer Certification (SCJD/OCMJD)
Bookmark "IceCreamMan.java threading issue (Monkhouse and Camerlengo)" Watch "IceCreamMan.java threading issue (Monkhouse and Camerlengo)" New topic
Author

IceCreamMan.java threading issue (Monkhouse and Camerlengo)

Alexandru Dragoi
Ranch Hand

Joined: May 09, 2008
Posts: 30
Hello,

Reading Monkhouse and Camerlengo book I (and also other people that have posted in the past to Javaranch SCJD forum) have spotted a problem on IceCreamMan.java class.

Here is the relevant fragment from this class:



Basically, dishes object is a queue that is accessed by 2 types of threads: Child threads (can be many of these), and IceCreamMan (is only one).
But as you can see in this example, the access to dishes object is synchronized (using the current IceCreamMan object lock) only when adding.

There is a correction in the Scjd book Errata, but it is referring only to removal from the queue:


But what about the other 2 cases where the queue is accessed :
1/ on line 7 in my example: dishes.isEmpty() &
2/ on line 16 in my example dishes.get(0)

I think also these 2 operations should be synchronized using the same lock (i.e. the current IceCreamMan object).

There were also other threads regarding this:
http://www.coderanch.com/t/189146/java-developer-SCJD/certification/SCJD-SE-thread#921556
and
http://www.coderanch.com/t/188273/java-developer-SCJD/certification/Monkhouse-Camerlengo#916913

In the last one Andrew responded regarding only the removal operation.
But nobody was discussing the other 2 operations: calling isEmpty() and get(int index) for the List class...

Does anyone know if there is an updated errata for the book?


Alex Dragoi, SCJP 1.4, OCPJP 6
Roel De Nijs
Bartender

Joined: Jul 19, 2004
Posts: 5539
    
  13

Whenever a method is invoked on the dishes list it should happen in a synchronized block (or method), because oherwise it's not thread-safe.

Errata can be found here, but it's last edited on November 2006. I don't think (based on the decreased popularity of this certification) that there will ever be another print of this book.


SCJA, SCJP (1.4 | 5.0 | 6.0), SCJD
http://www.javaroe.be/
Himai Minh
Ranch Hand

Joined: Jul 29, 2012
Posts: 799
    
    1

1/ on line 7 in my example: dishes.isEmpty() &
2/ on line 16 in my example dishes.get(0)



The dishes is a private variable, so it won't be shared by more than one IcecreamMan. In this case, there is only one IcecreamMan. So, synchronization is not needed for accessing dishes.
James Boswell
Bartender

Joined: Nov 09, 2011
Posts: 1030
    
    5

Himai

What if the following code was run:



i.e. 2 threads accessing the same List<IceCreamDish>
Himai Minh
Ranch Hand

Joined: Jul 29, 2012
Posts: 799
    
    1
James,
You are right. If there are two ice cream men accessing the same public dishes.
But in this case, there is only one ice cream man accessing its only private dishes.
James Boswell
Bartender

Joined: Nov 09, 2011
Posts: 1030
    
    5

James,
You are right. If there are two ice cream men accessing the same public dishes.
But in this case, there is only one ice cream man accessing its only private dishes.


I don't understand what you mean.

The OP is talking about making the IceCreamMan class thread-safe i.e. catering for multiple threads acting on the same IceCreamMan object.

If 2 or more threads act on the same IceCreamMan object, there is the possibility that the private member dishes could face concurrent modification issues. As the OP has correctly stated, any modification to the list should be wrapped in a synchronized block. The fact that the list is private is irrelevant.

James Boswell
Bartender

Joined: Nov 09, 2011
Posts: 1030
    
    5

Himai

To illustrate my point further, try running the following class a few times:

Notice how the output is sometimes a series of 0s, a series of 1s or even a ArrayIndexOutOfBoundsException is thrown.

Now, add the synchronized keyword to the changeIndex() method and re-run. The output will always be a series of 0s as the modification of the private member index is locked to a single thread.
Roel De Nijs
Bartender

Joined: Jul 19, 2004
Posts: 5539
    
  13

Himai Minh wrote:James,
You are right. If there are two ice cream men accessing the same public dishes.
But in this case, there is only one ice cream man accessing its only private dishes.

That makes no sense at all! If you want to make a class thread-safe, the number of instances doesn't matter. If 1 instance of a class is used by different threads, you can get into trouble if your class isn't thread-safe.

In this case (the example given in the book) 3 children (threads) will be accessing methods from 1 IceCreamMan, so this class must be thread-safe, otherwise these children might get into fight before the ice cream truck because one child gets all the ice cream and the other 2 get none.
Roel De Nijs
Bartender

Joined: Jul 19, 2004
Posts: 5539
    
  13

James Boswell wrote:To illustrate my point further, try running the following class a few times:

And that's exactly what's described in the book: 1 ice cream man instance (cfr. the NumberIndex instance a) is accessed by different children (threads t1 and t2).
Alexandru Dragoi
Ranch Hand

Joined: May 09, 2008
Posts: 30
I agree with Roel and James.

However, maybe we should make a distinction between the words "non-accurate" and "bad".

There is an example in "Java Concurrency in practice" by Brian Goetz that elaborates some important ideas. Here is a NotThreadSafe class (Chapter 3.1.1 Stale Data; Listing 3.2 ):



Brian brings the discussion to the point where only the set method is synchronized:
"Synchronizing only the setter would not be sufficient: threads calling get would still be able to see stale values."
and:
"Reading data without synchronization is analogous to using the READ_UNCOMMITTED isolation level in a database, where you are willing to trade accuracy for performance".

So the question that we should ask ourselves is: If we synchronize only when adding and removing from dishes queue, but NOT when reading it (i.e. doing dishes.isEmpty() & dishes.get(0)) it is really bad?
That surely isn't thread safe (and implicitly accurate), but how bad it really is in our case?

If our dishes queue is initially empty and some child thread adds an element to it, but in the same time IceCreamMan interrogates the queue using isEmpty() method, the IceCreamMan could see a stale state for dishes object.
So what?
We are gaining some performance because we synchronize only the write but not the read. The lock contention is actually reduced.
If IceCreamMan reads a stale value now, it is not a big deal because it will catch the updated state of the dishes at the next iteration!

In the conclusion, I want to specify again that I agree with the fact every access to the dishes list must be synchronized, otherwise our code is not thread safe. But we should also ask ourselves that the lack of synchronized keyword has a bad effect in our specific case.
Roel De Nijs
Bartender

Joined: Jul 19, 2004
Posts: 5539
    
  13

Alexandru Dragoi wrote:If our dishes queue is initially empty and some child thread adds an element to it, but in the same time IceCreamMan interrogates the queue using isEmpty() method, the IceCreamMan could see a stale state for dishes object.
So what?
We are gaining some performance because we synchronize only the write but not the read. The lock contention is actually reduced.
If IceCreamMan reads a stale value now, it is not a big deal because it will catch the updated state of the dishes at the next iteration!

I agree with you here. I believe it's also mentioned in one of the two threads mentioned in the original post.

You only have to be careful that the dishes.get(0); will never be invoked on an empty list, otherwise the ice cream man is in big trouble. In this example the children put their dish in the queue and the ice cream man serves them ice cream 1 by 1. So no problem in this scenario. It would be a whole different story (using this class) if 2 ice cream men would be serving the children. Or if 1 ice cream man would do nothing but serving ice cream dishes (a give away or another promotional activity) and a gazillion kids were trying to grab a dish. In these scenario's not making the reading of the list of dishes thread-safe will definitely cause mayhem

But you made an absolute justified remark. Definitely deserves a +1
 
It is sorta covered in the JavaRanch Style Guide.
 
subject: IceCreamMan.java threading issue (Monkhouse and Camerlengo)