You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@hbase.apache.org by Ryan Rawson <ry...@gmail.com> on 2010/04/16 00:00:59 UTC

volatile considered harmful

So there I was, running a unit test.  I step away and hours later it
still had not finished.  It was stuck on this line of the code:

      putThread.done();
>    putThread.join();

So the putThread was not finishing as it was supposed to do.  Well,
what does putThread.done() do:

    public void done() {
      done = true;
      synchronized (this) {
        interrupt();
      }
    }


Looks reasonable, wait, what is this access of a variable shared among
multiple threads outside of a sync block?  Oh wait, its "ok", it's
volatile:

    private volatile boolean done;


And what does the run loop of the thread look like?  Again reasonable:

    public void run() {
      done.set(false);
      int val = 0;
      while (!done.get()) {
       // Do HRegion.put call with some exception handling
      }
    }


Well in theory this all looks reasonable.

But why harmful?

I had changed the Gets to not acquire row locks, thus removing a
synchronization block between the main thread (which did Gets and then
eventually called that code up top).  Two consequences, one was puts
happened much faster (good!), and a negative consequence is sometimes
the test would fail to stop the put thread.  For minutes and hours
after the main thread had signalled the put thread to stop, it still
had not.

I switched the volatile boolean => AtomicBoolean and the problem went away.

In short, the use of volatile is not guaranteed to see an update from
a different thread in any timely manner. The Java Memory Model does
not ensure this.  If you need to rely on passing data between threads
you need to either use a monitor lock (using synchronized keyword
either at the method level or as an explicit block) or use one of the
concurrent utilities classes - such as AtomicLong, AtomicBoolean, or
the excellent read/write locks.

If you wish to increment a counter from multiple threads, doing this:
volatile int count = 0;
// later:
count++;

will not work.  You must use an AtomicInt/Long or use a monitor lock.
This is because the operation "count++" is not atomic, it is actually
3 - get, add, put.

As we code in HBase, we should avoid the use of volatile as much as
possible.  The guarantees it makes are not good enough and can cause a
lot of pain figuring this out months after you originally coded it.

Re: volatile considered harmful

Posted by Joydeep Sarma <js...@gmail.com>.

just to clarify - the volatile in that link referred to C language
'volatile' keyword that happened to be used in the JDK native
implementation of park(), unpark() routines. C language 'volatile'
only prevents compiler optimizations - it doesn't apply memory
barriers after/before load/store.

i remember we had a terrible lost interrupt in hdfs once that was
similar to this problem (as ryan finally described it).

