ThreadLocal example bug.

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

ThreadLocal example bug.

Jason Mehrens
Correct me if I am wrong but, it appears that the new ThreadLocal example in
b99 code has a bug in the getCurrentThreadId method.  It returns the value
of the counter (uniqueId) and not the value from thread local (uniqueNum).  
June archives of the c-i list has the old traffic on this issue.

Regards,

Jason Mehrens


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadLocal example bug.

Thomas Hawtin
Jason Mehrens wrote:
> Correct me if I am wrong but, it appears that the new ThreadLocal example in
> b99 code has a bug in the getCurrentThreadId method.  It returns the value
> of the counter (uniqueId) and not the value from thread local (uniqueNum).  
> June archives of the c-i list has the old traffic on this issue.

Nice.

I guess that blindness that comes from viewing the same thing too often.

Perhaps following the rule of thumb that variables should be declared
with as tight a scope as possible should be applied here:

  import java.util.concurrent.atomic.AtomicInteger;

  public class UniqueThreadIdGenerator {

      private static final ThreadLocal < Integer > uniqueNum =
          new ThreadLocal < Integer > () {
              final AtomicInteger uniqueId = new AtomicInteger(0);
              @Override protected Integer initialValue() {
                  return uniqueId.getAndIncrement();
              }
          };

      public static int getCurrentThreadId() {
          return uniqueNum.get();
      }
  } // UniqueThreadIdGenerator

Tom Hawtin
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadLocal example bug.

Pete.Soper
In reply to this post by Jason Mehrens
Hi Jason,
             For the example code field uniqueId is a "dispenser" of the
next available id value and that is used once per thread (but is visible
to all threads) via the overriden initialVlaue method invocation that is
a side effect of the first invocation of uniqueNum.get. The per-thread
value is maintained by ThreadLocal code that you can't see and it never
changes for a given thread. But maybe I'm missing something. Do you have
a misbehaving test?

Regards,
Pete

  import java.util.concurrent.atomic.AtomicInteger;

  public class UniqueThreadIdGenerator {

      private static final ThreadLocal < Integer > uniqueNum =
          new ThreadLocal < Integer > () {
              final AtomicInteger uniqueId = new AtomicInteger(0);
              @Override protected Integer initialValue() {
                  return uniqueId.getAndIncrement();
              }
          };

      public static int getCurrentThreadId() {
          return uniqueNum.get();
      }
  } // UniqueThreadIdGenerator

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadLocal example bug.

Jason Mehrens
In reply to this post by Jason Mehrens
Pete,

The code you have attached bellow is correct (and what i would expect to
see) however, it is not the code out on the web. From
http://download.java.net/jdk6/docs/api/java/lang/ThreadLocal.html

import java.util.concurrent.atomic.AtomicInteger;

public class UniqueThreadIdGenerator {

     private static final AtomicInteger uniqueId = new AtomicInteger(0);

     private static final ThreadLocal < Integer > uniqueNum =
         new ThreadLocal < Integer > () {
             @Override protected Integer initialValue() {
                 return uniqueId.getAndIncrement();
         }
     };

     public static int getCurrentThreadId() {
         return uniqueId.get();
     }
} // UniqueThreadIdGenerator


Regards,

Jason Mehrens


>From: [hidden email]
>To: Jason Mehrens <[hidden email]>
>CC: [hidden email]
>Subject: Re: [concurrency-interest] ThreadLocal example bug.
>Date: Wed, 27 Sep 2006 13:20:20 -0400
>
>Hi Jason,
>             For the example code field uniqueId is a "dispenser" of the
>next available id value and that is used once per thread (but is visible to
>all threads) via the overriden initialVlaue method invocation that is a
>side effect of the first invocation of uniqueNum.get. The per-thread value
>is maintained by ThreadLocal code that you can't see and it never changes
>for a given thread. But maybe I'm missing something. Do you have a
>misbehaving test?
>
>Regards,
>Pete
>
>  import java.util.concurrent.atomic.AtomicInteger;
>
>  public class UniqueThreadIdGenerator {
>
>      private static final ThreadLocal < Integer > uniqueNum =
>          new ThreadLocal < Integer > () {
>              final AtomicInteger uniqueId = new AtomicInteger(0);
>              @Override protected Integer initialValue() {
>                  return uniqueId.getAndIncrement();
>              }
>          };
>
>      public static int getCurrentThreadId() {
>          return uniqueNum.get();
>      }
>  } // UniqueThreadIdGenerator
>


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadLocal example bug.

