Simplifies BrowserManagerServer based on the premise that the cleanup thread is the only guy who *ever* does any cleanup.
Review by: zundel
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@1864 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/junit/remote/BrowserManagerProcess.java b/user/src/com/google/gwt/junit/remote/BrowserManagerProcess.java
index 781c9b3..3d7f21d 100644
--- a/user/src/com/google/gwt/junit/remote/BrowserManagerProcess.java
+++ b/user/src/com/google/gwt/junit/remote/BrowserManagerProcess.java
@@ -47,20 +47,21 @@
/**
* Kills the child process when fired, unless it is no longer the active
- * {@link BrowserManagerProcess#killTask} in its outer ProcessManager.
+ * {@link BrowserManagerProcess#killTask}.
*/
private final class KillTask extends TimerTask {
@Override
public void run() {
synchronized (BrowserManagerProcess.this) {
/*
- * CORNER CASE: Verify we're still the active KillTask, because it's
- * possible we were bumped out by a keepAlive call after our execution
- * started but before we could grab the lock.
+ * Verify we're still the active KillTask! If we're not the active
+ * killTask, it means we've been rescheduled and a newer kill timer is
+ * active.
*/
- if (killTask == this) {
- logger.info("Timeout expired for task: " + token);
- doKill();
+ if (killTask == this && !deadOrDying) {
+ logger.info("Timeout expired for: " + token);
+ process.destroy();
+ deadOrDying = true;
}
}
}
@@ -69,24 +70,22 @@
private static final Logger logger = Logger.getLogger(BrowserManagerProcess.class.getName());
/**
- * Exit callback when the process exits.
+ * Compute elapsed time.
+ *
+ * @param startTime the time the process started
+ * @return returns a string representing the number of seconds elapsed since
+ * the process started.
*/
- private final ProcessExitCb cb;
+ private static String getElapsed(long intervalMs) {
+ NumberFormat nf = NumberFormat.getNumberInstance();
+ nf.setMaximumFractionDigits(3);
+ return nf.format(intervalMs / 1000.0);
+ }
/**
- * Set to 'true' when the process exits.
+ * Set to 'true' when the process exits or starts being killed.
*/
- private boolean exited = false;
-
- /**
- * Set to the exitValue() of the process when it actually exits.
- */
- private int exitValue = -1;
-
- /**
- * The key associated with <code>process</code>.
- */
- private final int token;
+ private boolean deadOrDying = false;
/**
* If non-null, the active TimerTask which will kill <code>process</code>
@@ -100,16 +99,16 @@
private final Process process;
/**
- * Time the exec'ed child actually started.
- */
- private final long startTime;
-
- /**
* Timer instance passed in from BrowserManagerServer.
*/
private final Timer timer;
/**
+ * The key associated with <code>process</code>.
+ */
+ private final int token;
+
+ /**
* Constructs a new ProcessManager for the specified process.
*
* @param timer timer passed in from BrowserManagerServer instance.
@@ -118,30 +117,26 @@
* @param initKeepAliveMs the initial time to wait before killing
* <code>process</code>
*/
- BrowserManagerProcess(ProcessExitCb cb, Timer timer, final int token,
- final Process process, long initKeepAliveMs) {
- this.cb = cb;
- this.timer = timer;
+ public BrowserManagerProcess(final ProcessExitCb cb, Timer timer,
+ final int token, final Process process, long initKeepAliveMs) {
this.process = process;
+ this.timer = timer;
this.token = token;
- schedule(initKeepAliveMs);
- startTime = System.currentTimeMillis();
+ final long startTime = System.currentTimeMillis();
Thread cleanupThread = new Thread() {
@Override
public void run() {
- try {
- exitValue = process.waitFor();
- if (cleanupBrowser() != 0) {
- logger.warning("Browser " + token + "exited with bad status: "
- + exitValue);
- } else {
- logger.info("Browser " + token + " process exited normally. "
- + getElapsed() + " milliseconds.");
+ while (true) {
+ try {
+ int exitValue = process.waitFor();
+ doCleanup(cb, exitValue, token, System.currentTimeMillis()
+ - startTime);
+ return;
+ } catch (InterruptedException e) {
+ logger.log(Level.WARNING,
+ "Interrupted waiting for process exit of: " + token, e);
}
- } catch (InterruptedException e) {
- logger.log(Level.WARNING, "Couldn't wait for process exit. token: "
- + token, e);
}
}
};
@@ -149,45 +144,7 @@
cleanupThread.setDaemon(true);
cleanupThread.setName("Browser-" + token + "-Wait");
cleanupThread.start();
- }
-
- /**
- * Kills the managed process.
- *
- * @return the exit value of the task.
- */
- public int doKill() {
-
- boolean doCleanup = false;
- synchronized (this) {
- if (!exited) {
- logger.info("Killing browser process for " + this.token);
- process.destroy();
-
- // Wait for the process to exit.
- try {
- exitValue = process.waitFor();
- doCleanup = true;
- } catch (InterruptedException ie) {
- logger.severe("Interrupted waiting for browser " + token
- + " exit during kill.");
- }
- }
- }
-
- // Run cleanupBrowser() outside the critical section.
- if (doCleanup) {
- if (cleanupBrowser() != 0) {
- logger.warning("Kill Browser " + token + "exited with bad status: "
- + exitValue);
-
- } else {
- logger.info("Kill Browser " + token + " process exited normally. "
- + getElapsed() + " milliseconds.");
- }
- }
-
- return exitValue;
+ keepAlive(initKeepAliveMs);
}
/**
@@ -199,88 +156,47 @@
* @return <code>true</code> if the process was successfully kept alive,
* <code>false</code> if the process is already dead.
*/
- public boolean keepAlive(long keepAliveMs) {
- synchronized (this) {
- try {
- /*
- * See if the managed process is still alive. WEIRD: The only way to
- * check the process's liveness appears to be asking for its exit status
- * and seeing whether it throws an IllegalThreadStateException.
- */
- process.exitValue();
- } catch (IllegalThreadStateException e) {
- // The process is still alive.
- schedule(keepAliveMs);
- return true;
- }
+ public synchronized boolean keepAlive(long keepAliveMs) {
+ assert (keepAliveMs > 0);
+ if (!deadOrDying) {
+ killTask = new KillTask();
+ timer.schedule(killTask, keepAliveMs);
+ return true;
}
-
- // The process is dead already; perform cleanup.
- cleanupBrowser();
return false;
}
/**
- * Routine that informs the BrowserManagerServer of the exit status once and
- * only once.
- *
- * This should be called WITHOUT the lock on BrowserManagerProcess.this being
- * held.
- *
- * @return The exit value returned by the process when it exited.
+ * Kills the underlying browser process.
*/
- private int cleanupBrowser() {
- boolean doCb = false;
+ public synchronized void killBrowser() {
+ if (!deadOrDying) {
+ process.destroy();
+ deadOrDying = true;
+ }
+ }
+
+ /**
+ * Cleans up when the underlying process terminates. The lock must not be held
+ * when calling this method or deadlock could result.
+ *
+ * @param cb the callback to fire
+ * @param exitValue the exit value of the process
+ * @param token the id of this browser instance
+ * @param startTime the time the process started
+ */
+ private void doCleanup(ProcessExitCb cb, int exitValue, int token,
+ long intervalMs) {
synchronized (this) {
- if (!exited) {
- exited = true;
- exitValue = process.exitValue();
- // Stop the timer for this thread.
- schedule(0);
- doCb = true;
- }
+ deadOrDying = true;
}
-
- /*
- * Callback must occur without holding my own lock. This is because the
- * callee will try to acquire the lock on
- * BrowserManagerServer.processByToken. If another thread already has that
- * lock and is tries to lock me at the same time, a deadlock would ensure.
- */
- if (doCb) {
- cb.childExited(token, exitValue);
+ if (exitValue != 0) {
+ logger.warning("Browser: " + token + " exited with bad status: "
+ + exitValue);
+ } else {
+ logger.info("Browser: " + token + " process exited normally after "
+ + getElapsed(intervalMs) + "s");
}
-
- return exitValue;
- }
-
- /**
- * Compute elapsed time.
- *
- * @return returns a string representing the number of seconds elapsed since
- * the process started.
- */
- private synchronized String getElapsed() {
- NumberFormat nf = NumberFormat.getNumberInstance();
- nf.setMaximumFractionDigits(3);
- return nf.format((System.currentTimeMillis() - startTime) / 1000.0);
- }
-
- /**
- * Cancels any existing kill task and optionally schedules a new one to run
- * <code>keepAliveMs</code> from now.
- *
- * @param keepAliveMs if > 0, schedules a new kill task to run in keepAliveMs
- * milliseconds; if <= 0, a new kill task is not scheduled.
- */
- private void schedule(long keepAliveMs) {
- if (killTask != null) {
- killTask.cancel();
- killTask = null;
- }
- if (keepAliveMs > 0) {
- killTask = new KillTask();
- timer.schedule(killTask, keepAliveMs);
- }
+ cb.childExited(token, exitValue);
}
}
diff --git a/user/src/com/google/gwt/junit/remote/BrowserManagerServer.java b/user/src/com/google/gwt/junit/remote/BrowserManagerServer.java
index 9a39822..4459938 100644
--- a/user/src/com/google/gwt/junit/remote/BrowserManagerServer.java
+++ b/user/src/com/google/gwt/junit/remote/BrowserManagerServer.java
@@ -238,14 +238,13 @@
}
BrowserManagerProcess process = processByToken.get(token);
if (process != null) {
- logger.fine("Killing browser. token=" + token);
- process.doKill();
+ logger.info("Client kill for active browser: " + token);
+ process.killBrowser();
} else if (launchCommandQueue.contains(new LaunchCommand(token))) {
launchCommandQueue.remove(new LaunchCommand(token));
- logger.info(token + " removed from delayed launch queue.");
+ logger.info("Client kill for waiting browser: " + token);
} else {
- logger.fine("No action taken. Browser not active for token " + token
- + ".");
+ logger.info("Client kill for inactive browser: " + token);
}
}
}
@@ -276,7 +275,7 @@
return myToken;
}
} catch (IOException e) {
- logger.log(Level.SEVERE, "Error launching browser" + launchCmd
+ logger.log(Level.SEVERE, "Error launching browser '" + launchCmd
+ "' for '" + url + "'", e);
throw new RuntimeException("Error launching browser '" + launchCmd
+ "' for '" + url + "'", e);
@@ -341,7 +340,6 @@
* (Assumes that code is already synchronized by processBytoken)
*/
private void launchDelayedCommand() {
-
if (!serializeFlag || !processByToken.isEmpty()) {
// No need to launch if serialization is off or
// something is already running
@@ -352,22 +350,18 @@
// successfully.
while (!launchCommandQueue.isEmpty()) {
LaunchCommand lc = launchCommandQueue.remove();
-
try {
execChild(lc.token, lc.url, lc.keepAliveMsecs);
// No exception? Great!
- logger.info("Started delayed browser " + lc.token);
+ logger.info("Started delayed browser: " + lc.token);
return;
} catch (IOException e) {
-
- logger.log(Level.SEVERE, "Error launching browser" + launchCmd
+ logger.log(Level.SEVERE, "Error launching browser '" + launchCmd
+ "' for '" + lc.url + "'", e);
-
throw new RuntimeException("Error launching browser '" + launchCmd
+ "' for '" + lc.url + "'", e);
}
-
// If an exception occurred, keep pulling cmds off the queue.
}
}
