Rémi Forax | 2 Aug 15:33 2006
Picon

concurrency errors in java.lang.Throwable

I think i've found a concurrency bug in the Sun implementation
of java.lang.Throwable.

Since 1.4, Throwable accepts a cause at construction time
using a ad hoc constructors or after if initilialisation using
the method initCause().

First, the field can't be final and is not volatile, the method 
getCause() is not
synchronized and  doesn't use a lock, so a thread can create an exception
with a cause and another thread can see the same exception without
a cause.

Second, initCause is synchronized but getCause() is not, so a thread
can call initCause() and another thread never see the change.

Am i right ?

Should the field that store the cause be volatile and use
a CAS in initCause() to initialize the cause (the cause can't be
initialized twice) ?

Rémi Forax
Mike Quilleash | 2 Aug 16:59 2006

Setting the name of a Thread

I have a ThreadPoolExecutor that runs an arbitrary number of housekeeping tasks in a system.  Each of these tasks implements an interface extends Runnable and has getName().  I want to submit each of these to the executor and have the name of the thread set to the getName().
 
I thought I could just use a custom ThreadFactory and cast the Runnable but it seems the ThreadPool implementation wraps each Runnable up in a Worker inner class that I can't get my Runnable out of.
 
So I extended ThreadPoolExecutor and used the beforeExecute() hook which has my Runnable in it so I can cast out and change the thread name.  This worked fine.
 
Later on I decided to wrap up the Executor with a ExecutorCompletionService so I could trap any of these housekeeper tasks terminating early and report that in an error log.  But adding the CompletionService has now add another wrapper around my Runnable (the QueuedFuture that ECS uses) so my code in beforeExecute() broke with a ClassCastException.  I thought about storing the future result of ECS.submit -> my Runnable but it's then possible for the beforeExecute() to run before I've added the result to the map.
 
I ended up implementing ECS myself and storing a map of custom QueuedFuture -> Runnable and then doing the lookup in beforeExecute() to get my Runnable and therefore the thread name.
 
Is there any way or working around this as I've basically copied ECS out into my own code so I can create the FutureTask and add it to a map before it can be executed by the ThreadPool?
 
Any suggestions appreciated.
 

This e-mail is bound by the terms and conditions described at http://www.subexazure.com/mail-disclaimer.html

_______________________________________________
Concurrency-interest mailing list
Concurrency-interest <at> altair.cs.oswego.edu
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Ernst, Matthias | 2 Aug 17:24 2006

Re: Setting the name of a Thread

> I have a ThreadPoolExecutor that runs an arbitrary number of 
> housekeeping tasks in a system.  Each of these tasks 
> implements an interface extends Runnable and has getName().  
> I want to submit each of these to the executor and have the 
> name of the thread set to the getName().

Why don't you submit wrappers that implement the name-setting?

execute(final NamedRunnable namedRunnable) {
  tpe.execute(new Runnable() {
    public void run() {
      Thread t = Thread.currentThread();
      String oldName = t.getName();
      t.setName(namedRunnable.getName());
      try {
        namedRunnable.run();
      } finally {
        t.setName(oldName);
      }
    }
  });
}

Matthias
Mike Quilleash | 2 Aug 18:57 2006

Re: Setting the name of a Thread

Thanks.

That's a lot better and neater than what I had.  I've put the proper ECS
back in and it all works perfectly.

Cheers.

-----Original Message-----
From: Ernst, Matthias [mailto:matthias.ernst <at> coremedia.com] 
Sent: 02 August 2006 16:25
To: Mike Quilleash ; concurrency-interest <at> cs.oswego.edu
Subject: AW: [concurrency-interest] Setting the name of a Thread

> I have a ThreadPoolExecutor that runs an arbitrary number of 
> housekeeping tasks in a system.  Each of these tasks implements an 
> interface extends Runnable and has getName().
> I want to submit each of these to the executor and have the name of 
> the thread set to the getName().

