JDK-6454029 : JDK 1.6 not compliant with Java Memory Model
  • Type: Bug
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 6
  • Priority: P3
  • Status: Closed
  • Resolution: Not an Issue
  • OS: windows_xp
  • CPU: x86
  • Submitted: 2006-07-28
  • Updated: 2011-02-16
  • Resolved: 2006-07-30
Related Reports
Relates :  
Description
FULL PRODUCT VERSION :
java version "1.6.0-beta2"
Java(TM) SE Runtime Environment (build 1.6.0-beta2-b86)
Java HotSpot(TM) Server VM (build 1.6.0-beta2-b86, mixed mode)

ADDITIONAL OS VERSION INFORMATION :
Microsoft Windows XP [Version 5.1.2600]

A DESCRIPTION OF THE PROBLEM :
We have a request/reply scenario with 2 different threads. Thread 1 performs a request via Thread 2 and wait on a semaphore. Thread 2 sets a reply in the request and notifies the semaphore. The set/getReply in the Request class is either synchronized or the Reply field is volatile (see test source). With both versions we have situations where the semaphore is was notified but getReply returns null in thread 1.

It seems the instructions of thread 2 (setReply, notify semaphore) have been reordered in (notify, setReply) or the memory changes of thread 2 have not been flushed when thread 1 calls getReply.

Everything works fine with JDK1.5.0_07.


REGRESSION.  Last worked in version mustang

STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
Use the attached testdriver as follows:

JMMTest 100000

It runs 30, 60, 100 pairs of request/reply threads with either volatile or synchronized Request class. It fails often but you might be required to run it multiple times. Of course, use a multiprocessor machine.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
You should see this kind of output which indicates a getReply() == null:

D:\jdk1.6.0\bin\java -Xmx1024M -cp ;../testdriver JMMTest 100000
-- Type: 1
---- 30 Pairs ...
-- Type: 2
---- 30 Pairs ...
Reply == null, n=75717, class=JMMTest$SRequest
ACTUAL -
D:\jdk1.6.0\bin\java -Xmx1024M -cp ;../testdriver JMMTest 100000
-- Type: 1
---- 30 Pairs ...
-- Type: 2
---- 30 Pairs ...
Reply == null, n=75717, class=JMMTest$SRequest

REPRODUCIBILITY :
This bug can be reproduced often.

---------- BEGIN SOURCE ----------
public class JMMTest
{
  public JMMTest(int n)
  {
    int[] pairs = new int[]{30, 60, 100};

    for (int i = 0; i < pairs.length; i++)
    {
      for (int k = 1; k < 3; k++)
      {
        System.out.println("-- Type: " + k);
        System.out.println("---- " + pairs[i] + " Pairs ...");
        Replier[] replier = new Replier[pairs[i]];
        Requestor[] requestor = new Requestor[pairs[i]];
        for (int j = 0; j < pairs[i]; j++)
        {
          replier[j] = new Replier(n);
          requestor[j] = new Requestor(new RequestFactory(k), replier[j], n);
        }
        for (int j = 0; j < pairs[i]; j++)
        {
          replier[j].start();
          requestor[j].start();
        }
        for (int j = 0; j < pairs[i]; j++)
        {
          try
          {
            replier[j].join();
            requestor[j].join();
          } catch (InterruptedException e)
          {
            e.printStackTrace();
          }
        }
      }
    }

  }
  public static void main(String args[])
  {
    new JMMTest(Integer.parseInt(args[0]));
  }

  private class Requestor extends Thread
  {
    RequestFactory factory = null;
    Replier replier = null;
    int n = 0;

    public Requestor(RequestFactory factory, Replier replier, int n)
    {
      this.factory = factory;
      this.replier = replier;
      this.n = n;
    }

    public void run()
    {
      for (int i = 0; i < n; i++)
      {
        Request request = factory.createRequest();
        replier.setRequest(request);
        replier.getSemaphore().notifySingleWaiter();
        request.getSemaphore().waitHere();
        synchronized (replier)
        {
          if (request.getReply() == null)
          {
            System.err.println("Reply == null, n=" + request.getN() + ", class=" + request.getClass().getName());
            System.exit(-1);
          }
        }
      }
    }
  }

  private class Replier extends Thread
  {
    Request request = null;
    Semaphore semaphore = new Semaphore();
    int n = 0;

    public Replier(int n)
    {
      this.n = n;
    }

    public Semaphore getSemaphore()
    {
      return semaphore;
    }

    public synchronized void setRequest(Request request)
    {
      this.request = request;
    }

    public void run()
    {
      for (int i = 0; i < n; i++)
      {
        semaphore.waitHere();
        synchronized (this)
        {
          if (request == null)
          {
            System.err.println("request == null, n=" + n);
            System.exit(-1);
          }
          semaphore.reset();
          Semaphore sem = request.getSemaphore();
          request.setReply(new Reply(request.getN()));
          request = null;
          sem.notifySingleWaiter();
        }
      }
    }
  }


  private class Semaphore
  {
    boolean notified = false;

    public Semaphore()
    {
    }

    public synchronized void waitHere()
    {
      if (!notified)
      {
        try
        {
          wait();
        } catch (Exception ignored)
        {
        }
      }
    }

