Attachment 639678 Details for Bug 872738 – incomplete patch, but addresses some of the design

[patch] incomplete patch, but addresses some of the design

0001-Bug-872738-incomplete-fix.patch (text/plain), 8.82 KB, created by Elias Ross on 2012-11-06 22:42:48 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Elias Ross

Created: 2012-11-06 22:42:48 UTC

Size: 8.82 KB

patch

obsolete

>From 2c120c2ec45723edefb83f5ba09e53373a2c5624 Mon Sep 17 00:00:00 2001
>From: Elias Ross <elias_ross@apple.com>
>Date: Tue, 6 Nov 2012 14:24:33 -0800
>Subject: [PATCH] Bug 872738 incomplete fix
>
>---
> .../java/org/rhq/core/system/JavaSystemInfo.java   |    4 +-
> .../org/rhq/core/system/SystemInfoFactory.java     |    5 ++-
> .../org/rhq/core/util/exec/ProcessExecutor.java    |   47 ++++++++++++++------
> .../org/rhq/core/util/exec/StreamRedirector.java   |   14 +++---
> 4 files changed, 48 insertions(+), 22 deletions(-)
>
>diff --git a/modules/core/native-system/src/main/java/org/rhq/core/system/JavaSystemInfo.java b/modules/core/native-system/src/main/java/org/rhq/core/system/JavaSystemInfo.java
>index 61ede0c..b4dac82 100644
>--- a/modules/core/native-system/src/main/java/org/rhq/core/system/JavaSystemInfo.java
>+++ b/modules/core/native-system/src/main/java/org/rhq/core/system/JavaSystemInfo.java
>@@ -48,6 +48,9 @@
>  * @author John Mazzitelli
>  */
> public class JavaSystemInfo implements SystemInfo {
>+
>+    private final ProcessExecutor javaExec = new ProcessExecutor();
>+
>     /**
>      * Returns <code>false</code> - this implementation relies solely on the Java API to get all the system information
>      * it can.
>@@ -194,7 +197,6 @@ public ProcessExecutionResults executeProcess(ProcessExecution processExecution)
>             executionResults.setCapturedOutputStream(outputStream);
>         }
> 
>-        ProcessExecutor javaExec = new ProcessExecutor();
>         ProcessExecutorResults javaExecResults = javaExec.execute(process);
>         executionResults.setExitCode(javaExecResults.getExitCode());
>         executionResults.setError(javaExecResults.getError());
>diff --git a/modules/core/native-system/src/main/java/org/rhq/core/system/SystemInfoFactory.java b/modules/core/native-system/src/main/java/org/rhq/core/system/SystemInfoFactory.java
>index 8d9e05d..4405e09 100644
>--- a/modules/core/native-system/src/main/java/org/rhq/core/system/SystemInfoFactory.java
>+++ b/modules/core/native-system/src/main/java/org/rhq/core/system/SystemInfoFactory.java
>@@ -59,6 +59,7 @@
>     private static boolean disabled;
>     private static boolean initialized = false;
> 
>+    private static SystemInfo javaSystemInfo = new JavaSystemInfo();
>     private static SystemInfo cachedSystemInfo;
> 
>     // to avoid having this class need the native JNI classes at compile or load time, we'll build up
>@@ -242,7 +243,7 @@ public static synchronized SystemInfo createSystemInfo() {
>      * @return Java-only {@link SystemInfo} implementation
>      */
>     public static SystemInfo createJavaSystemInfo() {
>-        return new JavaSystemInfo();
>+        return javaSystemInfo;
>     }
> 
>     /**
>@@ -407,7 +408,7 @@ public static TemplateEngine fetchTemplateEngine() {
>                 }
>             }
> 
>-            // create a base IP address - this one is known to java and should always exist no matter what platform we are on 
>+            // create a base IP address - this one is known to java and should always exist no matter what platform we are on
>             try {
>                 try {
>                     tokens.put(TOKEN_PREFIX + "interfaces.java.address", InetAddress
>diff --git a/modules/core/util/src/main/java/org/rhq/core/util/exec/ProcessExecutor.java b/modules/core/util/src/main/java/org/rhq/core/util/exec/ProcessExecutor.java
>index 390bb5e..31e1cca 100644
>--- a/modules/core/util/src/main/java/org/rhq/core/util/exec/ProcessExecutor.java
>+++ b/modules/core/util/src/main/java/org/rhq/core/util/exec/ProcessExecutor.java
>@@ -34,6 +34,7 @@
> import java.text.SimpleDateFormat;
> import java.util.Date;
> import java.util.concurrent.Callable;
>+import java.util.concurrent.ExecutionException;
> import java.util.concurrent.ExecutorService;
> import java.util.concurrent.Executors;
> import java.util.concurrent.Future;
>@@ -43,7 +44,8 @@
> import org.rhq.core.util.UtilI18NResourceKeys;
> 
> /**
>- * Executes a process using 100% Java API.
>+ * Executes processes using the Java API.
>+ * This instance is thread safe and can be reused.
>  *
>  * <p><b>Warning: caution should be exercised when using this class - it allows any process to be started with no
>  * security restrictions.</b></p>
>@@ -52,7 +54,26 @@
>  */
> public class ProcessExecutor {
> 
>-    private static ExecutorService threadPool = Executors.newCachedThreadPool();
>+    /**
>+     * A thread pool for executing processes.
>+     * Although it would make sense to have this 'static', it mucks up RHQ Agent cleanup.
>+     * Far better is to keep a reference to this instance.
>+     */
>+    private final ExecutorService threadPool;
>+
>+    /**
>+     * Constructs with a thread pool which executes tasks.
>+     */
>+    public ProcessExecutor(ExecutorService threadPool) {
>+        this.threadPool = threadPool;
>+    }
>+
>+    /**
>+     * Constructs using a new, non-cached thread pool.
>+     */
>+    public ProcessExecutor() {
>+        this(Executors.newCachedThreadPool());
>+    }
> 
>     /**
>      * This executes any operating system process as described in the given start command. When this method returns, it
>@@ -147,10 +168,10 @@ public Integer call() throws Exception {
>      */
>     protected static class RedirectThreads {
> 
>-        private final StreamRedirector stdout;
>-        private final StreamRedirector stderr;
>+        private final Future<?> stdout;
>+        private final Future<?> stderr;
> 
>-        private RedirectThreads(StreamRedirector stdout, StreamRedirector stderr) {
>+        private RedirectThreads(Future<?> stdout, Future<?> stderr) {
>             this.stdout = stdout;
>             this.stderr = stderr;
>         }
>@@ -158,17 +179,17 @@ private RedirectThreads(StreamRedirector stdout, StreamRedirector stderr) {
>         /**
>          * Waits for output to be fully captured.
>          */
>-        public void join() throws InterruptedException {
>-            stderr.join();
>-            stdout.join();
>+        public void join() throws InterruptedException, ExecutionException {
>+            stderr.get();
>+            stdout.get();
>         }
> 
>         /**
>          * Interrupts these threads.
>          */
>         public void interrupt() {
>-            stderr.interrupt();
>-            stdout.interrupt();
>+            stderr.cancel(true);
>+            stdout.cancel(true);
>         }
> 
>     }
>@@ -217,8 +238,8 @@ protected RedirectThreads redirectStreams(ProcessToStart process, Process childP
>         StreamRedirector stdoutThread = new StreamRedirector(threadNamePrefix + "-stdout", stdout, fileOutputStream);
>         StreamRedirector stderrThread = new StreamRedirector(threadNamePrefix + "-stderr", stderr, fileOutputStream);
> 
>-        stdoutThread.start();
>-        stderrThread.start();
>+        Future<?> out = threadPool.submit(stdoutThread);
>+        Future<?> err = threadPool.submit(stderrThread);
> 
>         // if an input file was specified, take the file's data and write it to the process' stdin
>         File inputFile = getInputFile(process);
>@@ -240,7 +261,7 @@ protected RedirectThreads redirectStreams(ProcessToStart process, Process childP
> 
>         stdin.close();
> 
>-        return new RedirectThreads(stdoutThread, stderrThread);
>+        return new RedirectThreads(out, err);
>     }
> 
>     /**
>diff --git a/modules/core/util/src/main/java/org/rhq/core/util/exec/StreamRedirector.java b/modules/core/util/src/main/java/org/rhq/core/util/exec/StreamRedirector.java
>index afff92d..f070736 100644
>--- a/modules/core/util/src/main/java/org/rhq/core/util/exec/StreamRedirector.java
>+++ b/modules/core/util/src/main/java/org/rhq/core/util/exec/StreamRedirector.java
>@@ -31,7 +31,7 @@
>  *
>  * @author John Mazzitelli
>  */
>-public class StreamRedirector extends Thread {
>+public class StreamRedirector implements Runnable {
>     /**
>      * the stream where we read data from
>      */
>@@ -42,6 +42,8 @@
>      */
>     private final OutputStream m_output;
> 
>+    private final String m_name;
>+
>     /**
>      * Constructor for {@link StreamRedirector} that takes an input stream where we read data in and an output stream
>      * where we write the data read from the input stream. If the output stream is <code>null</code>, the incoming data
>@@ -54,18 +56,14 @@
>      * @throws IllegalArgumentException if input stream is <code>null</code>
>      */
>     public StreamRedirector(String name, InputStream is, OutputStream os) throws IllegalArgumentException {
>-        super(name);
> 
>         if (is == null) {
>             throw new IllegalArgumentException("is=null");
>         }
> 
>-        setDaemon(true);
>-
>+        m_name = name;
>         m_input = is;
>         m_output = os;
>-
>-        return;
>     }
> 
>     /**
>@@ -73,6 +71,10 @@ public StreamRedirector(String name, InputStream is, OutputStream os) throws Ill
>      */
>     @Override
>     public void run() {
>+        Thread t = Thread.currentThread();
>+        t.setName(m_name);
>+        t.setDaemon(true);
>+
>         final int bufferSize = 4096;
>         byte[] buffer = new byte[bufferSize];
>         boolean keepGoing = true;
>-- 
>1.7.9.3
>

Actions: View | Diff

Attachments on bug 872738: 639678 | 763100