Why don't you submit wrappers that implement the name-setting?

execute(final NamedRunnable namedRunnable) {
  tpe.execute(new Runnable() {
    public void run() {
      Thread t = Thread.currentThread();
      String oldName = t.getName();
      t.setName(namedRunnable.getName());
      try {
        namedRunnable.run();
      } finally {
        t.setName(oldName);
      }
    }
  });
}

Matthias

 This e-mail is bound by the terms and conditions described at http://www.subexazure.com/mail-disclaimer.html
Joe Bowbeer | 2 Aug 18:59 2006
Picon

Re: concurrency errors in java.lang.Throwable

On 8/2/06, Rémi Forax <forax <at> univ-mlv.fr> wrote:
> I think i've found a concurrency bug in the Sun implementation
> of java.lang.Throwable.
>
> Since 1.4, Throwable accepts a cause at construction time
> using a ad hoc constructors or after if initilialisation using
> the method initCause().
>
> First, the field can't be final and is not volatile, the method
> getCause() is not
> synchronized and  doesn't use a lock, so a thread can create an
> exception with a cause and another thread can see the same
> exception without a cause.
>
> Second, initCause is synchronized but getCause() is not, so a thread
> can call initCause() and another thread never see the change.
>
> Am i right ?
>
> Should the field that store the cause be volatile and use
> a CAS in initCause() to initialize the cause (the cause can't be
> initialized twice) ?
>
> Rémi Forax
>

I noticed the same thing a short while ago and discussed it with the
experts.  A summary of that discussion follows.

David Holmes writes:

  "Being safely publishable even when published without
synchronization goes a step beyond basic thread-safety. That is not
the general expectation from a thread-safe class. Yes the publishing
aspect is subtle and most people don't get it until they read
something like JCiP, but it is a classic concurrency problem: the
variable to which the object is published is a shared, mutable
variable - hence access to it requires synchronization.

I don't think we should be expending effort trying to make thread-safe
classes publishable without synchronization.(*) Rather we need to
educate because no matter what we do with JDK classes the programmer
will still screw up their own if they don't understand the issues."

(*) An exception is made for classes like String that are involved in
security.  These must be publishable without synchronization.  Or,
rather, unsafe publication of these classes must not represent a
security vulnerability.

David Holmes again:

  "As far as I can see with unsafe publication (and synchronized
getCause) the worst that can happen is you will see no-cause when
there should be one. Even if you see the default initialized cause
value that will be null and getCause will return null. We don't try to
make this work for other mutable classes so I don't see why Throwable
needs to be special. In the few scenarios where I can imagine passing
Throwables across threads, the Throwable isn't mutated after being
thrown, and the exchange between threads will involve
synchronization."

We decided that it would be an improvement (for clarity as much as
anything) if detailMessage were declared final.

One could also argue for synchronizing getCause, since all the other
setter/getter methods are, though, as far as we know, the
synchronization of initCause is only supposed to be preventing
multiple threads from calling that method simultaneously.  It's
supposed to be "call-once".

Are we overlooking a security vulnerability?

As an exercise, we sketched a Throwable implementation that would be
"safe" for un-safe publication:

public class Throwable {

  private final AtomicReference<Throwable> causeRef =
     new AtomicReference<Throwable>(this);

  public Throwable getCause() {
     Throwable t = causeRef.get();
     return (t == this ? null : t);
 }

  public Throwable initCause(Throwable cause) {
     if (cause == this)
         throw new IllegalArgumentException("Self-causation not permitted");
    if (!causeRef.compareAndSet(this, cause))
      throw new IllegalArgumentException("Can't overwrite cause");
  }
}

We thought it was a good, concise example of a thread-safe one-shot --
but that it was overkill in this situation...

--Joe
Gregg Wonderly | 2 Aug 19:57 2006
Picon
Picon

Re: concurrency errors in java.lang.Throwable

