Win a copy of Functional Reactive Programming this week in the Other Languages forum!
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

IceCreamMan.java threading issue (Monkhouse and Camerlengo)

 
Alexandru Dragoi
Ranch Hand
Posts: 36
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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?
 
Roel De Nijs
Sheriff
Posts: 10603
143
AngularJS Chrome Eclipse IDE Hibernate Java jQuery MySQL Database Spring Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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.
 
Himai Minh
Ranch Hand
Posts: 1328
6
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

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
Posts: 1051
5
Chrome Eclipse IDE Hibernate
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Himai

What if the following code was run:



i.e. 2 threads accessing the same List<IceCreamDish>
 
Himai Minh
Ranch Hand
Posts: 1328
6
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Posts: 1051
5
Chrome Eclipse IDE Hibernate
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Posts: 1051
5
Chrome Eclipse IDE Hibernate
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Sheriff
Posts: 10603
143
AngularJS Chrome Eclipse IDE Hibernate Java jQuery MySQL Database Spring Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Sheriff
Posts: 10603
143
AngularJS Chrome Eclipse IDE Hibernate Java jQuery MySQL Database Spring Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Posts: 36
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
Sheriff
Posts: 10603
143
AngularJS Chrome Eclipse IDE Hibernate Java jQuery MySQL Database Spring Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
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
 
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic