fix typoes, uncaught exceptions, started unit tests for ProcessManager

src/main/java/JobServ/ProcessController.java

@@ -8,6 +8,11 @@
 
 package JobServ;
 
+import java.util.Scanner;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.io.IOException;
+
 /*
  * ProcessController
  * This class wraps a java Process object with metadata
@@ -36,11 +41,11 @@ class ProcessController {
         this.pid = ProcessController.nextPid;
         ProcessController.nextPid += 1;
 
-        this.process = Runtime.exec(command);
+        this.process = Runtime.getRuntime().exec(command);
         this.output = this.process.getOutputStream();
         this.input = this.process.getInputStream();
         this.outputScanner = new Scanner(this.input);
-        this.outputScanner.useDelimieter("\\A");
+        this.outputScanner.useDelimiter("\\A");
     }
 
     /*
@@ -60,12 +65,12 @@ class ProcessController {
      *
      * TODO: (for future release) return thread state
      */
-    public Boolean getStatus() {
+    public int getStatus() {
         try {
             process.exitValue();
-            return true;
+            return 1;
         } catch (IllegalThreadStateException e) {
-            return false;
+            return 0;
         }
     }
 
@@ -89,8 +94,8 @@ class ProcessController {
      */
     public String getOutput() {
         String out = "";
-        while(scanner.hasNext()) {
+        while(outputScanner.hasNext()) {
-            out += scanner.next();
+            out += outputScanner.next();
         }
 
         return out;
@@ -101,8 +106,13 @@ class ProcessController {
      * Cleans up resources and destroys process
      */
     public void kill() {
+        try {
             this.input.close();
             this.output.close();
+        } catch (IOException e) {
+            // streams already closed
+        }
+
         process.destroy();
     }
 }

src/main/java/JobServ/ProcessManager.java

@@ -9,41 +9,52 @@
 package JobServ;
 
 import java.util.concurrent.Future;
+import java.util.concurrent.Callable;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.ExecutionException;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.io.IOException;
 
 /*
+ * ProcessManager
  * Holds a list of ProcessControllers and controls access to them via mutex
- * Additionally, starts and manages a background thread that clears finished processes from the arraylist
+ * Mutex Timeout is declared here as well.
  */
 class ProcessManager {
     // TODO: LOCK_TIMEOUT should be defined in a configuration management system
     private final int LOCK_TIMEOUT = 5; // seconds
-    private HashTable<int, ProcessController> processQueue;
+    private HashMap<Integer, ProcessController> processMap;
     private Boolean processQueueMutex = false;
     private ExecutorService threadPool = Executors.newCachedThreadPool();
 
-    private Callable<void> getLockCallable = new Callable<void>() {
+    private Callable<Object> lockCallable = new Callable<Object>() {
-            public void Object call() {
+            public Object call() {
                 while(processQueueMutex){
                     continue; // spin!
                 }
 
                 processQueueMutex = true;
+                return 1;
             }
-        }
+        };
 
     /*
      * Constructor
      * initializes process queue and start the background process checking daemon
      */
     public ProcessManager() {
-        processQueue = new HashTable<int, ProcessController>();
+        processMap = new HashMap<Integer, ProcessController>();
         /* TODO: In a long running server over a large period of time
          * It is possible that the streams used to redirect IO in the
          * Processes may become a significant use of resources.
          * In this case a background thread should be called to periodically
          * remove dead ProcessControllers after calling kill() on them.
          *
-         * (grab lock, iterate over map, remove processes that are done executing, store exit codes, release lock, sleep, repeat)
+         * (grab lock, iterate over map, remove finished processes, store exit codes, release lock, sleep, repeat)
          */
     }
 
@@ -51,6 +62,7 @@ class ProcessManager {
      * newProcess()
      * Takes a command and returns the translated pid of a new process
      * Returns -1 if getLock fails
+     * Returns -2 if controller throws an IOException
      */
     public int newProcess(String command) {
         /*
@@ -61,23 +73,29 @@ class ProcessManager {
          *      which likely changed system state before it was killed.
          */
 
-        // Enter critical section
         try {
+            // Enter critical section
             this.getLock();
 
-        } catch (TimeoutException e) {
+            ProcessController newProc = new ProcessController(command);
-            // (lock was not grabbed)
+            this.processMap.put(newProc.getPid(), newProc);
-            System.err.println("Timeout starting new job '%s': " + e.getMessage, command);
-            return -1
-        }
-
-        ProcessController newProc = ProcessController(command);
-        this.processes.map(newProc.getPid(), newProc);
 
             // Exit critical section
             this.releaseLock();
 
             return newProc.getPid();
+
+        } catch (TimeoutException e) {
+            // (lock was not grabbed)
+            System.err.println("Timeout starting new job: " + e.getMessage());
+            return -1;
+
+        } catch (IOException e) {
+            // (lock was grabbed)
+            this.releaseLock();
+            System.err.println("ProcessController couldnt start process: " + e.getMessage());
+            return -2;
+        }
     }
 
     /*
@@ -97,8 +115,7 @@ class ProcessManager {
 
         } catch (TimeoutException e) {
             // lock could not be grabbed before timeout
-            System.err.println("Timeout getting process status for %s: " + e.getMessage(),
+            System.err.println("Timeout getting process status: " + e.getMessage());
-                               Integer.toString(pid));
             return 3;
         }
 
@@ -128,8 +145,7 @@ class ProcessManager {
             this.getLock();
 
         } catch (TimeoutException e) {
-            System.err.println("Timeout getting process return for %s: " + e.getMessage(),
+            System.err.println("Timeout getting process return: " + e.getMessage());
-                               Integer.toString(pid));
             return 257;
         }
 
@@ -156,20 +172,19 @@ class ProcessManager {
             this.getLock();
 
         } catch (TimeoutException e) {
-            System.err.println("Timeout getting process output for %s: " + e.getMessage(),
+            System.err.println("Timeout getting process output: " + e.getMessage());
-                               Integer.toString());
             return "[-] ERROR: Timeout grabbing lock to access process information";
         }
 
         ProcessController candidate = this.processMap.get(pid);
         if (candidate != null) {
-            output = iter.getOutput();
+            output = candidate.getOutput();
             this.releaseLock();
             return output;
         }
 
         this.releaseLock();
-        return "[-] ERROR: Process not found"
+        return "[-] ERROR: Process not found";
     }
 
     /*
@@ -182,7 +197,7 @@ class ProcessManager {
             this.getLock();
 
         } catch (TimeoutException e) {
-            System.err.println("Timeout killing process: " + e.getMessage);
+            System.err.println("Timeout killing process: " + e.getMessage());
             return false;
         }
 
@@ -202,9 +217,10 @@ class ProcessManager {
      * Throws TimeoutException when it fails to get the lock.
      */
     private synchronized void getLock() throws TimeoutException {
+        Future<Object> future = this.threadPool.submit(this.lockCallable);
+
         try {
-            Future<void> future = executor.submit(task);
+            future.get(this.LOCK_TIMEOUT, TimeUnit.SECONDS);
-            void result = future.get(this.LOCK_TIMEOUT, TimeUnit.SECONDS);
 
         } catch (InterruptedException e) {
             System.err.println("[!] ERROR: " + e.getMessage());
@@ -249,9 +265,14 @@ class ProcessManager {
      * releases resources held in the processController objects
      */
     private void shutdown() {
-        this.getLock();
+        this.processQueueMutex = true;
-        for (ProcessController p : this.processQueue) {
+
-            p.kill(); // exit threads, release IO streams, etc.
+        Iterator<HashMap.Entry<Integer, ProcessController>> iterator = this.processMap.entrySet().iterator();
+        while (iterator.hasNext()) {
+            HashMap.Entry<Integer, ProcessController> entry = iterator.next();
+
+            entry.getValue().kill();
+            iterator.remove();
         }
     }
 }

src/test/java/JobServ/ProcessManagerTest.java

@@ -0,0 +1,49 @@
+/*
+ * ProcessManagerTest
+ *
+ * v1.0
+ *
+ * May 22, 2019
+ */
+
+package JobServ;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+import static org.junit.Assert.assertEquals;
+
+
+/*
+ * ProcessManagerTest
+ * Class that performs positive and negative unit tests
+ * of every public method in ProcessManager. This not
+ * only unit tests ProcessManager but also integration
+ * tests it with ProcessController.
+ */
+public class ProcessManagerTest {
+    ProcessManager manager;
+
+    /*
+     * ProcessManagerTest constructor
+     * initializes the process manager
+     */
+    public ProcessManagerTest() {
+        manager = new ProcessManager();
+    }
+
+    /*
+     * addProcessTest()
+     * positive unit test for newProcess
+     */
+    @Test
+    public void addProcessesTest() {
+        int pid1 = manager.newProcess("ping google");
+        assertEquals(0, pid1);
+
+        int pid2 = manager.newProcess("ping google");
+        assertEquals(1, pid2);
+    }
+
+}
+