On Thu, Apr 15, 2010 at 3:28 PM, Kannan Muthukkaruppan
<Ka...@facebook.com> wrote:
> Ryan:
>
> Joy had brought up a related issue to attention recently, and fwd'ed this link:
>
> http://blogs.sun.com/dave/entry/a_race_in_locksupport_park
>
> It has to do with a volatile "_counter" variable as well.
>
> From the above writeup:
>
> <<< The problem would only manifest when we were using the -UseMembar optimization that lets us remove fences from certain hot thread state transitions paths that need to coordinate safepoints between mutator threads and the JVM. This feature is enabled by default, but we can turn it off with the -XX:+UseMembar switch, which causes the JVM to emit normal fence instructions in the state transitions paths. (That particular optimization is an example of asymmetric Dekker synchronization). Crucially, the park() path contains such a state transition. In reality the fence emitted by the +UseMembar switch was simply covering up the otherwise latent Parker:: bug. +UseMembar constitutes a work-around. Sensitivity to UseMembar was initially confounding but ultimately a valuable clue.>>>
>
>
> -----Original Message-----
> From: Dhruba Borthakur [mailto:dhruba@gmail.com]
> Sent: Thursday, April 15, 2010 3:24 PM
> To: hbase-dev@hadoop.apache.org
> Subject: Re: volatile considered harmful
>
> I agree. The Java spec clearly mentions that load/store/read/write to
> volatile are atomic
>
> http://java.sun.com/docs/books/jls/second_edition/html/memory.doc.html#28330
>
> <http://java.sun.com/docs/books/jls/second_edition/html/memory.doc.html#28330>but
> definitely not increment and decrement. increment and decrement operations
> need to load *and* store data to a volatile variable and are not atomic.
>
> -dhruba
>
>
> On Thu, Apr 15, 2010 at 3:00 PM, Ryan Rawson <ry...@gmail.com> wrote:
>
>> So there I was, running a unit test.  I step away and hours later it
>> still had not finished.  It was stuck on this line of the code:
>>
>>      putThread.done();
>> >    putThread.join();
>>
>> So the putThread was not finishing as it was supposed to do.  Well,
>> what does putThread.done() do:
>>
>>    public void done() {
>>      done = true;
>>      synchronized (this) {
>>        interrupt();
>>      }
>>    }
>>
>>
>> Looks reasonable, wait, what is this access of a variable shared among
>> multiple threads outside of a sync block?  Oh wait, its "ok", it's
>> volatile:
>>
>>    private volatile boolean done;
>>
>>
>> And what does the run loop of the thread look like?  Again reasonable:
>>
>>    public void run() {
>>      done.set(false);
>>      int val = 0;
>>      while (!done.get()) {
>>       // Do HRegion.put call with some exception handling
>>      }
>>    }
>>
>>
>> Well in theory this all looks reasonable.
>>
>> But why harmful?
>>
>> I had changed the Gets to not acquire row locks, thus removing a
>> synchronization block between the main thread (which did Gets and then
>> eventually called that code up top).  Two consequences, one was puts
>> happened much faster (good!), and a negative consequence is sometimes
>> the test would fail to stop the put thread.  For minutes and hours
>> after the main thread had signalled the put thread to stop, it still
>> had not.
>>
>> I switched the volatile boolean => AtomicBoolean and the problem went away.
>>
>> In short, the use of volatile is not guaranteed to see an update from
>> a different thread in any timely manner. The Java Memory Model does
>> not ensure this.  If you need to rely on passing data between threads
>> you need to either use a monitor lock (using synchronized keyword
>> either at the method level or as an explicit block) or use one of the
>> concurrent utilities classes - such as AtomicLong, AtomicBoolean, or
>> the excellent read/write locks.
>>
>> If you wish to increment a counter from multiple threads, doing this:
>> volatile int count = 0;
>> // later:
>> count++;
>>
>> will not work.  You must use an AtomicInt/Long or use a monitor lock.
>> This is because the operation "count++" is not atomic, it is actually
>> 3 - get, add, put.
>>
>> As we code in HBase, we should avoid the use of volatile as much as
>> possible.  The guarantees it makes are not good enough and can cause a
>> lot of pain figuring this out months after you originally coded it.
>>
>
>
>
> --
> Connect to me at http://www.facebook.com/dhruba
>
>

RE: volatile considered harmful

Posted by Kannan Muthukkaruppan <Ka...@facebook.com>.

Ryan:

Joy had brought up a related issue to attention recently, and fwd'ed this link:

http://blogs.sun.com/dave/entry/a_race_in_locksupport_park

It has to do with a volatile "_counter" variable as well.

>From the above writeup:

<<< The problem would only manifest when we were using the -UseMembar optimization that lets us remove fences from certain hot thread state transitions paths that need to coordinate safepoints between mutator threads and the JVM. This feature is enabled by default, but we can turn it off with the -XX:+UseMembar switch, which causes the JVM to emit normal fence instructions in the state transitions paths. (That particular optimization is an example of asymmetric Dekker synchronization). Crucially, the park() path contains such a state transition. In reality the fence emitted by the +UseMembar switch was simply covering up the otherwise latent Parker:: bug. +UseMembar constitutes a work-around. Sensitivity to UseMembar was initially confounding but ultimately a valuable clue.>>>

-----Original Message-----
From: Dhruba Borthakur [mailto:dhruba@gmail.com]
Sent: Thursday, April 15, 2010 3:24 PM
To: hbase-dev@hadoop.apache.org
Subject: Re: volatile considered harmful

I agree. The Java spec clearly mentions that load/store/read/write to
volatile are atomic

http://java.sun.com/docs/books/jls/second_edition/html/memory.doc.html#28330

<http://java.sun.com/docs/books/jls/second_edition/html/memory.doc.html#28330>but
definitely not increment and decrement. increment and decrement operations
need to load *and* store data to a volatile variable and are not atomic.

-dhruba

On Thu, Apr 15, 2010 at 3:00 PM, Ryan Rawson <ry...@gmail.com> wrote:

