From 7c58ba3edd10c9aea168e8d98543623e38b35d6a Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Tue, 17 Oct 2006 19:28:20 +0000 Subject: [PATCH] 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 | 11 +++++++++++ .../apache/tools/ant/taskdefs/ExecuteJava.java | 4 ++-- .../tools/ant/taskdefs/ExecuteWatchdog.java | 14 +++++++------- src/main/org/apache/tools/ant/util/Watchdog.java | 16 ++++++++++++++-- 4 files changed, 34 insertions(+), 11 deletions(-) diff --git a/WHATSNEW b/WHATSNEW index 8420cb0bc..a8971e7cd 100644 --- a/WHATSNEW +++ b/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: ----------- diff --git a/src/main/org/apache/tools/ant/taskdefs/ExecuteJava.java b/src/main/org/apache/tools/ant/taskdefs/ExecuteJava.java index 064b668d8..e0a4b5046 100644 --- a/src/main/org/apache/tools/ant/taskdefs/ExecuteJava.java +++ b/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; /** diff --git a/src/main/org/apache/tools/ant/taskdefs/ExecuteWatchdog.java b/src/main/org/apache/tools/ant/taskdefs/ExecuteWatchdog.java index 02a6621ca..81c791d47 100644 --- a/src/main/org/apache/tools/ant/taskdefs/ExecuteWatchdog.java +++ b/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); diff --git a/src/main/org/apache/tools/ant/util/Watchdog.java b/src/main/org/apache/tools/ant/util/Watchdog.java index e8dcd1024..826a8fd75 100644 --- a/src/main/org/apache/tools/ant/util/Watchdog.java +++ b/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();