Use per-frame ticks on "mainloop" LLEventPump to update LLProcess.

When we reimplemented LLProcess on APR, necessitating APR's funny callback
mechanism to sense child-process status, every isRunning() or getStatus() call
called the APR poll function that calls ALL registered LLProcess callbacks. In
other words, every time any consumer called any LLProcess::isRunning() method,
all LLProcess callbacks were redundantly fired. Change that so that the single
APR poll function is called once per frame, courtesy of the "mainloop"
LLEventPump. Once per viewer frame should be well within the realtime duration
in which it's reasonable to expect child-process status to change.
In effect, this changes LLProcess's public API to introduce a dependency on
"mainloop" ticks. Add such ticks to llprocess_test.cpp as well.

3 files changed, 114 insertions, 44 deletions

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index de71595f16..b13e8eb8e0 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -32,8 +32,10 @@
 #include "stringize.h"
 #include "llapr.h"
 #include "apr_signal.h"
+#include "llevents.h"
 
 #include <boost/foreach.hpp>
+#include <boost/bind.hpp>
 #include <iostream>
 #include <stdexcept>
 
@@ -47,6 +49,74 @@ struct LLProcessError: public std::runtime_error
 	LLProcessError(const std::string& msg): std::runtime_error(msg) {}
 };
 
+/**
+ * Ref-counted "mainloop" listener. As long as there are still outstanding
+ * LLProcess objects, keep listening on "mainloop" so we can keep polling APR
+ * for process status.
+ */
+class LLProcessListener
+{
+	LOG_CLASS(LLProcessListener);
+public:
+	LLProcessListener():
+		mCount(0)
+	{}
+
+	void addPoll(const LLProcess&)
+	{
+		// Unconditionally increment mCount. If it was zero before
+		// incrementing, listen on "mainloop".
+		if (mCount++ == 0)
+		{
+			LL_DEBUGS("LLProcess") << "listening on \"mainloop\"" << LL_ENDL;
+			mConnection = LLEventPumps::instance().obtain("mainloop")
+				.listen("LLProcessListener", boost::bind(&LLProcessListener::tick, this, _1));
+		}
+	}
+
+	void dropPoll(const LLProcess&)
+	{
+		// Unconditionally decrement mCount. If it's zero after decrementing,
+		// stop listening on "mainloop".
+		if (--mCount == 0)
+		{
+			LL_DEBUGS("LLProcess") << "disconnecting from \"mainloop\"" << LL_ENDL;
+			mConnection.disconnect();
+		}
+	}
+
+private:
+	/// called once per frame by the "mainloop" LLEventPump
+	bool tick(const LLSD&)
+	{
+		// Tell APR to sense whether each registered LLProcess is still
+		// running and call handle_status() appropriately. We should be able
+		// to get the same info from an apr_proc_wait(APR_NOWAIT) call; but at
+		// least in APR 1.4.2, testing suggests that even with APR_NOWAIT,
+		// apr_proc_wait() blocks the caller. We can't have that in the
+		// viewer. Hence the callback rigmarole. (Once we update APR, it's
+		// probably worth testing again.) Also -- although there's an
+		// apr_proc_other_child_refresh() call, i.e. get that information for
+		// one specific child, it accepts an 'apr_other_child_rec_t*' that's
+		// mentioned NOWHERE else in the documentation or header files! I
+		// would use the specific call in LLProcess::getStatus() if I knew
+		// how. As it is, each call to apr_proc_other_child_refresh_all() will
+		// call callbacks for ALL still-running child processes. That's why we
+		// centralize such calls, using "mainloop" to ensure it happens once
+		// per frame, and refcounting running LLProcess objects to remain
+		// registered only while needed.
+		LL_DEBUGS("LLProcess") << "calling apr_proc_other_child_refresh_all()" << LL_ENDL;
+		apr_proc_other_child_refresh_all(APR_OC_REASON_RUNNING);
+		return false;
+	}
+
+	/// If this object is destroyed before mCount goes to zero, stop
+	/// listening on "mainloop" anyway.
+	LLTempBoundListener mConnection;
+	unsigned mCount;
+};
+static LLProcessListener sProcessListener;
+
 LLProcessPtr LLProcess::create(const LLSDOrParams& params)
 {
 	try
@@ -159,6 +229,8 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 	// arrange to call status_callback()
 	apr_proc_other_child_register(&mProcess, &LLProcess::status_callback, this, mProcess.in,
 								  gAPRPoolp);
+	// and make sure we poll it once per "mainloop" tick
+	sProcessListener.addPoll(*this);
 	mStatus.mState = RUNNING;
 
 	mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcess.pid << ')');