Pete.Soper
Oh. I thought the fragment of the class file Tom sent me was the code in
question. Dang.

Pete

Jason Mehrens wrote:

> Pete,
>
> The code you have attached bellow is correct (and what i would expect
> to see) however, it is not the code out on the web. From
> http://download.java.net/jdk6/docs/api/java/lang/ThreadLocal.html
>
> import java.util.concurrent.atomic.AtomicInteger;
>
> public class UniqueThreadIdGenerator {
>
>      private static final AtomicInteger uniqueId = new AtomicInteger(0);
>
>      private static final ThreadLocal < Integer > uniqueNum =
>          new ThreadLocal < Integer > () {
>              @Override protected Integer initialValue() {
>                  return uniqueId.getAndIncrement();
>          }
>      };
>
>      public static int getCurrentThreadId() {
>          return uniqueId.get();
>      }
> } // UniqueThreadIdGenerator
>
>
> Regards,
>
> Jason Mehrens
>
>
>> From: [hidden email]
>> To: Jason Mehrens <[hidden email]>
>> CC: [hidden email]
>> Subject: Re: [concurrency-interest] ThreadLocal example bug.
>> Date: Wed, 27 Sep 2006 13:20:20 -0400
>>
>> Hi Jason,
>>             For the example code field uniqueId is a "dispenser" of
>> the next available id value and that is used once per thread (but is
>> visible to all threads) via the overriden initialVlaue method
>> invocation that is a side effect of the first invocation of
>> uniqueNum.get. The per-thread value is maintained by ThreadLocal code
>> that you can't see and it never changes for a given thread. But maybe
>> I'm missing something. Do you have a misbehaving test?
>>
>> Regards,
>> Pete
>>
>>  import java.util.concurrent.atomic.AtomicInteger;
>>
>>  public class UniqueThreadIdGenerator {
>>
>>      private static final ThreadLocal < Integer > uniqueNum =
>>          new ThreadLocal < Integer > () {
>>              final AtomicInteger uniqueId = new AtomicInteger(0);
>>              @Override protected Integer initialValue() {
>>                  return uniqueId.getAndIncrement();
>>              }
>>          };
>>
>>      public static int getCurrentThreadId() {
>>          return uniqueNum.get();
>>      }
>>  } // UniqueThreadIdGenerator
>>
>
>
>

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadLocal example bug.

Joshua Bloch-2
I would argue that the variable names could be better (and used to be).  Also the vertical spacing conventions and comment conventions aren't exactly standard (though not far off).  How about this?

import java.util.concurrent.atomic.AtomicInteger ;

 public class ThreadId {
     // Atomic integer containing the next thread ID to be assigned
     private static final AtomicInteger nextId = new AtomicInteger(0);

     // Thread local variable containing each thread's ID
     private static final ThreadLocal < Integer > threadId =
         new ThreadLocal<Integer>() {
             @Override protected Integer initialValue() {
                 return nextId.getAndIncrement ();
         }
     };
 
     // Returns the current thread's unique ID, assigning it if necessary
     public static int get() {
         return threadId.get();
     }
 }

Note that the client now says ThreadId.get(), rather than ThreadIdGenerator.getCurrentThreadId().

           Josh




On 9/27/06, [hidden email] <[hidden email]> wrote:
Oh. I thought the fragment of the class file Tom sent me was the code in
question. Dang.

Pete

Jason Mehrens wrote:

> Pete,
>
> The code you have attached bellow is correct (and what i would expect
> to see) however, it is not the code out on the web. From
> http://download.java.net/jdk6/docs/api/java/lang/ThreadLocal.html
>
> import java.util.concurrent.atomic.AtomicInteger;
>

