public inbox for mauve-patches@sourceware.org
 help / color / mirror / Atom feed
* RFC: Harness with no busy-waiting
@ 2006-06-26 19:28 Anthony Balkissoon
  2006-06-27  8:41 ` Mark Wielaard
  0 siblings, 1 reply; 6+ messages in thread
From: Anthony Balkissoon @ 2006-06-26 19:28 UTC (permalink / raw)
  To: mark, mauve-patches

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

This removes the busy waiting from the Harness and replaces it with
blocking read() calls using Sockets with specified timeouts.

This is RFC because I'd like it to be tested to see if it speeds things
up as much as Mark's workaround before it is checked in.

--Tony

2006-06-26  Anthony Balkissoon  <abalkiss@redhat.com>

	* Harness.java:
	(bcp_timeout): Removed this field.
	(server): New field.
	(client): New field.
	(runner_watcher): Removed this field.
	(testIsHung): Removed this field.
	(runner_lock): Removed this field.
	(getBootClassPath): Removed the TimeoutWatcher here, just assume this 
	tiny program doesn't hang. Also remove the workaround for busy-waiting.
	(finalize): Close the server socket.
	(initProcess): Setup the server socket and client socket. Set the 
	runner_in and runner_out streams to be from the getInputStream() and 
	getOutputStream() methods for the client socket rather than from the 
	forked process. Removed the references to runner_watcher.
	(runTest): Redesigned to use a blocking readLine() with a specified 
	timeout, rather than polling for input with a separate TimeoutWatcher.
	(TimeoutWatcher): Removed this class.
	* RunnerProcess.java: Replaced all references to System.out with the 
	new PrintWriter "out".  Also:
	(FAILED_TO_LOAD_DESCRIPTION): New field.
	(socket): Likewise.
	(out): Likewise.
	(in): Likewise.
	(main): Setup the Socket-based communications with Harness. Return if 
	testname is null.
	(runtest): Set description at the start to avoid NPE later. If the test
	throws a Throwable when loading set the description to
	FAILED_TO_LOAD_DESCRIPTION and print an error message.

[-- Attachment #2: Sockets2.diff --]
[-- Type: text/x-patch, Size: 29909 bytes --]

Index: Harness.java
===================================================================
RCS file: /cvs/mauve/mauve/Harness.java,v
retrieving revision 1.15
diff -u -r1.15 Harness.java
--- Harness.java	24 Jun 2006 17:33:22 -0000	1.15
+++ Harness.java	26 Jun 2006 19:02:50 -0000
@@ -31,10 +31,13 @@
 import java.io.FileReader;
 import java.io.IOException;
 import java.io.InputStreamReader;
+import java.io.InterruptedIOException;
 import java.io.OutputStream;
 import java.io.PrintWriter;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
+import java.net.ServerSocket;
+import java.net.Socket;
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.util.StringTokenizer;
@@ -79,9 +82,6 @@
   // The location of the eclipse-ecj.jar file
   private static String ecjJarLocation = null;
   
-  // How long the bootclasspath finder program can run before termination
-  private static long bcp_timeout = 60000;
-  
   // How long a test may run before it is considered hung
   private static long runner_timeout = 60000;
 
@@ -123,6 +123,12 @@
   // All the tests that were explicitly excluded via the -exclude option
   private static Vector excludeTests = new Vector();
   
+  // A ServerSocket to use for communication with RunnerProcess
+  private static ServerSocket server = null;
+  
+  // A Socket to use for communication with RunnerProcess
+  private static Socket client = null;
+  
   // A way to speak to the runner process
   private static PrintWriter runner_out = null;
 
@@ -135,15 +141,6 @@
   // The process that will run the tests for us
   private static Process runnerProcess = null;
 
-  // A watcher to determine if runnerProcess is hung
-  private static TimeoutWatcher runner_watcher = null;
-  
-  // A flag indicating whether or not runnerProcess is hung
-  private static boolean testIsHung = false;
-  
-  // A lock used for synchronizing access to testIsHung
-  private static Object runner_lock = new Object();
-  
   // The arguments used when this Harness was invoked, we use this to create an
   // appropriate RunnerProcess
   private static String[] harnessArgs = null;
@@ -484,52 +481,23 @@
         new BufferedReader
         (new InputStreamReader(p.getInputStream()));
       String bcpOutput = null;
-      // Create a timer to watch this new process.
-      TimeoutWatcher tw = new TimeoutWatcher(bcp_timeout);
-      tw.start();
       while (true)
         {
-          // If for some reason the process hangs, return null, indicating we
-          // cannot auto-detect the bootclasspath.
-          if (testIsHung)
-            {
-              synchronized (runner_lock)
-              {
-                testIsHung = false;
-              }
-              br.close();
-              p.destroy();
-              return null;
-            }
           if (br.ready())
             {
               bcpOutput = br.readLine();
               if (bcpOutput == null)
                 {
                   // This means the auto-detection failed.
-                  tw.stop();
                   return null;
                 }
               if (bcpOutput.startsWith("BCP_FINDER:"))
                 {
-                  tw.stop();
                   return bcpOutput.substring(11);
                 }
               else
                 System.out.println(bcpOutput);
             }
-	  else
-	    {
-	      // FIXME - Quickworkaround for busy-waiting, needs proper FIX.
-	      try
-	        {
-		  Thread.sleep(100);
-		}
-	      catch (InterruptedException ie)
-	        {
-		  // ignore
-		}
-	    }
         }
     }
     catch (IOException ioe)
@@ -634,6 +602,7 @@
     try
       {
         runTest("_dump_data_");
+        server.close();
         runner_in.close();
         runner_in_err.close();
         runner_out.close();        
@@ -662,30 +631,33 @@
     try
       {
         // Exec the process and set up in/out communications with it.
-        runnerProcess = Runtime.getRuntime().exec(sb.toString());
-        runner_out = new PrintWriter(runnerProcess.getOutputStream(), true);
+        server = new ServerSocket(4444);
+        server.setSoTimeout(30000);
+        runnerProcess = Runtime.getRuntime().exec(sb.toString());        
+        client = server.accept();
+        client.setSoTimeout((int)runner_timeout);
+        runner_out = new PrintWriter(client.getOutputStream(), true);
         runner_in = 
           new BufferedReader
-          (new InputStreamReader(runnerProcess.getInputStream()));
+          (new InputStreamReader(client.getInputStream()));
         runner_in_err = 
             new BufferedReader
             (new InputStreamReader(runnerProcess.getErrorStream()));                
       }
+    catch (InterruptedIOException intioe)
+    {
+      System.err.println ("Harness timed out waiting for connection from " +
+            "RunnerProcess.  Exit.");
+      System.exit(1);
+    }
     catch (IOException e)
       {
         System.err.println("Problems invoking RunnerProcess: " + e);
         System.exit(1);
       }
-
-    // Create a timer to watch this new process.  After confirming the
-    // RunnerProcess started properly, we create a new runner_watcher 
-    // because it may be a while before we run the next test (due to 
-    // preprocessing and compilation) and we don't want the runner_watcher
-    // to time out.
-    runner_watcher = new TimeoutWatcher(runner_timeout);    
+    
+    // Confirm that the RunnerProcess was started properly.
     runTest("_confirm_startup_");
-    runner_watcher.stop();
-    runner_watcher = new TimeoutWatcher(runner_timeout);
   }
   
   /**
@@ -773,146 +745,116 @@
   private static void runTest(String testName)
   {
     String tn = stripPrefix(testName.replace(File.separatorChar, '.'));
-    String outputFromTest;
-    StringBuffer sb = new StringBuffer();
+    String outputFromTest = "";
     boolean invalidTest = false;
-    int temp = -1;
-
-    // Start the timeout watcher
-    if (runner_watcher.isAlive())
-      runner_watcher.reset();
-    else
-      runner_watcher.start();
+    int temp = -99;
     
     // Tell the RunnerProcess to run test with name testName
     runner_out.println(testName);
     
     while (true)
       {
-        // This while loop polls for output from the test process and 
-        // passes it to System.out unless it is the signal that the 
-        // test finished properly.  Also checks to see if the watcher
-        // thread has declared the test hung and if so ends the process.
-        if (testIsHung)
+        try
+        {
+          // This readLine() will timeout after runner_timeout milliseconds
+          // if nothing is received.
+          outputFromTest = runner_in.readLine();
+        }
+        catch (InterruptedIOException intioe)
+        {
+          // This means the readLine() timed out and we consider the test to be
+          // hung.
+          
+          // Setting temp to -1 is a flag that the test timed out
+          temp = -1;
+          try
           {
-            System.err.print(sb.toString());
-            if (testName.equals("_confirm_startup_"))
+            server.close();
+            runner_in.close();
+            runner_in_err.close();
+          }
+          catch (IOException ioe2)
+          {
+            System.err.println("Could not close old communication lines.  Exit.");
+            System.exit(1);
+          }
+          runner_out.close();
+          runnerProcess.destroy();
+          initProcess(harnessArgs);
+          break;
+        }
+        catch (IOException ioe)
+        {
+          System.err.println("Unexpected IO Error.  Exit.");
+          System.exit(1);
+        }
+        
+        // Okay, some text was received from the RunnerProcess, now let's
+        // process it.
+        if (outputFromTest.startsWith("RunnerProcess:"))
+          {
+            invalidTest = false;
+            // This means the RunnerProcess has sent us back some
+            // information. This could be telling us that a check() call
+            // was made and we should reset the timer, or that the
+            // test passed, or failed, or that it wasn't a test.
+            if (outputFromTest.endsWith("restart-timer"))
+              // Do nothing here, but the timeout on the readLine() was
+              // reset so the Harness knows the test isn't hung yet.
+              ;
+            else if (outputFromTest.endsWith("pass"))
               {
-                System.err.println("ERROR: Cannot create test runner process.  Exit");
-                System.exit(1);
+                temp = 0;
+                break;
               }
-            synchronized (runner_lock)
-            {
-              testIsHung = false;
-            }
-            try
+            else if (outputFromTest.indexOf("fail-") != -1)
               {
-                runner_in.close();
-                runner_in_err.close();
-                runner_out.close();
-                runnerProcess.destroy();
+                total_check_fails += Integer.parseInt(outputFromTest.substring(19));
+                temp = 1;
+                break;
               }
-            catch (IOException e)
+            else if (outputFromTest.endsWith("not-a-test"))
               {
-                System.err.println("Could not close the interprocess pipes.");
-                System.exit(- 1);
+                // Temporarily decrease the total number of tests,
+                // because it will be incremented later even
+                // though the test was not a real test.
+                invalidTest = true;
+                total_tests--;
+                temp = 0;
+                break;
               }
-            initProcess(harnessArgs);
-            break;
           }
-        try
-        {
-          if (runner_in_err.ready())
-            sb.append(runner_in_err.readLine() + "\n");
-          if (runner_in.ready())
-            {
-              outputFromTest = runner_in.readLine();              
-              if (outputFromTest.startsWith("RunnerProcess:"))
-                {
-                  invalidTest = false;
-                  // This means the RunnerProcess has sent us back some
-                  // information.  This could be telling us that a check() call
-                  // was made and we should reset the timer, or that the 
-                  // test passed, or failed, or that it wasn't a test.
-                  if (outputFromTest.endsWith("restart-timer"))
-                    runner_watcher.reset();
-                  else if (outputFromTest.endsWith("pass"))
-                    {
-                      temp = 0;
-                      break;
-                    }
-                  else if (outputFromTest.indexOf("fail-") != -1)
-                    {
-                      total_check_fails += 
-                        Integer.parseInt(outputFromTest.substring(19));
-                      temp = 1;
-                      break;
-                    }
-                  else if (outputFromTest.endsWith("not-a-test"))
-                    {
-                      // Temporarily decrease the total number of tests,
-                      // because it will be incremented later even 
-                      // though the test was not a real test.
-                      invalidTest = true;
-                      total_tests--;
-                      temp = 0;
-                      break;
-                    }                  
-                } 
-              else if (outputFromTest.equals("_startup_okay_") || 
-                  outputFromTest.equals("_data_dump_okay_"))
-                return;
-              else
-                // This means it was just output from the test, like a 
-                // System.out.println within the test itself, we should
-                // pass these on to stdout.
-                System.out.println(outputFromTest);
-            }
-          else
-            {
-	      if (sb.length() != 0)
-	        {
-                  System.err.print(sb.toString());
-                  sb = new StringBuffer();
-		}
-
-              // FIXME - Quickworkaround for busy-waiting, needs proper FIX.
-              try
-                {
-                  Thread.sleep(100);
-                }
-              catch (InterruptedException ie)
-                {
-                  // ignore
-                }
-            }
-        }
-        catch (IOException e)
-        {
-        }
+        else if (outputFromTest.equals("_startup_okay_")
+            || outputFromTest.equals("_data_dump_okay_"))
+          // These are just handshaking signals received from the RunnerProcess
+          // no action needs to be taken and we should exit this method.
+          return;
+        else
+          // This means it was just output from the test, like a
+          // System.out.println within the test itself, we should
+          // pass these on to stdout.
+          System.out.println(outputFromTest);
       }
-    System.err.print(sb.toString());
+    
     if (temp == -1)
-      {        
-        // This means the watcher thread had to stop the process 
-        // from running.  So this is a fail.
+      {
+        // This means the test timed out, which is a fail.
         if (verbose)
-          System.out.println("  FAIL: timed out. \nTEST FAILED: timeout " + 
-                             tn);
+          System.out.println("  FAIL: timed out. \nTEST FAILED: timeout " + tn);
         else
-        System.out.println("FAIL: " + tn
-                           + "\n  Test timed out.  Use -timeout [millis] " +
-                                "option to change the timeout value.");
+          System.out.println("FAIL: " + tn
+                             + "\n  Test timed out.  Use -timeout [millis] "
+                             + "option to change the timeout value.");
         
         total_test_fails++;
       }
     else
       total_test_fails += temp;
-    total_tests ++;
+    total_tests++;
     
     // If the test passed and the user wants to know about passes, tell them.
     if (showPasses && temp == 0 && !verbose && !invalidTest)
-      System.out.println ("PASS: "+tn);
+      System.out.println("PASS: " + tn);
   }  
   
   /**
@@ -1145,94 +1087,6 @@
         return x.startsWith(s);
       }
   }
-
-  /**
-   * This class is used for our timer to cancel tests that have hung.
-   * @author Anthony Balkissoon abalkiss at redhat dot com
-   *
-   */
-  private static class TimeoutWatcher implements Runnable
-  {
-    private long millisToWait;
-    private Thread watcherThread;
-    private boolean loop = true;
-    private boolean shouldContinue = true;
-    
-    /**
-     * Creates a new TimeoutWatcher that will wait for <code>millis</code>
-     * milliseconds once started.
-     * @param millis the number of milliseconds to wait before declaring the 
-     * test as hung
-     */
-    public TimeoutWatcher(long millis)
-    {
-      millisToWait = millis;
-      watcherThread = new Thread(this);
-    }
-    
-    /**
-     * Start the watcher thread, ie start the countdown.     
-     */
-    public void start()
-    {
-      watcherThread.start();      
-    }
-    
-    /**
-     * Stops the run() method.
-     *
-     */
-    public synchronized void stop()
-    {
-      shouldContinue = false;
-      notify();
-    }
-    
-    /**
-     * Return true if the watcher thread is currently counting down.
-     * @return true if the watcher thread is alive
-     */
-    public boolean isAlive()
-    {
-      return watcherThread.isAlive();
-    }
-    
-    /**
-     * Reset the counter and wait another <code>millisToWait</code>
-     * milliseconds before declaring the test as hung.     
-     */
-    public synchronized void reset()
-    {
-      loop = true;
-      notify();
-    }
-    
-    public synchronized void run()
-    {
-      Thread.currentThread().setPriority(Thread.MAX_PRIORITY);
-      while (loop && shouldContinue)
-        {
-          // We set loop to false here, it will get reset to true if 
-          // reset() is called from the main Harness thread.
-          loop = false;
-          try
-          {
-            wait(millisToWait);
-          }
-          catch (InterruptedException ie)
-          {}
-        }
-      if (shouldContinue)
-        {
-          // The test is hung, set testIsHung to true so the process will be 
-          // destroyed and restarted.      
-          synchronized (runner_lock)
-          {
-            testIsHung = true;
-          }
-        }
-    }
-  }
   
   /**
    * This tiny class is used for finding the bootclasspath of the VM used
Index: RunnerProcess.java
===================================================================
RCS file: /cvs/mauve/mauve/RunnerProcess.java,v
retrieving revision 1.8
diff -u -r1.8 RunnerProcess.java
--- RunnerProcess.java	19 Jun 2006 22:07:37 -0000	1.8
+++ RunnerProcess.java	26 Jun 2006 19:02:50 -0000
@@ -39,8 +39,10 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
+import java.io.PrintWriter;
 import java.io.Reader;
 import java.lang.reflect.Method;
+import java.net.Socket;
 import java.net.URL;
 import java.net.URLClassLoader;
 import java.util.Vector;
@@ -51,6 +53,9 @@
   // A description of files that are not tests
   private static final String NOT_A_TEST_DESCRIPTION = "not-a-test";
   
+  // A description of files that are not tests
+  private static final String FAILED_TO_LOAD_DESCRIPTION = "failed-to-load";
+  
   // Total number of harness.check calls since the last checkpoint
   private int count = 0;
   
@@ -106,6 +111,15 @@
   // The failure message for the last failing check()
   private String lastFailureMessage = null;
   
+  // A Socket to use for communication with Harness
+  private static Socket socket = null;
+  
+  // A PrintWriter to send data through the Socket to the Harness
+  private static PrintWriter out = null;
+  
+  // A BufferedReader to get data from the Harness
+  private static BufferedReader in = null;
+  
   protected RunnerProcess()
   {    
     try
@@ -132,9 +146,19 @@
     // The test that Harness wants us to run.
     String testname = null;
     
-    // This reader is used to get testnames from Harness
-    BufferedReader in = new BufferedReader(new InputStreamReader(System.in));
-    
+    // Setup the communications with the Harness
+    try
+    {
+      socket = new Socket("", 4444);
+      out = new PrintWriter(socket.getOutputStream(), true);
+      in = new BufferedReader(new InputStreamReader(socket.getInputStream()));
+    }
+    catch (IOException ioe)
+    {
+      System.err.println("Cannot create communication links between Harness " +
+            "and RunnerProcess.  Exit.");
+      System.exit(1);
+    }
     // Parse the arguments so we can create an appropriate RunnerProcess
     // to run the tests.
     for (int i = 0; i < args.length; i++)
@@ -195,15 +219,17 @@
         try
         {
           testname = in.readLine();
+          if (testname == null)
+            return;
           if (testname.equals("_dump_data_"))
             {
               if (useEMMA)
                 dumpCoverageData();
               else
-                System.out.println("_data_dump_okay_");
+                out.println("_data_dump_okay_");
             }
           else if (testname.equals("_confirm_startup_"))
-            System.out.println("_startup_okay_");
+            out.println("_startup_okay_");
           else
             {
               RunnerProcess harness = new RunnerProcess();
@@ -214,14 +240,14 @@
         {          
           String shortName = stripPrefix(testname);
           if (verbose)
-            System.out.println ("TEST: " + shortName + 
-                                "\n  FAIL: failed to load\n" +
-                                "TEST FAILED: failed to load "+ 
-                                shortName);
+            out.println ("TEST: " + shortName + 
+                         "\n  FAIL: failed to load\n" +
+                         "TEST FAILED: failed to load "+ 
+                         shortName);
           else
-            System.out.println("FAIL: " + stripPrefix(testname)
-                                 + ": failed to load");
-          System.out.println("RunnerProcess:fail-0");
+            out.println("FAIL: " + stripPrefix(testname)
+                        + ": failed to load");
+          out.println("RunnerProcess:fail-0");
         }
       }
   }
@@ -238,6 +264,7 @@
     System.runFinalization();
 
     currentResult = new TestResult(name.substring(12));
+    description = name;
 
     checkPoint(null);
 
@@ -256,6 +283,8 @@
       }
     catch (Throwable ex)
       {
+        description = FAILED_TO_LOAD_DESCRIPTION;
+        
         // Maybe the file was marked not-a-test, check that before we report
         // it as an error
         try
@@ -282,15 +311,16 @@
         {          
         }
         
-        String d = "FAIL: " + stripPrefix(name)
-                   + "uncaught exception when loading";
-        currentResult.addException(ex, "failed loading class ",
-                                   "couldn't load: " + name);
+        String d = "FAIL: " + stripPrefix(name);
+        currentResult.addException(ex, "failed loading class: " + name,
+                                   "Should the test be marked not-a-test?" );
         if (verbose || exceptions)
-          d += ": " + ex.toString();
+          d += " (while loading): " + ex.toString();
+        
+        out.println(d);
 
         if (exceptions)
-          ex.printStackTrace(System.out);
+          ex.printStackTrace(out);
         debug(ex);
         if (ex instanceof InstantiationException
             || ex instanceof IllegalAccessException)
@@ -303,25 +333,24 @@
     // If the harness started okay, now we run the test.
     if (t != null)
       {
-        description = name;
         try
           {
             if (verbose)
-              System.out.println("TEST: " + stripPrefix(name));
+              out.println("TEST: " + stripPrefix(name));
             t.test(this);
             removeSecurityManager();
           }
         catch (Throwable ex)
           {
             if (failures == 0 && !verbose)
-              System.out.println ("FAIL: " + stripPrefix(name) + ":");
+              out.println ("FAIL: " + stripPrefix(name) + ":");
             removeSecurityManager();
             String d = exceptionDetails(ex, name, exceptions);
             String r = getStackTraceString(ex, "          ");
             currentResult.addException(ex, d, r);
-            System.out.println(d);
+            out.println(d);
             if (exceptions)
-              System.out.println(getStackTraceString(ex, "   "));
+              out.println(getStackTraceString(ex, "   "));
             debug(ex);
             ++failures;
             ++total;
@@ -362,9 +391,12 @@
     // If the test wasn't a real test, return and tell Harness so.
     if (harness.description.equals(NOT_A_TEST_DESCRIPTION))
       {
-        System.out.println("RunnerProcess:not-a-test");
+        out.println("RunnerProcess:not-a-test");
         return;
       }
+    else if (harness.description.equals(FAILED_TO_LOAD_DESCRIPTION))
+      out.println("RunnerProcess:fail-0");
+    
     // Print out a summary.
     int temp = harness.done();
     // Print the report if necessary.
@@ -388,9 +420,9 @@
     // time (specified in the timeout variable) it will consider the test hung
     // and will terminate and restart this Process.
     if (temp == 0)
-      System.out.println ("RunnerProcess:pass");
+      out.println ("RunnerProcess:pass");
     else
-      System.out.println("RunnerProcess:fail-" + temp);
+      out.println("RunnerProcess:fail-" + temp);
   }
   
   private final String getDescription(StackTraceElement[] st)
@@ -439,7 +471,7 @@
     String desc = check2(false);    
     lastFailureMessage = "forced fail";
     currentResult.addFail(desc + " -- " +lastFailureMessage);
-    System.out.println(lastFailureMessage);
+    out.println(lastFailureMessage);
   }
   
   /**
@@ -476,20 +508,20 @@
                 lastFailureMessage = "\n           got " + gotString
                                    + "\n\n           but expected " + expString
                                    + "\n\n";
-                System.out.println(lastFailureMessage);
+                out.println(lastFailureMessage);
                 return;
               }
             else
               {
                 lastFailureMessage = "Objects were not equal";
-                System.out.println("objects were not equal.  " +
+                out.println("objects were not equal.  " +
                         "Use -debug for more information.");
                 return;
               }
           }
         lastFailureMessage = "got " + gotString + " but expected " + expString;
         currentResult.addFail(desc + " -- " +lastFailureMessage);
-        System.out.println(lastFailureMessage);
+        out.println(lastFailureMessage);
       }
   }
 
@@ -508,7 +540,7 @@
       {
         lastFailureMessage = "got " + result + " but expected " + expected;
         currentResult.addFail(desc + " -- " +lastFailureMessage);
-        System.out.println(lastFailureMessage);
+        out.println(lastFailureMessage);
       }
   }
 
@@ -527,7 +559,7 @@
       {
         lastFailureMessage = "got " + result + " but expected " + expected;
         currentResult.addFail(desc + " -- " +lastFailureMessage);
-        System.out.println(lastFailureMessage);
+        out.println(lastFailureMessage);
       }
   }
 
@@ -546,7 +578,7 @@
       {
         lastFailureMessage = "got " + result + " but expected " + expected;
         currentResult.addFail(desc + " -- " +lastFailureMessage);
-        System.out.println(lastFailureMessage);
+        out.println(lastFailureMessage);
       }
   }
 
@@ -574,7 +606,7 @@
       {
         lastFailureMessage = "got " + result + " but expected " + expected;
         currentResult.addFail(desc + " -- " +lastFailureMessage);
-        System.out.println(lastFailureMessage);
+        out.println(lastFailureMessage);
       }
   }
   
@@ -590,7 +622,7 @@
       {
         lastFailureMessage = "boolean passed to check was false";
         currentResult.addFail(desc + " -- " +lastFailureMessage);
-        System.out.println(lastFailureMessage);
+        out.println(lastFailureMessage);
       }
   }
   
@@ -602,7 +634,7 @@
   {
     // Send a message to the Harness to let it know the current test
     // isn't hung, to restart the timer.
-    System.out.println("RunnerProcess:restart-timer");
+    out.println("RunnerProcess:restart-timer");
     
     // If the test failed we have to print out some explanation.
     StackTraceElement[] st = new Throwable().getStackTrace();
@@ -615,16 +647,16 @@
             // If the failure wasn't expected, we need to print it to the
             // screen.
             if (failures == 0 && !verbose)
-              System.out.println ("FAIL: " + stripPrefix(description) + ":");
+              out.println ("FAIL: " + stripPrefix(description) + ":");
             if (verbose)
-              System.out.print("  FAIL:");
-            System.out.print(desc + " -- ");
+              out.print("  FAIL:");
+            out.print(desc + " -- ");
             ++failures;
           }
         else if (verbose)
           {
             // If it was expected but verbose is true, we also print it.
-            System.out.println("X" + desc + " -- ");
+            out.println("X" + desc + " -- ");
             ++xfailures;
           }
       }
@@ -636,11 +668,11 @@
           {
             if (expected_xfails.contains(desc))
               {
-                System.out.println("XPASS: " + desc);
+                out.println("XPASS: " + desc);
                 ++xpasses;
               }
             else
-              System.out.println("  pass:" + desc);
+              out.println("  pass:" + desc);
           }
       }
     ++count;
@@ -698,7 +730,7 @@
   public void verbose(String message)
   {
     if (verbose)
-      System.out.println(message);
+      out.println(message);
   }
 
   public void debug(String message)
@@ -711,16 +743,16 @@
     if (debug)
       {
         if (newline)
-          System.out.println(message);
+          out.println(message);
         else
-          System.out.print(message);
+          out.print(message);
       }
   }
 
   public void debug(Throwable ex)
   {
     if (debug)
-      ex.printStackTrace(System.out);
+      ex.printStackTrace(out);
   }
 
   public void debug(Object[] o, String desc)
@@ -813,18 +845,18 @@
   {
     if (failures > 0 && verbose)
       {
-        System.out.print("TEST FAILED: ");
-        System.out.println(failures + " of " + total + " checks failed "
+        out.print("TEST FAILED: ");
+        out.println(failures + " of " + total + " checks failed "
                            + stripPrefix(description));
       }
     else if (verbose)
-      System.out.println("TEST PASSED (" + total + " checks) "
+      out.println("TEST PASSED (" + total + " checks) "
                          + stripPrefix(description));
     if (xpasses > 0)
-      System.out.println(xpasses + " of " + total
+      out.println(xpasses + " of " + total
                          + " tests unexpectedly passed");
     if (xfailures > 0)
-      System.out.println(xfailures + " of " + total
+      out.println(xfailures + " of " + total
                          + " tests expectedly failed");
     return failures;
   }
@@ -896,6 +928,6 @@
     {
       // This shouldn't happen.
     }
-    System.out.println("_data_dump_okay_");
+    out.println("_data_dump_okay_");
   }
 }

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: RFC: Harness with no busy-waiting
  2006-06-26 19:28 RFC: Harness with no busy-waiting Anthony Balkissoon
@ 2006-06-27  8:41 ` Mark Wielaard
  2006-06-27 19:04   ` Anthony Balkissoon
  0 siblings, 1 reply; 6+ messages in thread
From: Mark Wielaard @ 2006-06-27  8:41 UTC (permalink / raw)
  To: Anthony Balkissoon; +Cc: mauve-patches

Hi Anthony,

On Mon, 2006-06-26 at 15:27 -0400, Anthony Balkissoon wrote:
> This removes the busy waiting from the Harness and replaces it with
> blocking read() calls using Sockets with specified timeouts.
>
> This is RFC because I'd like it to be tested to see if it speeds things
> up as much as Mark's workaround before it is checked in.

I don't really like this design. It puts even more stuff inside the
RunnerProcess which really should be as simple as possible so you can
easily run and debug it. Otherwise you might be trying to debug the
infrastructure and not the test itself. And by adding Sockets you a also
make it very hard/impossible to run single tests without the Harness to
really debug them. Please just use normal streams for communication
between the RunnerProcess and whatever calls it. And add a old-fashion
timeout with Object.wait()/notify() to the Harness, or if you want
something more fancy there add a nio.channel.Selector. But please don't
make the RunnerProcess depend on too much infrastructure.

Cheers,

Mark

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: RFC: Harness with no busy-waiting
  2006-06-27  8:41 ` Mark Wielaard
