Having learned about how the java memory model really works, I have had a quick code review of the threading here.

1. stuff that is shared read is always marked volatile, to avoid being compiled out.

2. added more synchronization when appropriate. 

I make no claims as to thread safety here, as I was never that good at formal proofs of correctness.

git-svn-id: https://svn.apache.org/repos/asf/ant/core/trunk@465013 13f79535-47bb-0310-9956-ffa450edef68

WHATSNEW

@@ -12,6 +12,10 @@ Other changes:
 
 * Upgraded XML API and parser to Xerces 2.8.1
 
+* A code review of some threaded logic has tightened up the synchronization
+  of Watchdog, ExecuteWatchdog and ExecuteJava, which could reduce the occurence
+  of race conditions here, especially on Java1.5+.
+
 Changes from Ant 1.7.0Beta2 to Ant 1.7.0Beta3 
 =============================================
 
@@ -24,6 +28,13 @@ Changes that could break older environments:
   the java class file. Bugzilla report 33604.
 
 * Defer reference process. Bugzilla 36955, 34458, 37688.
+  This may break build files in which a reference was set in a target which was
+  never executed. Historically, Ant would set the reference early on, during parse
+  time, so the datatype would be defined. Now it requires the reference to have
+  been in a bit of the build file which was actually executed. If you get
+  an error about an undefined reference, locate the reference and move it somewhere
+  where it is used, or fix the depends attribute of the target in question to
+  depend on the target which defines the reference/datatype.
 
 Fixed bugs:
 -----------

src/main/org/apache/tools/ant/taskdefs/ExecuteJava.java

@@ -50,8 +50,8 @@ public class ExecuteJava implements Runnable, TimeoutObserver {
     private Permissions  perm = null;
     private Method main = null;
     private Long timeout = null;
-    private Throwable caught = null;
-    private boolean timedOut = false;
+    private volatile Throwable caught = null;
+    private volatile boolean timedOut = false;
     private Thread thread = null;
 
     /**

src/main/org/apache/tools/ant/taskdefs/ExecuteWatchdog.java

@@ -45,13 +45,13 @@ public class ExecuteWatchdog implements TimeoutObserver {
     private Process process;
 
     /** say whether or not the watchdog is currently monitoring a process */
-    private boolean watch = false;
+    private volatile boolean watch = false;
 
     /** exception that might be thrown during the process execution */
     private Exception caught = null;
 
     /** say whether or not the process was killed due to running overtime */
-    private boolean     killedProcess = false;
+    private volatile boolean killedProcess = false;
 
     /** will tell us whether timeout has occurred */
     private Watchdog watchdog;
@@ -103,15 +103,15 @@ public class ExecuteWatchdog implements TimeoutObserver {
      */
     public synchronized void stop() {
         watchdog.stop();
-        watch = false;
-        process = null;
+        cleanUp();
     }
 
     /**
      * Called after watchdog has finished.
+     * This can be called in the watchdog thread
      * @param w the watchdog
      */
-    public void timeoutOccured(Watchdog w) {
+    public synchronized void timeoutOccured(Watchdog w) {
         try {
             try {
                 // We must check if the process was not stopped
@@ -135,7 +135,7 @@ public class ExecuteWatchdog implements TimeoutObserver {
     /**
      * reset the monitor flag and the process.
      */
-    protected void cleanUp() {
+    protected synchronized void cleanUp() {
         watch = false;
         process = null;
     }
@@ -148,7 +148,7 @@ public class ExecuteWatchdog implements TimeoutObserver {
      * @throws  BuildException  a wrapped exception over the one that was
      * silently swallowed and stored during the process run.
      */
-    public void checkException() throws BuildException {
+    public synchronized void checkException() throws BuildException {
         if (caught != null) {
             throw new BuildException("Exception in ExecuteWatchdog.run: "
                                      + caught.getMessage(), caught);

src/main/org/apache/tools/ant/util/Watchdog.java

@@ -33,7 +33,16 @@ public class Watchdog implements Runnable {
 
     private Vector observers = new Vector(1);
     private long timeout = -1;
-    private boolean stopped = false;
+    /**
+     * marked as volatile to stop the compiler caching values or (in java1.5+,
+     * reordering access)
+     */
+    private volatile boolean stopped = false;
+    /**
+     * Error string.
+     * {@value}
+     */
+    public static final String ERROR_INVALID_TIMEOUT = "timeout less than 1.";
 
     /**
      * Constructor for Watchdog.
@@ -41,7 +50,7 @@ public class Watchdog implements Runnable {
      */
     public Watchdog(long timeout) {
         if (timeout < 1) {
-            throw new IllegalArgumentException("timeout less than 1.");
+            throw new IllegalArgumentException(ERROR_INVALID_TIMEOUT);
         }
         this.timeout = timeout;
     }
@@ -51,6 +60,7 @@ public class Watchdog implements Runnable {
      * @param to the timeout observer to add.
      */
     public void addTimeoutObserver(TimeoutObserver to) {
+        //no need to synchronize, as Vector is always synchronized
         observers.addElement(to);
     }
 
@@ -59,11 +69,13 @@ public class Watchdog implements Runnable {
      * @param to the timeout observer to remove.
      */
     public void removeTimeoutObserver(TimeoutObserver to) {
+        //no need to synchronize, as Vector is always synchronized
         observers.removeElement(to);
     }
 
     /**
      * Inform the observers that a timeout has occurred.
+     * This happens in the watchdog thread.
      */
     protected final void fireTimeoutOccured() {
         Enumeration e = observers.elements();