concurrency errors in java.lang.Throwable

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

concurrency errors in java.lang.Throwable

Remi Forax
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



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

Re: concurrency errors in java.lang.Throwable

Joe Bowbeer
On 8/2/06, Rémi Forax <[hidden email]> 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

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

Re: concurrency errors in java.lang.Throwable

Gregg Wonderly-3
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
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: concurrency errors in java.lang.Throwable

Brian Goetz
> 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.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: concurrency errors in java.lang.Throwable

Gregg Wonderly-2


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

Re: concurrency errors in java.lang.Throwable

Joe Bowbeer
On 8/2/06, Gregg Wonderly <[hidden email]> 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
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: concurrency errors in java.lang.Throwable

Gregg Wonderly-3
Joe Bowbeer wrote:

> On 8/2/06, Gregg Wonderly <[hidden email]> 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
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: concurrency errors in java.lang.Throwable

Dhanji R. Prasanna
This is purely hypothetical (and probably not feasible), but if the
cause is effectively immutable (after being set) and getCause()'s
contract is to only ever return the cause, is there a problem with
making it synchronized and final?

Obviously it would break backwards compat but maybe something like a
SafeThrowable?

On 8/3/06, Gregg Wonderly <[hidden email]> wrote:

> Joe Bowbeer wrote:
> > On 8/2/06, Gregg Wonderly <[hidden email]> 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
> _______________________________________________
> 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: concurrency errors in java.lang.Throwable

David Holmes-3
In reply to this post by Joe Bowbeer
Joe,

> 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

getCause being synchronized would only be sufficient if the constructor used
the synchronized initCause() - otherwise we'd need a volatile, or atomics or
...

But to repeat what I've said elsewhere I don't expect Throwables to be
passed across threads very often. And if they are passed across threads then
that mechanism will need to involve synchronization. Hence access to the
cause would be safe.

I think this is a non-issue, but might support "fixing" it purely for reason
3. Only trouble is today it is Throwable tomorrow  its ...

Cheers,
David Holmes

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

Re: concurrency errors in java.lang.Throwable

Remi Forax
David Holmes a écrit :

> Joe,
>
>> 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
>
> getCause being synchronized would only be sufficient if the constructor used
> the synchronized initCause() - otherwise we'd need a volatile, or atomics or
> ...
>
> But to repeat what I've said elsewhere I don't expect Throwables to be
> passed across threads very often. And if they are passed across threads then
> that mechanism will need to involve synchronization. Hence access to the
> cause would be safe.
>
> I think this is a non-issue, but might support "fixing" it purely for reason
> 3. Only trouble is today it is Throwable tomorrow  its ...
>
> Cheers,
> David Holmes

I've a test case that shows the bug :

public class ThrowableConcurrencyBug {
   public static void initCause(Throwable t) {
     t.initCause(new Throwable());
   }

   public static void main(String[] args) {
     final Throwable t=new Throwable();
     new Thread() {
       @Override
       public void run() {
         for(;;) {
           if (t.getCause()!=null)
             System.out.println("bang !");
         }
       }
     }.start();

     for(int i=0;;i++) {
       initCause((i==100000)?t:new Throwable());
     }
   }
}

"bang !" is never (at least 15min :) printed with the server VM.

best regards

Rémi Forax

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

Re: concurrency errors in java.lang.Throwable

Gregg Wonderly-3
Rémi Forax wrote:
> I've a test case that shows the bug :

...

> "bang !" is never (at least 15min :) printed with the server VM.

Right...  Because you are publishing without synchronization, the initCause()
call's action of setting the field in one thread is not guarenteed to be visible
in another threads read of that field.

On my laptop (dual core Xeon), it does become visible.  Perhaps because of the
rate of garbage generation (new Throwable()), the GC activities are driving the
visibility...

As David said, "Only trouble is today it is Throwable tomorrow, its ...".  There
are all kinds of opportunities for threads to forget to synchronize
communications/publishing of values between threads.  That's the education issue.

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

Re: concurrency errors in java.lang.Throwable

Joe Bowbeer
In reply to this post by David Holmes-3
On 8/2/06, David Holmes <[hidden email]> wrote:
> Joe,
>
> getCause being synchronized would only be sufficient if the constructor
> used the synchronized initCause() - otherwise we'd need a volatile, or
> atomics or
>

Agreed.  In my original response to c-i, I said "one could argue" for
synchronizing getCause, but it wouldn't solve the problem.

Even without solving the problem, though, I thought it was desirable
to synchronize getCause -- for consistency.  But now I think
documentation is even more important.  Adding a "synchronized" that
won't fix anything may be just as confusing as leaving it out.

I still think using atomics is overkill unless there's a security vulnerability.

So I revise the work list to:

1. Declare detailMessage final.
2. Document why getCause isn't synchronized


I think it's also worth mentioning here that declaring cause as
volatile won't necessarily work.

Example from Jeremy Manson:

Thread 1:

// begin constructor
myThrowable.cause = this;
// end constructor
global = myThrowable;

Thread 2:

yourThrowable = global;
if (yourThrowable.cause == null)
  System.err.println("Null cause");

As Jeremy explains, the problem is that the assignment to global can
be hoisted above the write to cause (it isn't final).  In which case
the second thread won't see the appropriately initialized value of
cause.

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