@ 2006-06-27 19:04   ` Anthony Balkissoon
  2006-06-27 19:32     ` Anthony Balkissoon
  2006-06-30 23:08     ` Mark Wielaard
  0 siblings, 2 replies; 6+ messages in thread
From: Anthony Balkissoon @ 2006-06-27 19:04 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: mauve-patches

[-- Attachment #1: Type: text/plain, Size: 2288 bytes --]

On Tue, 2006-06-27 at 10:41 +0200, Mark Wielaard wrote:
> Hi Anthony,
> 
> On Mon, 2006-06-26 at 15:27 -0400, Anthony Balkissoon wrote:
> > This removes the busy waiting from the Harness and replaces it with
> > blocking read() calls using Sockets with specified timeouts.
> >
> > This is RFC because I'd like it to be tested to see if it speeds things
> > up as much as Mark's workaround before it is checked in.
> 
> I don't really like this design. It puts even more stuff inside the
> RunnerProcess which really should be as simple as possible so you can
> easily run and debug it. Otherwise you might be trying to debug the
> infrastructure and not the test itself. And by adding Sockets you a also
> make it very hard/impossible to run single tests without the Harness to
> really debug them. Please just use normal streams for communication
> between the RunnerProcess and whatever calls it. And add a old-fashion
> timeout with Object.wait()/notify() to the Harness, or if you want
> something more fancy there add a nio.channel.Selector. But please don't
> make the RunnerProcess depend on too much infrastructure.
> 
> Cheers,
> 
> Mark
> 

Okay, here's a second attempt, no Sockets, no tricks.  Still RFC.
Comments?

2006-06-27  Anthony Balkissoon  <abalkiss@redhat.com>

	* Harness.java:
	(bcp_timeout): Removed this field.
	(testIsHung): Likewise.
	(runner_lock): Likewise.
	(getClasspathInstallString): Return an empty string instead of null 
	when no bootclasspath is detected.
	(getBootClassPath): Removed the TimeoutWatcher and replaced the busy 
	waiting with a blocking readLine().
	(runTest): Redesigned to replace busy waiting with blocking readLine() 
	call.
	(TimeoutWatcher.run): If the timeout occurs, kill the RunnerProcess and
	restart it instead of notifying the Harness and letting the Harness do 
	the work.
	(DetectBootclasspath.main): If the bootclasspath isn't detectable, 
	return a String saying so.
	* RunnerProcess.java:
	(FAILED_TO_LOAD_DESCRIPTION): New field.
	(UNCAUGHT_EXCEPTION_DESCRIPTION): Likewise.
	(main): If the testname is null, call System.exit.
	(runtest): Set the description earlier to avoid NPE.  Handle loading 
	exceptions and uncaught exceptions better.
	(runAndReport): Handle loading exceptions and uncaught exceptions.

--Tony

[-- Attachment #2: RemoveBusyWait2.diff --]
[-- Type: text/x-patch, Size: 16170 bytes --]

Index: Harness.java
===================================================================
RCS file: /cvs/mauve/mauve/Harness.java,v
retrieving revision 1.15
diff -u -r1.15 Harness.java
--- Harness.java	24 Jun 2006 17:33:22 -0000	1.15
+++ Harness.java	27 Jun 2006 18:50:08 -0000
@@ -79,9 +79,6 @@
   // The location of the eclipse-ecj.jar file
   private static String ecjJarLocation = null;
   
-  // How long the bootclasspath finder program can run before termination
-  private static long bcp_timeout = 60000;
-  
   // How long a test may run before it is considered hung
   private static long runner_timeout = 60000;
 
@@ -138,12 +135,6 @@
   // A watcher to determine if runnerProcess is hung
   private static TimeoutWatcher runner_watcher = null;
   
-  // A flag indicating whether or not runnerProcess is hung
-  private static boolean testIsHung = false;
-  
-  // A lock used for synchronizing access to testIsHung
-  private static Object runner_lock = new Object();
-  
   // The arguments used when this Harness was invoked, we use this to create an
   // appropriate RunnerProcess
   private static String[] harnessArgs = null;
@@ -447,7 +438,7 @@
         // " -bootclasspath " followed by the detected path.
         if (temp != null)              
           return " -bootclasspath " + temp;
-        return temp;
+        return "";
       }
     
     // This section is for bootclasspath's specified with
@@ -484,52 +475,22 @@
         new BufferedReader
         (new InputStreamReader(p.getInputStream()));
       String bcpOutput = null;
-      // Create a timer to watch this new process.
-      TimeoutWatcher tw = new TimeoutWatcher(bcp_timeout);
-      tw.start();
       while (true)
         {
-          // If for some reason the process hangs, return null, indicating we
-          // cannot auto-detect the bootclasspath.
-          if (testIsHung)
+          // This readLine() is a blocking call.  This will hang if the 
+          // bootclasspath finder hangs.
+          bcpOutput = br.readLine();
+          if (bcpOutput.equals("BCP_FINDER:can't_find_bcp_"))
             {
-              synchronized (runner_lock)
-              {
-                testIsHung = false;
-              }
-              br.close();
-              p.destroy();
+              // This means the auto-detection failed.
               return null;
             }
-          if (br.ready())
+          else if (bcpOutput.startsWith("BCP_FINDER:"))
             {
-              bcpOutput = br.readLine();
-              if (bcpOutput == null)
-                {
-                  // This means the auto-detection failed.
-                  tw.stop();
-                  return null;
-                }
-              if (bcpOutput.startsWith("BCP_FINDER:"))
-                {
-                  tw.stop();
-                  return bcpOutput.substring(11);
-                }
-              else
-                System.out.println(bcpOutput);
+              return bcpOutput.substring(11);
             }
-	  else
-	    {
-	      // FIXME - Quickworkaround for busy-waiting, needs proper FIX.
-	      try
-	        {
-		  Thread.sleep(100);
-		}
-	      catch (InterruptedException ie)
-	        {
-		  // ignore
-		}
-	    }
+          else
+            System.out.println(bcpOutput);
         }
     }
     catch (IOException ioe)
@@ -774,9 +735,8 @@
   {
     String tn = stripPrefix(testName.replace(File.separatorChar, '.'));
     String outputFromTest;
-    StringBuffer sb = new StringBuffer();
     boolean invalidTest = false;
-    int temp = -1;
+    int temp = -99;
 
     // Start the timeout watcher
     if (runner_watcher.isAlive())
@@ -789,113 +749,71 @@
     
     while (true)
       {
-        // This while loop polls for output from the test process and 
-        // passes it to System.out unless it is the signal that the 
-        // test finished properly.  Also checks to see if the watcher
-        // thread has declared the test hung and if so ends the process.
-        if (testIsHung)
-          {
-            System.err.print(sb.toString());
-            if (testName.equals("_confirm_startup_"))
-              {
-                System.err.println("ERROR: Cannot create test runner process.  Exit");
-                System.exit(1);
-              }
-            synchronized (runner_lock)
-            {
-              testIsHung = false;
-            }
-            try
-              {
-                runner_in.close();
-                runner_in_err.close();
-                runner_out.close();
-                runnerProcess.destroy();
-              }
-            catch (IOException e)
-              {
-                System.err.println("Could not close the interprocess pipes.");
-                System.exit(- 1);
-              }
-            initProcess(harnessArgs);
-            break;
-          }
+        // Continue getting output from the RunnerProcess until it
+        // signals the test completed or was invalid, or until the
+        // TimeoutWatcher stops the RunnerProcess forcefully.
         try
         {
-          if (runner_in_err.ready())
-            sb.append(runner_in_err.readLine() + "\n");
-          if (runner_in.ready())
+          outputFromTest = runner_in.readLine();
+          if (outputFromTest == null)
             {
-              outputFromTest = runner_in.readLine();              
-              if (outputFromTest.startsWith("RunnerProcess:"))
-                {
-                  invalidTest = false;
-                  // This means the RunnerProcess has sent us back some
-                  // information.  This could be telling us that a check() call
-                  // was made and we should reset the timer, or that the 
-                  // test passed, or failed, or that it wasn't a test.
-                  if (outputFromTest.endsWith("restart-timer"))
-                    runner_watcher.reset();
-                  else if (outputFromTest.endsWith("pass"))
-                    {
-                      temp = 0;
-                      break;
-                    }
-                  else if (outputFromTest.indexOf("fail-") != -1)
-                    {
-                      total_check_fails += 
-                        Integer.parseInt(outputFromTest.substring(19));
-                      temp = 1;
-                      break;
-                    }
-                  else if (outputFromTest.endsWith("not-a-test"))
-                    {
-                      // Temporarily decrease the total number of tests,
-                      // because it will be incremented later even 
-                      // though the test was not a real test.
-                      invalidTest = true;
-                      total_tests--;
-                      temp = 0;
-                      break;
-                    }                  
-                } 
-              else if (outputFromTest.equals("_startup_okay_") || 
-                  outputFromTest.equals("_data_dump_okay_"))
-                return;
-              else
-                // This means it was just output from the test, like a 
-                // System.out.println within the test itself, we should
-                // pass these on to stdout.
-                System.out.println(outputFromTest);
+              // This means the test hung.
+              temp = - 1;
+              break;
             }
-          else
+          else if (outputFromTest.startsWith("RunnerProcess:"))
             {
-	      if (sb.length() != 0)
-	        {
-                  System.err.print(sb.toString());
-                  sb = new StringBuffer();
-		}
-
-              // FIXME - Quickworkaround for busy-waiting, needs proper FIX.
-              try
+              invalidTest = false;
+              // This means the RunnerProcess has sent us back some
+              // information. This could be telling us that a check() call
+              // was made and we should reset the timer, or that the
+              // test passed, or failed, or that it wasn't a test.
+              if (outputFromTest.endsWith("restart-timer"))
+                runner_watcher.reset();
+              else if (outputFromTest.endsWith("pass"))
+                {
+                  temp = 0;
+                  break;
+                }
+              else if (outputFromTest.indexOf("fail-loading") != -1)
+                {
+                  temp = 1;
+                  System.out.println(outputFromTest.substring(27));
+                }
+              else if (outputFromTest.indexOf("fail-") != - 1)
                 {
-                  Thread.sleep(100);
+                  total_check_fails += Integer.parseInt(outputFromTest.substring(19));
+                  temp = 1;
+                  break;
                 }
-              catch (InterruptedException ie)
+              else if (outputFromTest.endsWith("not-a-test"))
                 {
-                  // ignore
+                  // Temporarily decrease the total number of tests,
+                  // because it will be incremented later even
+                  // though the test was not a real test.
+                  invalidTest = true;
+                  total_tests--;
+                  temp = 0;
+                  break;
                 }
             }
+          else if (outputFromTest.equals("_startup_okay_")
+              || outputFromTest.equals("_data_dump_okay_"))
+            return;
+          else
+            // This means it was just output from the test, like a
+            // System.out.println within the test itself, we should
+            // pass these on to stdout.
+            System.out.println(outputFromTest);
         }
         catch (IOException e)
         {
         }
       }
-    System.err.print(sb.toString());
     if (temp == -1)
       {        
-        // This means the watcher thread had to stop the process 
-        // from running.  So this is a fail.
+        // This means the watcher thread had to stop the process
+        // from running. So this is a fail.
         if (verbose)
           System.out.println("  FAIL: timed out. \nTEST FAILED: timeout " + 
                              tn);
@@ -1224,12 +1142,20 @@
         }
       if (shouldContinue)
         {
-          // The test is hung, set testIsHung to true so the process will be 
-          // destroyed and restarted.      
-          synchronized (runner_lock)
+          // The test is hung, destroy and restart the RunnerProcess.      
+          try
+          {
+            runnerProcess.destroy();
+            runner_in.close();
+            runner_in_err.close();
+            runner_out.close();
+          }
+          catch (IOException e)
           {
-            testIsHung = true;
+            System.err.println("Could not close the interprocess pipes.");
+            System.exit(- 1);
           }
+          initProcess(harnessArgs);
         }
     }
   }
@@ -1279,7 +1205,7 @@
               System.err.println("WARNING: Cannot auto-detect the " +
                       "bootclasspath for your VM, please file a bug report" +
                       " specifying which VM you are testing.");
-              return;
+              temp = "can't_find_bcp_";              
             }
         }
       System.out.println(result + temp);
Index: RunnerProcess.java
===================================================================
RCS file: /cvs/mauve/mauve/RunnerProcess.java,v
retrieving revision 1.9
diff -u -r1.9 RunnerProcess.java
--- RunnerProcess.java	27 Jun 2006 15:14:06 -0000	1.9
+++ RunnerProcess.java	27 Jun 2006 18:50:08 -0000
@@ -51,6 +51,12 @@
   // A description of files that are not tests
   private static final String NOT_A_TEST_DESCRIPTION = "not-a-test";
   
+  // A description of files that fail to load
+  private static final String FAIL_TO_LOAD_DESCRIPTION = "failed-to-load";
+  
+  // A description of a test that throws an uncaught exception
+  private static final String UNCAUGHT_EXCEPTION_DESCRIPTION = "uncaught-exception";
+  
   // Total number of harness.check calls since the last checkpoint
   private int count = 0;
   
@@ -195,8 +201,10 @@
         try
         {
           testname = in.readLine();
+          if (testname == null)
+            System.exit(0);
           if (testname.equals("_dump_data_"))
-            {
+            {              
               if (useEMMA)
                 dumpCoverageData();
               else
@@ -238,6 +246,7 @@
     System.runFinalization();
 
     currentResult = new TestResult(name.substring(12));
+    description = name;
 
     checkPoint(null);
 
@@ -256,6 +265,7 @@
       }
     catch (Throwable ex)
       {
+        description = FAIL_TO_LOAD_DESCRIPTION;
         // Maybe the file was marked not-a-test, check that before we report
         // it as an error
         try
@@ -282,28 +292,35 @@
         {          
         }
         
-        String d = "FAIL: " + stripPrefix(name)
-                   + "uncaught exception when loading";
-        currentResult.addException(ex, "failed loading class ",
-                                   "couldn't load: " + name);
-        if (verbose || exceptions)
-          d += ": " + ex.toString();
-
-        if (exceptions)
-          ex.printStackTrace(System.out);
+        String r = getStackTraceString(ex, "          ");
+        currentResult.addException(ex, "failed loading class ", r);
+        
         debug(ex);
         if (ex instanceof InstantiationException
             || ex instanceof IllegalAccessException)
           debug("Hint: is the code we just loaded a public non-abstract "
                 + "class with a public nullary constructor???");
-        ++failures;
-        ++total;
+
+        
+        if (!verbose)
+          System.out.println ("FAIL: " + stripPrefix(name) + ": exception when loading:");
+        else
+          {
+            System.out.println ("TEST: "+stripPrefix(name));
+            System.out.println("  FAIL: exception when loading");
+          }
+        if (exceptions)
+          System.out.println(getStackTraceString(ex, "   "));
+        
+        if (verbose)
+          System.out.println("TEST FAILED: exception when loading "
+                             + stripPrefix(name));
+        return;
       }
 
     // If the harness started okay, now we run the test.
     if (t != null)
       {
-        description = name;
         try
           {
             if (verbose)
@@ -313,18 +330,24 @@
           }
         catch (Throwable ex)
           {
-            if (failures == 0 && !verbose)
-              System.out.println ("FAIL: " + stripPrefix(name) + ":");
-            removeSecurityManager();
+            
             String d = exceptionDetails(ex, name, exceptions);
             String r = getStackTraceString(ex, "          ");
-            currentResult.addException(ex, d, r);
+
+            if (failures == 0 && !verbose)
+                System.out.println ("FAIL: " + stripPrefix(name) + ":");
             System.out.println(d);
+            removeSecurityManager();
+            currentResult.addException(ex, d, r);
+            
             if (exceptions)
               System.out.println(getStackTraceString(ex, "   "));
             debug(ex);
-            ++failures;
-            ++total;
+            
+            if (verbose)
+              System.out.println("TEST FAILED: uncaught exception "
+                                 + stripPrefix(description));
+            description = UNCAUGHT_EXCEPTION_DESCRIPTION;
           }
       }
     if (report != null)
@@ -365,6 +388,17 @@
         System.out.println("RunnerProcess:not-a-test");
         return;
       }
+    else if (harness.description.equals(FAIL_TO_LOAD_DESCRIPTION))
+      {
+        System.out.println("RunnerProcess:fail-0");
+        return;
+      }
+    else if (harness.description.equals(UNCAUGHT_EXCEPTION_DESCRIPTION))
+      {
+        System.out.println("RunnerProcess:fail-0");
+        return;
+      }
+    
     // Print out a summary.
     int temp = harness.done();
     // Print the report if necessary.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: RFC: Harness with no busy-waiting
  2006-06-27 19:04   ` Anthony Balkissoon