> So there I was, running a unit test.  I step away and hours later it
> still had not finished.  It was stuck on this line of the code:
>
>      putThread.done();
> >    putThread.join();
>
> So the putThread was not finishing as it was supposed to do.  Well,
> what does putThread.done() do:
>
>    public void done() {
>      done = true;
>      synchronized (this) {
>        interrupt();
>      }
>    }
>
>
> Looks reasonable, wait, what is this access of a variable shared among
> multiple threads outside of a sync block?  Oh wait, its "ok", it's
> volatile:
>
>    private volatile boolean done;
>
>
> And what does the run loop of the thread look like?  Again reasonable:
>
>    public void run() {
>      done.set(false);
>      int val = 0;
>      while (!done.get()) {
>       // Do HRegion.put call with some exception handling
>      }
>    }
>
>
> Well in theory this all looks reasonable.
>
> But why harmful?
>
> I had changed the Gets to not acquire row locks, thus removing a
> synchronization block between the main thread (which did Gets and then
> eventually called that code up top).  Two consequences, one was puts
> happened much faster (good!), and a negative consequence is sometimes
> the test would fail to stop the put thread.  For minutes and hours
> after the main thread had signalled the put thread to stop, it still
> had not.
>
> I switched the volatile boolean => AtomicBoolean and the problem went away.
>
> In short, the use of volatile is not guaranteed to see an update from
> a different thread in any timely manner. The Java Memory Model does
> not ensure this.  If you need to rely on passing data between threads
> you need to either use a monitor lock (using synchronized keyword
> either at the method level or as an explicit block) or use one of the
> concurrent utilities classes - such as AtomicLong, AtomicBoolean, or
> the excellent read/write locks.
>
> If you wish to increment a counter from multiple threads, doing this:
> volatile int count = 0;
> // later:
> count++;
>
> will not work.  You must use an AtomicInt/Long or use a monitor lock.
> This is because the operation "count++" is not atomic, it is actually
> 3 - get, add, put.
>
> As we code in HBase, we should avoid the use of volatile as much as
> possible.  The guarantees it makes are not good enough and can cause a
> lot of pain figuring this out months after you originally coded it.
>

--
Connect to me at http://www.facebook.com/dhruba

Re: volatile considered harmful

Posted by Dhruba Borthakur <dh...@gmail.com>.

I agree. The Java spec clearly mentions that load/store/read/write to
volatile are atomic

http://java.sun.com/docs/books/jls/second_edition/html/memory.doc.html#28330

<http://java.sun.com/docs/books/jls/second_edition/html/memory.doc.html#28330>but
definitely not increment and decrement. increment and decrement operations
need to load *and* store data to a volatile variable and are not atomic.

-dhruba


On Thu, Apr 15, 2010 at 3:00 PM, Ryan Rawson <ry...@gmail.com> wrote:

> So there I was, running a unit test.  I step away and hours later it
> still had not finished.  It was stuck on this line of the code:
>
>      putThread.done();
> >    putThread.join();
>
> So the putThread was not finishing as it was supposed to do.  Well,
> what does putThread.done() do:
>
>    public void done() {
>      done = true;
>      synchronized (this) {
>        interrupt();
>      }
>    }
>
>
> Looks reasonable, wait, what is this access of a variable shared among
> multiple threads outside of a sync block?  Oh wait, its "ok", it's
> volatile:
>
>    private volatile boolean done;
>
>
> And what does the run loop of the thread look like?  Again reasonable:
>
>    public void run() {
>      done.set(false);
>      int val = 0;
>      while (!done.get()) {
>       // Do HRegion.put call with some exception handling
>      }
>    }
>
>
> Well in theory this all looks reasonable.
>
> But why harmful?
>
> I had changed the Gets to not acquire row locks, thus removing a
> synchronization block between the main thread (which did Gets and then
> eventually called that code up top).  Two consequences, one was puts
> happened much faster (good!), and a negative consequence is sometimes
> the test would fail to stop the put thread.  For minutes and hours
> after the main thread had signalled the put thread to stop, it still
> had not.
>
> I switched the volatile boolean => AtomicBoolean and the problem went away.
>
> In short, the use of volatile is not guaranteed to see an update from
> a different thread in any timely manner. The Java Memory Model does
> not ensure this.  If you need to rely on passing data between threads
> you need to either use a monitor lock (using synchronized keyword
> either at the method level or as an explicit block) or use one of the
> concurrent utilities classes - such as AtomicLong, AtomicBoolean, or
> the excellent read/write locks.
>
> If you wish to increment a counter from multiple threads, doing this:
> volatile int count = 0;
> // later:
> count++;
>
> will not work.  You must use an AtomicInt/Long or use a monitor lock.
> This is because the operation "count++" is not atomic, it is actually
> 3 - get, add, put.
>
> As we code in HBase, we should avoid the use of volatile as much as
> possible.  The guarantees it makes are not good enough and can cause a
> lot of pain figuring this out months after you originally coded it.
>



-- 
Connect to me at http://www.facebook.com/dhruba