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.
+ * [EMAIL PROTECTED]
+ */
+ 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: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
