From 18a7fcc0b3e0a8e412d6e2123b0518647ab4e005 Mon Sep 17 00:00:00 2001 From: Aidan Hahn Date: Sat, 1 Jun 2019 14:59:16 -0700 Subject: [PATCH] refactored mutexes to use java ReentrantLock --- src/main/java/JobServ/ProcessController.java | 38 +++++++- src/main/java/JobServ/ProcessManager.java | 89 ++++++------------- .../ProcessManagerTestImplementation.java | 8 -- 3 files changed, 63 insertions(+), 72 deletions(-) diff --git a/src/main/java/JobServ/ProcessController.java b/src/main/java/JobServ/ProcessController.java index f7d4cef..b576211 100644 --- a/src/main/java/JobServ/ProcessController.java +++ b/src/main/java/JobServ/ProcessController.java @@ -8,11 +8,14 @@ package JobServ; +import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; -import java.io.IOException; -import java.io.InputStreamReader; import java.io.BufferedReader; +import java.io.InputStreamReader; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReentrantLock; /* * ProcessController @@ -33,18 +36,23 @@ class ProcessController { private BufferedReader reader; private Process process; - private Boolean killedManually = false; + private Lock lock; + private int lockTimeout; // seconds + /* * Constructor * Takes a command and spawns it in a new process * Redirects IO streams and assigns a fake PID */ - public ProcessController(String command) throws IOException { + public ProcessController(String command, int lockTimeout) throws IOException { this.pid = ProcessController.nextPid; ProcessController.nextPid += 1; + this.lock = new ReentrantLock(); + this.lockTimeout = lockTimeout; + this.process = Runtime.getRuntime().exec(command); this.output = this.process.getOutputStream(); this.input = this.process.getInputStream(); @@ -54,6 +62,28 @@ class ProcessController { JobServServer.logger.write("Job " + String.valueOf(this.pid) + ": " + command); } + /* + * getLock() + * attempts to get the lock for lockTimeout seconds + * or throws exceptions if interrupted + */ + public boolean getLock() throws InterruptedException { + return this.lock.tryLock(this.lockTimeout, TimeUnit.SECONDS); + } + + /* + * releaseLock() + * releases lock on process + */ + public void releaseLock() { + try { + this.lock.unlock(); + + } catch (IllegalMonitorStateException e) { + JobServServer.logger.write("Thread tried to release a lock it didnt have! " + e.getMessage()); + } + } + /* * getPid() * returns translated pid of this process diff --git a/src/main/java/JobServ/ProcessManager.java b/src/main/java/JobServ/ProcessManager.java index 14ac67d..8ea34f0 100644 --- a/src/main/java/JobServ/ProcessManager.java +++ b/src/main/java/JobServ/ProcessManager.java @@ -40,7 +40,6 @@ class ProcessManager { * processMap */ protected ConcurrentHashMap processMap; - protected ConcurrentHashMap lockMap; private ExecutorService threadPool = Executors.newCachedThreadPool(); /* @@ -49,7 +48,6 @@ class ProcessManager { */ public ProcessManager() { processMap = new ConcurrentHashMap(); - lockMap = new ConcurrentHashMap(); /* 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. @@ -68,11 +66,9 @@ class ProcessManager { public int newProcess(String command) { try { - ProcessController newProc = new ProcessController(command); + ProcessController newProc = new ProcessController(command, this.LOCK_TIMEOUT); // we dont need to lock the map yet - this.lockMap.put(newProc.getPid(), true); this.processMap.put(newProc.getPid(), newProc); - this.releaseLock(newProc.getPid()); return newProc.getPid(); @@ -95,14 +91,13 @@ class ProcessManager { public int getProcessStatus(int pid) { try { if(!this.getLock(pid)) { - return 3; + // lock could not be grabbed before timeout + JobServServer.logger.write("Timeout getting process status: " + String.valueOf(pid)); + return 4; } - } catch (TimeoutException e) { - // lock could not be grabbed before timeout - JobServServer.logger.write("Timeout getting process " + - String.valueOf(pid) + " status: " + e.getMessage()); - return 4; + } catch (IndexOutOfBoundsException e) { + return 3; } ProcessController candidate = this.processMap.get(pid); @@ -123,13 +118,12 @@ class ProcessManager { public int getProcessReturn(int pid) { try { if(!this.getLock(pid)) { - return 258; + JobServServer.logger.write("Timeout getting process return: " + String.valueOf(pid)); + return 259; } - } catch (TimeoutException e) { - JobServServer.logger.write("Timeout getting process " + - String.valueOf(pid) + " return: " + e.getMessage()); - return 259; + } catch (IndexOutOfBoundsException e) { + return 258; } ProcessController candidate = this.processMap.get(pid); @@ -146,13 +140,12 @@ class ProcessManager { public String getProcessOutput(int pid, int lines) { try { if(!this.getLock(pid)) { - return "[-] SERVER: Process not found"; + JobServServer.logger.write("Timeout getting process output: " + String.valueOf(pid)); + return "[-] SERVER: Timeout grabbing lock to access process information"; } - } catch (TimeoutException e) { - JobServServer.logger.write("Timeout getting process " + - String.valueOf(pid) + " output: " + e.getMessage()); - return "[-] SERVER: Timeout grabbing lock to access process information"; + } catch (IndexOutOfBoundsException e) { + return "[-] SERVER: Process not found"; } ProcessController candidate = this.processMap.get(pid); @@ -172,13 +165,13 @@ class ProcessManager { public int killProcess(int pid) { try { if(!this.getLock(pid)) { - return 2; + JobServServer.logger.write("Timeout killing process: " + String.valueOf(pid)); + return 3; } - } catch (TimeoutException e) { - JobServServer.logger.write("Timeout killing process " + - String.valueOf(pid) + ": " + e.getMessage()); - return 3; + } catch (IndexOutOfBoundsException e) { + + return 2; } ProcessController candidate = this.processMap.get(pid); @@ -194,45 +187,20 @@ class ProcessManager { * Function is synchronized to prevent multiple threads accessing the same lock at once * (ConcurrentHashMap will report whatever lock value was last to successfully update) */ - protected synchronized Boolean getLock(int pid) throws TimeoutException { - if (!lockMap.containsKey(pid)) { - return false; + protected synchronized Boolean getLock(int pid) throws IndexOutOfBoundsException { + ProcessController candidate = this.processMap.get(pid); + if (candidate == null) { + throw new IndexOutOfBoundsException(); } - Future future = this.threadPool.submit( - new Callable() { - public Object call() { - while(lockMap.get(pid)) { - continue; // spin! - } - - lockMap.replace(pid, true); - return 1; - } - }); - try { - future.get(this.LOCK_TIMEOUT, TimeUnit.SECONDS); + Boolean success = candidate.getLock(); + return success; } catch (InterruptedException e) { JobServServer.logger.write("[!] Couldnt get lock " + String.valueOf(pid) + ": "+ e.getMessage()); - future.cancel(true); - - // in case lock was grabbed after exception - this.releaseLock(pid); return false; - - } catch (ExecutionException e) { - JobServServer.logger.write("[!] Couldnt get lock " + - String.valueOf(pid) + ": "+ e.getMessage()); - future.cancel(true); - - // in case lock was grabbed after exception - this.releaseLock(pid); - return false; - - // cancel the attempt to grab the lock } /* @@ -247,8 +215,6 @@ class ProcessManager { * mediate access to the ProcessManager * object for fresh calls as well. */ - - return true; } /* @@ -256,7 +222,10 @@ class ProcessManager { * releases mutex so other threads can operate on processqueue */ protected void releaseLock(int pid) { - this.lockMap.put(pid, false); + ProcessController candidate = this.processMap.get(pid); + if (candidate == null) { + JobServServer.logger.write("Tried to release lock of process that doesnt exist!"); + } } /* diff --git a/src/test/java/JobServ/ProcessManagerTestImplementation.java b/src/test/java/JobServ/ProcessManagerTestImplementation.java index 58594b5..de61a36 100644 --- a/src/test/java/JobServ/ProcessManagerTestImplementation.java +++ b/src/test/java/JobServ/ProcessManagerTestImplementation.java @@ -27,18 +27,10 @@ class ProcessManagerTestImplementation extends ProcessManager { super.releaseLock(pid); - } catch (TimeoutException e) { - System.err.println("[!!] Long Call wasnt able to grab lock!"); - return; - } catch (InterruptedException e) { super.releaseLock(pid); // this doesnt happen, dont cancel this task System.err.println("[3] Released lock: interrupted"); return; } } - - public Boolean reportLockState(int pid) { - return super.lockMap.get(pid); - } }