@ 2006-06-27 19:32     ` Anthony Balkissoon
  2006-06-27 20:54       ` Anthony Balkissoon
  2006-06-30 23:08     ` Mark Wielaard
  1 sibling, 1 reply; 6+ messages in thread
From: Anthony Balkissoon @ 2006-06-27 19:32 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: mauve-patches

> Okay, here's a second attempt, no Sockets, no tricks.  Still RFC.
> Comments?

By the way, this patch includes a known regression, any System.err.print
statements inside of tests will not be echoed.  This will be fixed soon,
if the overall patch is approved.

--Tony

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: RFC: Harness with no busy-waiting
  2006-06-27 19:32     ` Anthony Balkissoon
@ 2006-06-27 20:54       ` Anthony Balkissoon
  0 siblings, 0 replies; 6+ messages in thread
From: Anthony Balkissoon @ 2006-06-27 20:54 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: mauve-patches

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

This is committed with a very small change, the actual patch it attached
to this message.

--Tony

On Tue, 2006-06-27 at 15:32 -0400, Anthony Balkissoon wrote:
> > Okay, here's a second attempt, no Sockets, no tricks.  Still RFC.
> > Comments?
> 
> By the way, this patch includes a known regression, any System.err.print
> statements inside of tests will not be echoed.  This will be fixed soon,
> if the overall patch is approved.
> 
> --Tony

[-- Attachment #2: RemoveBusyWait4.diff --]
[-- Type: text/x-patch, Size: 16904 bytes --]

Index: Harness.java
===================================================================
RCS file: /cvs/mauve/mauve/Harness.java,v
retrieving revision 1.15
diff -u -r1.15 Harness.java
--- Harness.java	24 Jun 2006 17:33:22 -0000	1.15
+++ Harness.java	27 Jun 2006 20:52:51 -0000
@@ -79,9 +79,6 @@
   // The location of the eclipse-ecj.jar file
   private static String ecjJarLocation = null;
   
-  // How long the bootclasspath finder program can run before termination
-  private static long bcp_timeout = 60000;
-  
   // How long a test may run before it is considered hung
   private static long runner_timeout = 60000;
 
@@ -138,12 +135,6 @@
   // A watcher to determine if runnerProcess is hung
   private static TimeoutWatcher runner_watcher = null;
   
-  // A flag indicating whether or not runnerProcess is hung
-  private static boolean testIsHung = false;
-  
-  // A lock used for synchronizing access to testIsHung
-  private static Object runner_lock = new Object();
-  
   // The arguments used when this Harness was invoked, we use this to create an
   // appropriate RunnerProcess
   private static String[] harnessArgs = null;
@@ -447,7 +438,7 @@
         // " -bootclasspath " followed by the detected path.
         if (temp != null)              
           return " -bootclasspath " + temp;
-        return temp;
+        return "";
       }
     
     // This section is for bootclasspath's specified with
@@ -484,52 +475,22 @@
         new BufferedReader
         (new InputStreamReader(p.getInputStream()));
       String bcpOutput = null;
-      // Create a timer to watch this new process.
-      TimeoutWatcher tw = new TimeoutWatcher(bcp_timeout);
-      tw.start();
       while (true)
         {
-          // If for some reason the process hangs, return null, indicating we
-          // cannot auto-detect the bootclasspath.
-          if (testIsHung)
+          // This readLine() is a blocking call.  This will hang if the 
+          // bootclasspath finder hangs.
+          bcpOutput = br.readLine();
+          if (bcpOutput.equals("BCP_FINDER:can't_find_bcp_"))
             {
-              synchronized (runner_lock)
-              {
-                testIsHung = false;
-              }
-              br.close();
-              p.destroy();
+              // This means the auto-detection failed.
               return null;
             }
-          if (br.ready())
+          else if (bcpOutput.startsWith("BCP_FINDER:"))
             {
-              bcpOutput = br.readLine();
-              if (bcpOutput == null)
-                {
-                  // This means the auto-detection failed.
-                  tw.stop();
-                  return null;
-                }
-              if (bcpOutput.startsWith("BCP_FINDER:"))
-                {
-                  tw.stop();
-                  return bcpOutput.substring(11);
-                }
-              else
-                System.out.println(bcpOutput);
+              return bcpOutput.substring(11);
             }
-	  else
-	    {
-	      // FIXME - Quickworkaround for busy-waiting, needs proper FIX.
-	      try
-	        {
-		  Thread.sleep(100);
-		}
-	      catch (InterruptedException ie)
-	        {
-		  // ignore
-		}
-	    }
+          else
+            System.out.println(bcpOutput);
         }
     }
     catch (IOException ioe)
@@ -634,10 +595,10 @@
     try
       {
         runTest("_dump_data_");
+        runnerProcess.destroy();
         runner_in.close();
         runner_in_err.close();
-        runner_out.close();        
-        runnerProcess.destroy();
+        runner_out.close();                
       } 
     catch (IOException e) 
       {
@@ -682,6 +643,8 @@
     // because it may be a while before we run the next test (due to 
     // preprocessing and compilation) and we don't want the runner_watcher
     // to time out.
+    if (runner_watcher != null)
+      runner_watcher.stop();
     runner_watcher = new TimeoutWatcher(runner_timeout);    
     runTest("_confirm_startup_");
     runner_watcher.stop();
@@ -774,9 +737,8 @@
   {
     String tn = stripPrefix(testName.replace(File.separatorChar, '.'));
     String outputFromTest;
-    StringBuffer sb = new StringBuffer();
     boolean invalidTest = false;
-    int temp = -1;
+    int temp = -99;
 
     // Start the timeout watcher
     if (runner_watcher.isAlive())
@@ -789,113 +751,72 @@
     
     while (true)
       {
-        // This while loop polls for output from the test process and 
-        // passes it to System.out unless it is the signal that the 
-        // test finished properly.  Also checks to see if the watcher
-        // thread has declared the test hung and if so ends the process.
-        if (testIsHung)
-          {
-            System.err.print(sb.toString());
-            if (testName.equals("_confirm_startup_"))
-              {
-                System.err.println("ERROR: Cannot create test runner process.  Exit");
-                System.exit(1);
-              }
-            synchronized (runner_lock)
-            {
-              testIsHung = false;
-            }
-            try
-              {
-                runner_in.close();
-                runner_in_err.close();
-                runner_out.close();
-                runnerProcess.destroy();
-              }
-            catch (IOException e)
-              {
-                System.err.println("Could not close the interprocess pipes.");
-                System.exit(- 1);
-              }
-            initProcess(harnessArgs);
-            break;
-          }
+        // Continue getting output from the RunnerProcess until it
+        // signals the test completed or was invalid, or until the
+        // TimeoutWatcher stops the RunnerProcess forcefully.
         try
         {
-          if (runner_in_err.ready())
-            sb.append(runner_in_err.readLine() + "\n");
-          if (runner_in.ready())
+          outputFromTest = runner_in.readLine();
+          if (outputFromTest == null)
             {
-              outputFromTest = runner_in.readLine();              
-              if (outputFromTest.startsWith("RunnerProcess:"))
-                {
-                  invalidTest = false;
-                  // This means the RunnerProcess has sent us back some
-                  // information.  This could be telling us that a check() call
-                  // was made and we should reset the timer, or that the 
-                  // test passed, or failed, or that it wasn't a test.
-                  if (outputFromTest.endsWith("restart-timer"))
-                    runner_watcher.reset();
-                  else if (outputFromTest.endsWith("pass"))
-                    {
-                      temp = 0;
-                      break;
-                    }
-                  else if (outputFromTest.indexOf("fail-") != -1)
-                    {
-                      total_check_fails += 
-                        Integer.parseInt(outputFromTest.substring(19));
-                      temp = 1;
-                      break;
-                    }
-                  else if (outputFromTest.endsWith("not-a-test"))
-                    {
-                      // Temporarily decrease the total number of tests,
-                      // because it will be incremented later even 
-                      // though the test was not a real test.
-                      invalidTest = true;
-                      total_tests--;
-                      temp = 0;
-                      break;
-                    }                  
-                } 
-              else if (outputFromTest.equals("_startup_okay_") || 
-                  outputFromTest.equals("_data_dump_okay_"))
-                return;
-              else
-                // This means it was just output from the test, like a 
-                // System.out.println within the test itself, we should
-                // pass these on to stdout.
-                System.out.println(outputFromTest);
+              // This means the test hung.
+              initProcess(harnessArgs);
+              temp = - 1;              
+              break;
             }
-          else
+          else if (outputFromTest.startsWith("RunnerProcess:"))
             {
-	      if (sb.length() != 0)
-	        {
-                  System.err.print(sb.toString());
-                  sb = new StringBuffer();
-		}
-
-              // FIXME - Quickworkaround for busy-waiting, needs proper FIX.
-              try
+              invalidTest = false;
+              // This means the RunnerProcess has sent us back some
+              // information. This could be telling us that a check() call
+              // was made and we should reset the timer, or that the
+              // test passed, or failed, or that it wasn't a test.
+              if (outputFromTest.endsWith("restart-timer"))
+                runner_watcher.reset();
+              else if (outputFromTest.endsWith("pass"))
+                {
+                  temp = 0;
+                  break;
+                }
+              else if (outputFromTest.indexOf("fail-loading") != -1)
+                {
+                  temp = 1;
+                  System.out.println(outputFromTest.substring(27));
+                }
+              else if (outputFromTest.indexOf("fail-") != - 1)
                 {
-                  Thread.sleep(100);
+                  total_check_fails += Integer.parseInt(outputFromTest.substring(19));
+                  temp = 1;
+                  break;
                 }
-              catch (InterruptedException ie)
+              else if (outputFromTest.endsWith("not-a-test"))
                 {
-                  // ignore
+                  // Temporarily decrease the total number of tests,
+                  // because it will be incremented later even
+                  // though the test was not a real test.
+                  invalidTest = true;
+                  total_tests--;
+                  temp = 0;
+                  break;
                 }
             }
+          else if (outputFromTest.equals("_startup_okay_")
+              || outputFromTest.equals("_data_dump_okay_"))
+            return;
+          else
+            // This means it was just output from the test, like a
+            // System.out.println within the test itself, we should
+            // pass these on to stdout.
+            System.out.println(outputFromTest);
         }
         catch (IOException e)
         {
         }
       }
-    System.err.print(sb.toString());
     if (temp == -1)
       {        
-        // This means the watcher thread had to stop the process 
-        // from running.  So this is a fail.
+        // This means the watcher thread had to stop the process
+        // from running. So this is a fail.
         if (verbose)
           System.out.println("  FAIL: timed out. \nTEST FAILED: timeout " + 
                              tn);
@@ -1224,12 +1145,19 @@
         }
       if (shouldContinue)
         {
-          // The test is hung, set testIsHung to true so the process will be 
-          // destroyed and restarted.      
-          synchronized (runner_lock)
+          // The test is hung, destroy and restart the RunnerProcess.      
+          try
           {
-            testIsHung = true;
+            runnerProcess.destroy();
+            runner_in.close();
+            runner_in_err.close();
+            runner_out.close();
           }
+          catch (IOException e)
+          {
+            System.err.println("Could not close the interprocess pipes.");
+            System.exit(- 1);
+          }          
         }
     }
   }
@@ -1279,7 +1207,7 @@
               System.err.println("WARNING: Cannot auto-detect the " +
                       "bootclasspath for your VM, please file a bug report" +
                       " specifying which VM you are testing.");
-              return;
+              temp = "can't_find_bcp_";              
             }
         }
       System.out.println(result + temp);
Index: RunnerProcess.java
===================================================================
RCS file: /cvs/mauve/mauve/RunnerProcess.java,v
retrieving revision 1.9
diff -u -r1.9 RunnerProcess.java
--- RunnerProcess.java	27 Jun 2006 15:14:06 -0000	1.9
+++ RunnerProcess.java	27 Jun 2006 20:52:51 -0000
@@ -51,6 +51,12 @@
   // A description of files that are not tests
   private static final String NOT_A_TEST_DESCRIPTION = "not-a-test";
   
+  // A description of files that fail to load
+  private static final String FAIL_TO_LOAD_DESCRIPTION = "failed-to-load";
+  
+  // A description of a test that throws an uncaught exception
+  private static final String UNCAUGHT_EXCEPTION_DESCRIPTION = "uncaught-exception";
+  
   // Total number of harness.check calls since the last checkpoint
   private int count = 0;
   
@@ -195,8 +201,10 @@
         try
         {
           testname = in.readLine();
+          if (testname == null)
+            System.exit(0);
           if (testname.equals("_dump_data_"))
-            {
+            {              
               if (useEMMA)
                 dumpCoverageData();
               else
@@ -238,6 +246,7 @@
     System.runFinalization();
 
     currentResult = new TestResult(name.substring(12));
+    description = name;
 
     checkPoint(null);
 
@@ -256,6 +265,7 @@
       }
     catch (Throwable ex)
       {
+        description = FAIL_TO_LOAD_DESCRIPTION;
         // Maybe the file was marked not-a-test, check that before we report
         // it as an error
         try
@@ -282,28 +292,35 @@
         {          
         }
         
-        String d = "FAIL: " + stripPrefix(name)
-                   + "uncaught exception when loading";
-        currentResult.addException(ex, "failed loading class ",
-                                   "couldn't load: " + name);
-        if (verbose || exceptions)
-          d += ": " + ex.toString();
-
-        if (exceptions)
-          ex.printStackTrace(System.out);
+        String r = getStackTraceString(ex, "          ");
+        currentResult.addException(ex, "failed loading class ", r);
+        
         debug(ex);
         if (ex instanceof InstantiationException
             || ex instanceof IllegalAccessException)
           debug("Hint: is the code we just loaded a public non-abstract "
                 + "class with a public nullary constructor???");
-        ++failures;
-        ++total;
+
+        
+        if (!verbose)
+          System.out.println ("FAIL: " + stripPrefix(name) + ": exception when loading:");
+        else
+          {
+            System.out.println ("TEST: "+stripPrefix(name));
+            System.out.println("  FAIL: exception when loading");
+          }
+        if (exceptions)
+          System.out.println(getStackTraceString(ex, "   "));
+        
+        if (verbose)
+          System.out.println("TEST FAILED: exception when loading "
+                             + stripPrefix(name));
+        return;
       }
 
     // If the harness started okay, now we run the test.
     if (t != null)
       {
-        description = name;
         try
           {
             if (verbose)
@@ -313,18 +330,24 @@
           }
         catch (Throwable ex)
           {
-            if (failures == 0 && !verbose)
-              System.out.println ("FAIL: " + stripPrefix(name) + ":");
-            removeSecurityManager();
+            
             String d = exceptionDetails(ex, name, exceptions);
             String r = getStackTraceString(ex, "          ");
-            currentResult.addException(ex, d, r);
+
+            if (failures == 0 && !verbose)
+                System.out.println ("FAIL: " + stripPrefix(name) + ":");
             System.out.println(d);
+            removeSecurityManager();
+            currentResult.addException(ex, d, r);
+            
             if (exceptions)
               System.out.println(getStackTraceString(ex, "   "));
             debug(ex);
-            ++failures;
-            ++total;
+            
+            if (verbose)
+              System.out.println("TEST FAILED: uncaught exception "
+                                 + stripPrefix(description));
+            description = UNCAUGHT_EXCEPTION_DESCRIPTION;
           }
       }
     if (report != null)
@@ -365,6 +388,17 @@
         System.out.println("RunnerProcess:not-a-test");
         return;
       }
+    else if (harness.description.equals(FAIL_TO_LOAD_DESCRIPTION))
+      {
+        System.out.println("RunnerProcess:fail-0");
+        return;
+      }
+    else if (harness.description.equals(UNCAUGHT_EXCEPTION_DESCRIPTION))
+      {
+        System.out.println("RunnerProcess:fail-0");
+        return;
+      }
+    
     // Print out a summary.
     int temp = harness.done();
     // Print the report if necessary.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: RFC: Harness with no busy-waiting
  2006-06-27 19:04   ` Anthony Balkissoon
  2006-06-27 19:32     ` Anthony Balkissoon
@ 2006-06-30 23:08     ` Mark Wielaard
  1 sibling, 0 replies; 6+ messages in thread
