diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index de71595f16b7310cf87c24c91e4c4c03cccf2e54..b13e8eb8e0ad2bfe0118f088091bb62d5c327d0b 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 0de033c15b3a3c718c5d7644a212623e14f07ef9..b95ae55701e764121421892a2c5083db69dfe14a 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 @@ class LL_COMMON_API LLProcess: public boost::noncopyable
* 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 711715e373b5cad3b3b89092296d40f5d1a51b40..8c21be196bb2df39b3d63334874f43d9d60edc68 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");