@@ -195,6 +267,8 @@ LLProcess::~LLProcess()
 		// information updated in this object by such a callback is no longer
 		// available to any consumer anyway.
 		apr_proc_other_child_unregister(this);
+		// One less LLProcess to poll for
+		sProcessListener.dropPoll(*this);
 	}
 
 	if (mAutokill)
@@ -228,26 +302,6 @@ bool LLProcess::isRunning(void)
 
 LLProcess::Status LLProcess::getStatus()
 {
-	// Only when mState is RUNNING might the status change dynamically. For
-	// any other value, pointless to attempt to update status: it won't
-	// change.
-	if (mStatus.mState == RUNNING)
-	{
-		// Tell APR to sense whether the child is still running and call
-		// handle_status() appropriately. We should be able to get the same
-		// info from an apr_proc_wait(APR_NOWAIT) call; but at least in APR
-		// 1.4.2, testing suggests that even with APR_NOWAIT, apr_proc_wait()
-		// blocks the caller. We can't have that in the viewer. Hence the
-		// callback rigmarole. Once we update APR, it's probably worth testing
-		// again. Also -- although there's an apr_proc_other_child_refresh()
-		// call, i.e. get that information for one specific child, it accepts
-		// an 'apr_other_child_rec_t*' that's mentioned NOWHERE else in the
-		// documentation or header files! I would use the specific call if I
-		// knew how. As it is, each call to this method will call callbacks
-		// for ALL still-running child processes. Sigh...
-		apr_proc_other_child_refresh_all(APR_OC_REASON_RUNNING);
-	}
-
 	return mStatus;
 }
 
@@ -341,6 +395,8 @@ void LLProcess::handle_status(int reason, int status)
 	// it already knows the child has terminated. We must pass the same 'data'
 	// pointer as for the register() call, which was our 'this'.
 	apr_proc_other_child_unregister(this);
+	// don't keep polling for a terminated process
+	sProcessListener.dropPoll(*this);
 	// We overload mStatus.mState to indicate whether the child is registered
 	// for APR callback: only RUNNING means registered. Track that we've
 	// unregistered. We know the child has terminated; might be EXITED or
diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h
index 0de033c15b..b95ae55701 100644
--- a/indra/llcommon/llprocess.h
+++ b/indra/llcommon/llprocess.h
@@ -49,9 +49,17 @@ class LLProcess;
 typedef boost::shared_ptr<LLProcess> LLProcessPtr;
 
 /**
- *	LLProcess handles launching external processes with specified command line arguments.
- *	It also keeps track of whether the process is still running, and can kill it if required.
-*/
+ * LLProcess handles launching an external process with specified command line
+ * arguments. It also keeps track of whether the process is still running, and
+ * can kill it if required.
+ *
+ * LLProcess relies on periodic post() calls on the "mainloop" LLEventPump: an
+ * LLProcess object's Status won't update until the next "mainloop" tick. The
+ * viewer's main loop already posts to that LLEventPump once per iteration
+ * (hence the name). See indra/llcommon/tests/llprocess_test.cpp for an
+ * example of waiting for child-process termination in a standalone test
+ * context.
+ */
 class LL_COMMON_API LLProcess: public boost::noncopyable
 {
 	LOG_CLASS(LLProcess);
@@ -72,11 +80,7 @@ public:
 		 * zero or more additional command-line arguments. Arguments are
 		 * passed through as exactly as we can manage, whitespace and all.
 		 * @note On Windows we manage this by implicitly double-quoting each
-		 * argument while assembling the command line. BUT if a given argument
-		 * is already double-quoted, we don't double-quote it again. Try to
-		 * avoid making use of this, though, as on Mac and Linux explicitly
-		 * double-quoted args will be passed to the child process including
-		 * the double quotes.
+		 * argument while assembling the command line.
 		 */
 		Multiple<std::string> args;
 		/// current working directory, if need it changed
diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp
index 711715e373..8c21be196b 100644
--- a/indra/llcommon/tests/llprocess_test.cpp
+++ b/indra/llcommon/tests/llprocess_test.cpp
@@ -33,6 +33,7 @@
 #include "../test/namedtempfile.h"
 #include "stringize.h"
 #include "llsdutil.h"
+#include "llevents.h"
 
 #if defined(LL_WINDOWS)
 #define sleep(secs) _sleep((secs) * 1000)
@@ -88,6 +89,27 @@ static std::string readfile(const std::string& pathname, const std::string& desc
     return output;
 }
 
+/// Looping on LLProcess::isRunning() must now be accompanied by pumping
+/// "mainloop" -- otherwise the status won't update and you get an infinite
+/// loop.
+void waitfor(LLProcess& proc)
+{
+    while (proc.isRunning())
+    {
+        sleep(1);
+        LLEventPumps::instance().obtain("mainloop").post(LLSD());
+    }
+}
+
+void waitfor(LLProcess::handle h, const std::string& desc)
+{
+    while (LLProcess::isRunning(h, desc))
+    {
+        sleep(1);
+        LLEventPumps::instance().obtain("mainloop").post(LLSD());
+    }
+}
+
 /**
  * Construct an LLProcess to run a Python script.
  */
@@ -120,10 +142,7 @@ struct PythonProcessLauncher
         // there's no API to wait for the child to terminate -- but given
         // its use in our graphics-intensive interactive viewer, it's
         // understandable.
-        while (mPy->isRunning())
-        {
-            sleep(1);
-        }
+        waitfor(*mPy);
     }
 
     /**
@@ -619,10 +638,7 @@ namespace tut
         // script has performed its first write and should now be sleeping.
         py.mPy->kill();
         // wait for the script to terminate... one way or another.
-        while (py.mPy->isRunning())
-        {
-            sleep(1);
-        }
+        waitfor(*py.mPy);
 #if LL_WINDOWS
         ensure_equals("Status.mState", py.mPy->getStatus().mState, LLProcess::EXITED);
         ensure_equals("Status.mData",  py.mPy->getStatus().mData,  -1);
@@ -672,10 +688,7 @@ namespace tut
             // Destroy the LLProcess, which should kill the child.
         }
         // wait for the script to terminate... one way or another.
-        while (LLProcess::isRunning(phandle, "kill() script"))
-        {
-            sleep(1);
-        }
+        waitfor(phandle, "kill() script");
         // If kill() failed, the script would have woken up on its own and
         // overwritten the file with 'bad'. But if kill() succeeded, it should
         // not have had that chance.
@@ -737,10 +750,7 @@ namespace tut
             outf << "go";
         } // flush and close.
         // now wait for the script to terminate... one way or another.
-        while (LLProcess::isRunning(phandle, "autokill script"))
-        {
-            sleep(1);
-        }
+        waitfor(phandle, "autokill script");
         // If the LLProcess destructor implicitly called kill(), the
         // script could not have written 'ack' as we expect.
         ensure_equals("autokill script output", readfile(from.getName()), "ack");