JDK-6251838 : (spec thread) Misleading example usage code in java.lang.ThreadLocal
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.lang
  • Affected Version: 5.0,6
  • Priority: P4
  • Status: Closed
  • Resolution: Duplicate
  • OS: generic,linux
  • CPU: generic,x86
  • Submitted: 2005-04-07
  • Updated: 2011-02-16
  • Resolved: 2006-05-17
Related Reports
Duplicate :  
Duplicate :  
Relates :  
Description
A DESCRIPTION OF THE PROBLEM :
The class description contains the following example usage code:


 public class SerialNum {
     // The next serial number to be assigned
     private static int nextSerialNum = 0;

     private static ThreadLocal serialNum = new ThreadLocal() {
         protected synchronized Object initialValue() {
             return new Integer(nextSerialNum++);
         }
     };

     public static int get() {
         return ((Integer) (serialNum.get())).intValue();
     }
 }
 

What's confusing is the role of the *synchronized* keyword in
the initialValue() override.  You should clearly state what it is
that you're protecting when using that keyword--especially in
a class like this one.  In this case, you're protecting the serial
number from concurrent access.

Again, why is this confusing?  Because this might lead readers
to believe that their ThreadLocal.initialValue() overrides must
always synchronize on the ThreadLocal object itelf.  Not only
is that untrue, it is bad practice.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
This version synchronize's on SerialNum itself, rather
than on the thread local variable. (In a real application
it would be even better if SerialNum synchronized
on a private static lock object so that no user could
deadlock the SerialNum class, but here, the goal is
clarity...)

 public class SerialNum {
     // The next serial number to be assigned
     private static int nextSerialNum = 0;

     private static ThreadLocal serialNum = new ThreadLocal() {
         protected Object initialValue() {
             int next;
             synchronized (SerialNum.class) {
                 next = nextSerialNum++;
             }
             return new Integer(next);
         }
     };

     public static int get() {
         return ((Integer) (serialNum.get())).intValue();
     }

 }
 
ACTUAL -
See the description.

URL OF FAULTY DOCUMENTATION :
http://java.sun.com/j2se/1.5.0/docs/api/java/lang/ThreadLocal.html
###@###.### 2005-04-07 08:13:53 GMT
Although there is a plan to systematically generify all Thread* code examples that were overlooked during Tiger, here is a specific example for this program. It still needs error correction but shows more modern usage of ThreadLocal. Merging this with the suggestions above will result in a "suggested fix."

class SerialNum {
    private static ThreadLocal<Integer> serialNum
	= new ThreadLocal<Integer>()
    {
	// The next serial number to be assigned
	private int nextSerialNum = 0;

	protected synchronized Integer initialValue() {
            return nextSerialNum++;
        }
    };

    public static int get() {
        return serialNum.get();
    }
}

Comments
EVALUATION This is being closed as a duplicate of older bug 6202942 (fix in progress) only because it's easier to combine the two changes than to use separate process steps for them. So the older bug will document the need for synchronized for the example's initialValue.
17-05-2006

EVALUATION The need for clarification of synchronization in the example is valid. ###@###.### 2005-04-27 15:21:56 GMT
27-04-2005