    public synchronized void waitHere(long ms)
    {
      if (!notified)
      {
        try
        {
          wait(ms);
        } catch (Exception ignored)
        {
        }
      }
    }

    public synchronized void notifySingleWaiter()
    {
      notified = true;
      notify();
    }

    public synchronized void reset()
    {
      notified = false;
    }
  }

  private class RequestFactory
  {
    int type = 0;
    int n = 0;

    public RequestFactory(int type)
    {
      this.type = type;
    }

    public Request createRequest()
    {
      Request r = null;
      switch (type)
      {
        case 0:
          r = new URequest(++n);
          break;
        case 1:
          r = new VRequest(++n);
          break;
        case 2:
          r = new SRequest(++n);
          break;
      }
      return r;
    }

  }

  private interface Request
  {
    public int getN();

    public Reply getReply();

    public void setReply(Reply reply);

    public Semaphore getSemaphore();
  }

  private class URequest implements Request
  {
    int n = 0;
    Reply reply = null;
    Semaphore semaphore = new Semaphore();

    public URequest(int n)
    {
      this.n = n;
    }

    public int getN()
    {
      return n;
    }

    public Reply getReply()
    {
      return reply;
    }

    public void setReply(Reply reply)
    {
      this.reply = reply;
    }

    public Semaphore getSemaphore()
    {
      return semaphore;
    }
  }

  private class VRequest implements Request
  {
    int n = 0;
    volatile Reply reply = null;
    Semaphore semaphore = new Semaphore();

    public VRequest(int n)
    {
      this.n = n;
    }

    public int getN()
    {
      return n;
    }

    public Reply getReply()
    {
      return reply;
    }

    public void setReply(Reply reply)
    {
      this.reply = reply;
    }

    public Semaphore getSemaphore()
    {
      return semaphore;
    }
  }

  private class SRequest implements Request
  {
    int n = 0;
    Reply reply = null;
    Semaphore semaphore = new Semaphore();

    public SRequest(int n)
    {
      this.n = n;
    }

    public int getN()
    {
      return n;
    }

    public synchronized Reply getReply()
    {
      return reply;
    }

    public synchronized void setReply(Reply reply)
    {
      this.reply = reply;
    }

    public Semaphore getSemaphore()
    {
      return semaphore;
    }
  }

  private class Reply
  {
    int n = 0;
    boolean ok = false;
    Exception exception = null;
    boolean timeout = false;
    int requestNumber = 0;
    transient byte[] b = new byte[1024];

    public Reply(int n)
    {
      this.n = n;
    }
  }
}

---------- END SOURCE ----------

CUSTOMER SUBMITTED WORKAROUND :
None. Nothing works to avoid that.

Release Regression From : 5.0u7
The above release value was the last known release where this 
bug was not reproducible. Since then there has been a regression.

Comments
SUGGESTED FIX Change "if (!notified)" to "while (!notified)"
30-07-2006

EVALUATION The problem here is nothing to do with the Java Memory Model but is a simple synchronization error in the Object.wait() logic. Object.wait() must always be called in a loop, testing the condition being waited for. See the 1.5 javadocs for Object.wait(), the Java Language Specification Third Edition Section 17.8, or any decent text on concurrent programming in Java. The test program uses paired producer/consumer threads that interact via a logical one-slot buffer. The producer creates a request, signals that a request is available, waits for a reply then processes the reply. The consumer waits for a request, creates a reply and signals that the reply is available. The two threads work in lock-step, each doing one thing then waiting for the other. (Presumably this is a stripped down test case as the use of different threads serves no purpose under such conditions.) The synchronization between the producer and consumer is done via what has been a termed a "Semaphore". In fact the class Semaphore does not define a semaphore - the key attribute of a semaphore is that the method that waits for a permit consumes the permit, which is not what happens in this class. This class "Semaphore" implements a simple gate: when the gate is in the notified state the waitHere method does not block, while when not notified it will. The notifySingleWaiter method opens the gate - but only allows a single blocked waiter to wake up. The reset method closes the gate again. The timed waitHere method is unusable because you could never tell if you timed out or were notified - a method of this form can be used as a time-in method (wait for condition X but if after time Y elapses the condition doesn't hold then proceed regardless) but not a time-out method (wait for condition X but if time Y elapses then abort). While this "Semaphore" class is extremely fragile for general use it would be functional in this scenario if not for one thing. The waitHere method only checks the notified state upon method entry, not after returning from the Object.wait call: if (!notified) { try { wait(); } catch (Exception ignored) { } This is incorrect. If you consult any decent text on concurrent programming in Java you will find that wait() must always be used with the condition being waited for tested in a loop ie: while (!notified) wait(); The reasons for this are three-fold in general: - that thread that gets notified may not have been waiting for the condition change that was signalled - by the time the thread reacquires the monitor lock the condition may have changed again - the return from wait() was a spurious wakeup The code is encountering spurious (ie not caused directly by a notify() or notifyAll()) returns from the wait() method. Simply changing the "if" to a "while" fixes the problem. The validity of spurious returns from Object.wait() was not documented prior to Java 1.5 but now it is. There are differences between the Java 1.5 and 6.0 implementations of low-level synchronization and thread creation that now allow spurious wakeups in certain circumstances. This is a side-effect of using more highly performing asynchronous coordination algorithms between threads.
30-07-2006