Joe Bowbeer wrote:
> I noticed the same thing a short while ago and discussed it with the
> experts.  A summary of that discussion follows.

> We thought it was a good, concise example of a thread-safe one-shot --
> but that it was overkill in this situation...

Typically causes are used in environments that are 'complicated'.  Either the 
method can't return an exception and it is wrapped and thrown, or a thread 
encounters a problem and publishes information about that problem while 
continuing to make whatever progress it can.

In either case, I think there is considerable information to be gained from 
making sure that the cause is available.  I think that there is some merit to 
Dougs thoughts that there will eventually be a synchronized piece of code that 
runs and forces the visibility.

This is one of the primary issues that developers in MP systems will fight with. 
  So, education is an important thing.  But, I also think that it will be 
important for core classes to provide a highlevel of correctness garantees which 
will make sure that developers get the right information for diagnosing the 
problems that they are having.

Gregg Wonderly
Brian Goetz | 2 Aug 21:18 2006

Re: concurrency errors in java.lang.Throwable

> In either case, I think there is considerable information to be gained from 
> making sure that the cause is available.  I think that there is some merit to 
> Dougs thoughts that there will eventually be a synchronized piece of code that 
> runs and forces the visibility.

JCiP coins the term "effectively immutable" for describing objects like 
this -- they don't meet the definition of immutability, but they are 
treated as if they were immutable forward from some point in time, and 
this point is prior to publication.  For effectively immutable objects, 
it is safe to use them without synchronization as long as they are 
published (once) with synchronization.
Gregg Wonderly | 2 Aug 22:49 2006

Re: concurrency errors in java.lang.Throwable


Brian Goetz wrote:
>>In either case, I think there is considerable information to be gained from 
>>making sure that the cause is available.  I think that there is some merit to 
>>Dougs thoughts that there will eventually be a synchronized piece of code that 
>>runs and forces the visibility.
> 
> 
> JCiP coins the term "effectively immutable" for describing objects like 
> this -- they don't meet the definition of immutability, but they are 
> treated as if they were immutable forward from some point in time, and 
> this point is prior to publication.  For effectively immutable objects, 
> it is safe to use them without synchronization as long as they are 
> published (once) with synchronization.

Yes, and that's my point.  It's important for some of these core things to 
happen 'safely' so that developers are not sidetracked by the same, constantly 
recurring behavior which is difficult to track down.

Maybe I missed something.  Is it felt that there's no real value in 
synchronizing getCause() because subclasses might override it and not 
synchronize?  Is there a feeling that synchronizing getCause() would place some 
undue burden on JVM performance?

Gregg Wonderly
Joe Bowbeer | 2 Aug 23:00 2006
Picon

Re: concurrency errors in java.lang.Throwable

On 8/2/06, Gregg Wonderly <gregg <at> cytetech.com> wrote:
>
> Maybe I missed something.  Is it felt that there's no real value in
> synchronizing getCause() because subclasses might override it and not
> synchronize?  Is there a feeling that synchronizing getCause() would
> place some undue burden on JVM performance?
>

It would be desirable to:

1. Declare detailMessage final
2. Make getCause synchronized
3. Most of all: document intent so we don't need to have these discussions
Gregg Wonderly | 2 Aug 23:09 2006
Picon
Picon

Re: concurrency errors in java.lang.Throwable

Joe Bowbeer wrote:
> On 8/2/06, Gregg Wonderly <gregg <at> cytetech.com> wrote:
> 
>>Maybe I missed something.  Is it felt that there's no real value in
>>synchronizing getCause() because subclasses might override it and not
>>synchronize?  Is there a feeling that synchronizing getCause() would
>>place some undue burden on JVM performance?
>
> It would be desirable to:
> 
> 1. Declare detailMessage final
> 2. Make getCause synchronized
> 3. Most of all: document intent so we don't need to have these discussions

Okay, that sounds like a good plan to me :-)

Gregg Wonderly

Gmane