> public class UniqueThreadIdGenerator {
>
>      private static final AtomicInteger uniqueId = new AtomicInteger(0);
>
>      private static final ThreadLocal < Integer > uniqueNum =
>          new ThreadLocal < Integer > () {
>              @Override protected Integer initialValue() {
>                  return uniqueId.getAndIncrement();
>          }
>      };
>

>      public static int getCurrentThreadId() {
>          return uniqueId.get();
>      }
> } // UniqueThreadIdGenerator
>
>
> Regards,
>
> Jason Mehrens
>
>
>> From: [hidden email]
>> To: Jason Mehrens <[hidden email]>
>> CC: [hidden email]
>> Subject: Re: [concurrency-interest] ThreadLocal example bug.
>> Date: Wed, 27 Sep 2006 13:20:20 -0400
>>
>> Hi Jason,
>>             For the example code field uniqueId is a "dispenser" of
>> the next available id value and that is used once per thread (but is
>> visible to all threads) via the overriden initialVlaue method
>> invocation that is a side effect of the first invocation of
>> uniqueNum.get. The per-thread value is maintained by ThreadLocal code
>> that you can't see and it never changes for a given thread. But maybe
>> I'm missing something. Do you have a misbehaving test?
>>
>> Regards,
>> Pete
>>
>>  import java.util.concurrent.atomic.AtomicInteger;
>>
>>  public class UniqueThreadIdGenerator {
>>
>>      private static final ThreadLocal < Integer > uniqueNum =
>>          new ThreadLocal < Integer > () {
>>              final AtomicInteger uniqueId = new AtomicInteger(0);
>>              @Override protected Integer initialValue() {
>>                  return uniqueId.getAndIncrement();
>>              }
>>          };
>>
>>      public static int getCurrentThreadId() {
>>          return uniqueNum.get();
>>      }
>>  } // UniqueThreadIdGenerator
>>
>
>
>

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ThreadLocal example bug.

Pete.Soper
In reply to this post by Pete.Soper
OK, this is now bug id 6475885 and starting "soon" if you google
"UniqueThreadIdGenerator" the fix should show up too. It might help to
visit this page in case I screwed it up too:

   
http://blogs.sun.com/microwaves/entry/java_lang_threadlocal_example_class

I have something less than 1440 minutes left to study for the hardest
math test I'll have ever taken at that point in my life, so it's hard to
attach much weight to the formatting right now.  The choice of names got
quite a lot of review way back wen and would require another formal
review by the community. I won't be in the position to facilitate that
for a while. But I stashed Josh's msg into the bug comments.

Regards,
Pete

I wrote:

>Oh. I thought the fragment of the class file Tom sent me was the code in
>question. Dang.
>
>Pete
>
>Jason Mehrens wrote:
>
>  
>
>>Pete,
>>
>>The code you have attached bellow is correct (and what i would expect
>>to see) however, it is not the code out on the web. From
>>http://download.java.net/jdk6/docs/api/java/lang/ThreadLocal.html
>>
>>import java.util.concurrent.atomic.AtomicInteger;
>>
>>public class UniqueThreadIdGenerator {
>>
>>     private static final AtomicInteger uniqueId = new AtomicInteger(0);
>>
>>     private static final ThreadLocal < Integer > uniqueNum =
>>         new ThreadLocal < Integer > () {
>>             @Override protected Integer initialValue() {
>>                 return uniqueId.getAndIncrement();
>>         }
>>     };
>>
>>     public static int getCurrentThreadId() {
>>         return uniqueId.get(); // INCORRECT
>>     }
>>} // UniqueThreadIdGenerator
>>
>>
>>Regards,
>>
>>Jason Mehrens
>>
>>
>>    
>>
>>>From: [hidden email]
>>>To: Jason Mehrens <[hidden email]>
>>>CC: [hidden email]
>>>Subject: Re: [concurrency-interest] ThreadLocal example bug.
>>>Date: Wed, 27 Sep 2006 13:20:20 -0400
>>>
>>>Hi Jason,
>>>            For the example code field uniqueId is a "dispenser" of
>>>the next available id value and that is used once per thread (but is
>>>visible to all threads) via the overriden initialVlaue method
>>>invocation that is a side effect of the first invocation of
>>>uniqueNum.get. The per-thread value is maintained by ThreadLocal code
>>>that you can't see and it never changes for a given thread. But maybe
>>>I'm missing something. Do you have a misbehaving test?
>>>
>>>Regards,
>>>Pete
>>>
>>> import java.util.concurrent.atomic.AtomicInteger;
>>>
>>> public class UniqueThreadIdGenerator {
>>>
>>>     private static final ThreadLocal < Integer > uniqueNum =
>>>         new ThreadLocal < Integer > () {
>>>             final AtomicInteger uniqueId = new AtomicInteger(0);
>>>             @Override protected Integer initialValue() {
>>>                 return uniqueId.getAndIncrement();
>>>             }
>>>         };
>>>
>>>     public static int getCurrentThreadId() {
>>>         return uniqueNum.get(); // CORRECT
>>>     }
>>> } // UniqueThreadIdGenerator
>>>
>>>      
>>>
>>
>>    
>>
>
>_______________________________________________
>Concurrency-interest mailing list
>[hidden email]
>http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
>
>  
>

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest