ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ste...@apache.org
Subject svn commit: r465013 - in /ant/core/trunk: WHATSNEW src/main/org/apache/tools/ant/taskdefs/ExecuteJava.java src/main/org/apache/tools/ant/taskdefs/ExecuteWatchdog.java src/main/org/apache/tools/ant/util/Watchdog.java
Date Tue, 17 Oct 2006 19:28:21 GMT
Author: stevel
Date: Tue Oct 17 12:28:20 2006
New Revision: 465013

URL: http://svn.apache.org/viewvc?view=rev&rev=465013
Log:
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.

Modified:
    ant/core/trunk/WHATSNEW
    ant/core/trunk/src/main/org/apache/tools/ant/taskdefs/ExecuteJava.java
    ant/core/trunk/src/main/org/apache/tools/ant/taskdefs/ExecuteWatchdog.java
    ant/core/trunk/src/main/org/apache/tools/ant/util/Watchdog.java

Modified: ant/core/trunk/WHATSNEW
URL: http://svn.apache.org/viewvc/ant/core/trunk/WHATSNEW?view=diff&rev=465013&r1=465012&r2=465013
==============================================================================
--- ant/core/trunk/WHATSNEW (original)
+++ ant/core/trunk/WHATSNEW Tue Oct 17 12:28:20 2006
@@ -12,6 +12,10 @@
 
 * 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 @@
   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:
 -----------

Modified: ant/core/trunk/src/main/org/apache/tools/ant/taskdefs/ExecuteJava.java
URL: http://svn.apache.org/viewvc/ant/core/trunk/src/main/org/apache/tools/ant/taskdefs/ExecuteJava.java?view=diff&rev=465013&r1=465012&r2=465013
==============================================================================
--- ant/core/trunk/src/main/org/apache/tools/ant/taskdefs/ExecuteJava.java (original)
+++ ant/core/trunk/src/main/org/apache/tools/ant/taskdefs/ExecuteJava.java Tue Oct 17 12:28:20
2006
@@ -50,8 +50,8 @@
     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;
 
     /**

Modified: ant/core/trunk/src/main/org/apache/tools/ant/taskdefs/ExecuteWatchdog.java
URL: http://svn.apache.org/viewvc/ant/core/trunk/src/main/org/apache/tools/ant/taskdefs/ExecuteWatchdog.java?view=diff&rev=465013&r1=465012&r2=465013
==============================================================================
--- ant/core/trunk/src/main/org/apache/tools/ant/taskdefs/ExecuteWatchdog.java (original)
+++ ant/core/trunk/src/main/org/apache/tools/ant/taskdefs/ExecuteWatchdog.java Tue Oct 17
12:28:20 2006
@@ -45,13 +45,13 @@
     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 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 @@
     /**
      * reset the monitor flag and the process.
      */
-    protected void cleanUp() {
+    protected synchronized void cleanUp() {
         watch = false;
         process = null;
     }
@@ -148,7 +148,7 @@
      * @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);

Modified: ant/core/trunk/src/main/org/apache/tools/ant/util/Watchdog.java
URL: http://svn.apache.org/viewvc/ant/core/trunk/src/main/org/apache/tools/ant/util/Watchdog.java?view=diff&rev=465013&r1=465012&r2=465013
==============================================================================
--- ant/core/trunk/src/main/org/apache/tools/ant/util/Watchdog.java (original)
+++ ant/core/trunk/src/main/org/apache/tools/ant/util/Watchdog.java Tue Oct 17 12:28:20 2006
@@ -33,7 +33,16 @@
 
     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 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 @@
      * @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 @@
      * @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();



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org


Mime
View raw message