From: Mark Wielaard @ 2006-06-30 23:08 UTC (permalink / raw)
  To: Anthony Balkissoon; +Cc: mauve-patches

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

Hi Anthony,

On Tue, 2006-06-27 at 15:04 -0400, Anthony Balkissoon wrote:
> Okay, here's a second attempt, no Sockets, no tricks.

I like it thanks!

I did have two small issues that took me a while to track down.
Sometimes the readLine() would throw an IOException after the process
was destroyed so I made an IOException do the same thing thing as if the
readLine() returned null. And sometimes there was a race condition
between the setting up a new RunnerProcess and the TimeoutWatcher
destroying the old one (the Process.destroy() could take some time). So
I added the Process as an explicit field of the TimeoutWatcher so that
it would only destroy the Process it is watching.

2006-06-30  Mark Wielaard  <mark@klomp.org>

        * Harness.java (initProcess): Initialize TimeoutWatcher with
        runnerProcess.
        (runAllTests): Treat IOException as timeout.
        (TimeoutWatcher.runnerProcess): New field.
        (TimeoutWatcher.run): Close streams of runnerProcess.

Committed,

Mark

[-- Attachment #2: Harness.patch --]
[-- Type: text/x-patch, Size: 2090 bytes --]

Index: Harness.java
===================================================================
RCS file: /cvs/mauve/mauve/Harness.java,v
retrieving revision 1.16
diff -u -r1.16 Harness.java
--- Harness.java	27 Jun 2006 20:53:35 -0000	1.16
+++ Harness.java	30 Jun 2006 23:06:52 -0000
@@ -645,10 +645,10 @@
     // to time out.
     if (runner_watcher != null)
       runner_watcher.stop();
-    runner_watcher = new TimeoutWatcher(runner_timeout);    
+    runner_watcher = new TimeoutWatcher(runner_timeout, runnerProcess);
     runTest("_confirm_startup_");
     runner_watcher.stop();
-    runner_watcher = new TimeoutWatcher(runner_timeout);
+    runner_watcher = new TimeoutWatcher(runner_timeout, runnerProcess);
   }
   
   /**
@@ -811,6 +811,9 @@
         }
         catch (IOException e)
         {
+          initProcess(harnessArgs);
+          temp = -1;
+          break;
         }
       }
     if (temp == -1)
@@ -1079,16 +1082,19 @@
     private boolean loop = true;
     private boolean shouldContinue = true;
     
+    private final Process runnerProcess;
+
     /**
      * Creates a new TimeoutWatcher that will wait for <code>millis</code>
      * milliseconds once started.
      * @param millis the number of milliseconds to wait before declaring the 
      * test as hung
      */
-    public TimeoutWatcher(long millis)
+    public TimeoutWatcher(long millis, Process runnerProcess)
     {
       millisToWait = millis;
       watcherThread = new Thread(this);
+      this.runnerProcess = runnerProcess;
     }
     
     /**
@@ -1148,10 +1154,10 @@
           // The test is hung, destroy and restart the RunnerProcess.      
           try
           {
-            runnerProcess.destroy();
-            runner_in.close();
-            runner_in_err.close();
-            runner_out.close();
+            this.runnerProcess.destroy();
+            this.runnerProcess.getInputStream().close();
+            this.runnerProcess.getErrorStream().close();
+            this.runnerProcess.getOutputStream().close();
           }
           catch (IOException e)
           {

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2006-06-30 23:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-06-26 19:28 RFC: Harness with no busy-waiting Anthony Balkissoon
2006-06-27  8:41 ` Mark Wielaard
2006-06-27 19:04   ` Anthony Balkissoon
2006-06-27 19:32     ` Anthony Balkissoon
2006-06-27 20:54       ` Anthony Balkissoon
2006-06-30 23:08     ` Mark Wielaard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).