Make LLProcess post termination event to specified pump if desired.

This way a caller need not spin on isRunning(); we can just listen for the
requested termination event.
Post a similar event containing error message if for any reason
LLProcess::create() failed to launch the child.
Add unit tests for both cases.

3 files changed, 151 insertions, 44 deletions

diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index d6a5a18565..9799ed1938 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -125,7 +125,7 @@ const LLProcess::BasePipe::size_type
 
 class WritePipeImpl: public LLProcess::WritePipe
 {
-    LOG_CLASS(WritePipeImpl);
+	LOG_CLASS(WritePipeImpl);
 public:
 	WritePipeImpl(const std::string& desc, apr_file_t* pipe):
 		mDesc(desc),
@@ -202,7 +202,7 @@ private:
 
 class ReadPipeImpl: public LLProcess::ReadPipe
 {
-    LOG_CLASS(ReadPipeImpl);
+	LOG_CLASS(ReadPipeImpl);
 public:
 	ReadPipeImpl(const std::string& desc, apr_file_t* pipe):
 		mDesc(desc),
@@ -394,6 +394,23 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params)
 	catch (const LLProcessError& e)
 	{
 		LL_WARNS("LLProcess") << e.what() << LL_ENDL;
+
+		// If caller is requesting an event on process termination, send one
+		// indicating bad launch. This may prevent someone waiting forever for
+		// a termination post that can't arrive because the child never
+		// started.
+		if (! std::string(params.postend).empty())
+		{
+			LLEventPumps::instance().obtain(params.postend)
+				.post(LLSDMap
+					  // no "id"
+					  ("desc", std::string(params.executable))
+					  ("state", LLProcess::UNSTARTED)
+					  // no "data"
+					  ("string", e.what())
+					 );
+		}
+
 		return LLProcessPtr();
 	}
 }
@@ -425,6 +442,8 @@ LLProcess::LLProcess(const LLSDOrParams& params):
 									   << LLSDNotationStreamer(params)));
 	}
 
+	mPostend = params.postend;
+
 	apr_procattr_t *procattr = NULL;
 	chkapr(apr_procattr_create(&procattr, gAPRPoolp));
 
@@ -744,6 +763,19 @@ void LLProcess::handle_status(int reason, int status)
 	// hand.
 	mStatus = interpret_status(status);
 	LL_INFOS("LLProcess") << getStatusString() << LL_ENDL;
+
+	// If caller requested notification on child termination, send it.
+	if (! mPostend.empty())
+	{
+		LLEventPumps::instance().obtain(mPostend)
+			.post(LLSDMap
+				  ("id",     getProcessID())
+				  ("desc",   mDesc)
+				  ("state",  mStatus.mState)
+				  ("data",   mStatus.mData)
+				  ("string", getStatusString())
+				 );
+	}
 }
 
 LLProcess::id LLProcess::getProcessID() const
@@ -769,72 +801,72 @@ std::string LLProcess::getPipeName(FILESLOT)
 template<class PIPETYPE>
 PIPETYPE* LLProcess::getPipePtr(std::string& error, FILESLOT slot)
 {
-    if (slot >= NSLOTS)
-    {
-        error = STRINGIZE(mDesc << " has no slot " << slot);
-        return NULL;
-    }
-    if (mPipes.is_null(slot))
-    {
-        error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a monitored pipe");
-        return NULL;
-    }
-    // Make sure we dynamic_cast in pointer domain so we can test, rather than
-    // accepting runtime's exception.
-    PIPETYPE* ppipe = dynamic_cast<PIPETYPE*>(&mPipes[slot]);
-    if (! ppipe)
-    {
-        error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a " << typeid(PIPETYPE).name());
-        return NULL;
-    }
-
-    error.clear();
-    return ppipe;
+	if (slot >= NSLOTS)
+	{
+		error = STRINGIZE(mDesc << " has no slot " << slot);
+		return NULL;
+	}
+	if (mPipes.is_null(slot))
+	{
+		error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a monitored pipe");
+		return NULL;
+	}
+	// Make sure we dynamic_cast in pointer domain so we can test, rather than
+	// accepting runtime's exception.
+	PIPETYPE* ppipe = dynamic_cast<PIPETYPE*>(&mPipes[slot]);
+	if (! ppipe)
+	{
+		error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a " << typeid(PIPETYPE).name());
+		return NULL;
+	}
+
+	error.clear();
+	return ppipe;
 }
 
 template <class PIPETYPE>
 PIPETYPE& LLProcess::getPipe(FILESLOT slot)
 {
-    std::string error;
-    PIPETYPE* wp = getPipePtr<PIPETYPE>(error, slot);
-    if (! wp)
-    {
-        throw NoPipe(error);
-    }
-    return *wp;
+	std::string error;
+	PIPETYPE* wp = getPipePtr<PIPETYPE>(error, slot);
+	if (! wp)
+	{
+		throw NoPipe(error);
+	}
+	return *wp;
 }
 
 template <class PIPETYPE>
 boost::optional<PIPETYPE&> LLProcess::getOptPipe(FILESLOT slot)
 {
-    std::string error;
-    PIPETYPE* wp = getPipePtr<PIPETYPE>(error, slot);
-    if (! wp)
-    {
-        LL_DEBUGS("LLProcess") << error << LL_ENDL;
-        return boost::optional<PIPETYPE&>();
-    }
-    return *wp;
+	std::string error;
+	PIPETYPE* wp = getPipePtr<PIPETYPE>(error, slot);
+	if (! wp)
+	{
+		LL_DEBUGS("LLProcess") << error << LL_ENDL;
+		return boost::optional<PIPETYPE&>();
+	}
+	return *wp;
 }
 
 LLProcess::WritePipe& LLProcess::getWritePipe(FILESLOT slot)
 {
-    return getPipe<WritePipe>(slot);
+	return getPipe<WritePipe>(slot);
 }
 
 boost::optional<LLProcess::WritePipe&> LLProcess::getOptWritePipe(FILESLOT slot)
 {
-    return getOptPipe<WritePipe>(slot);
+	return getOptPipe<WritePipe>(slot);
 }
 
 LLProcess::ReadPipe& LLProcess::getReadPipe(FILESLOT slot)
 {
-    return getPipe<ReadPipe>(slot);
+	return getPipe<ReadPipe>(slot);
 }
 
 boost::optional<LLProcess::ReadPipe&> LLProcess::getOptReadPipe(FILESLOT slot)
 {
-    return getOptPipe<ReadPipe>(slot);
+	return getOptPipe<ReadPipe>(slot);
 }
 
 std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params)
@@ -932,7 +964,7 @@ static std::string WindowsErrorString(const std::string& operation)
 					   NULL)
 		!= 0) 
 	{
-        // convert from wide-char string to multi-byte string
+		// convert from wide-char string to multi-byte string
 		char message[256];
 		wcstombs(message, error_str, sizeof(message));
 		message[sizeof(message)-1] = 0;
diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h
index 06be0954c0..96a3dce5b3 100644
--- a/indra/llcommon/llprocess.h
+++ b/indra/llcommon/llprocess.h
@@ -158,7 +158,8 @@ public:
 			args("args"),
 			cwd("cwd"),
 			autokill("autokill", true),
-			files("files")
+			files("files"),
+			postend("postend")
 		{}
 
 		/// pathname of executable
@@ -184,6 +185,20 @@ public:
 		 * underlying implementation library doesn't support that.
 		 */
 		Multiple<FileParam> files;
+		/**
+		 * On child-process termination, if this LLProcess object still
+		 * exists, post LLSD event to LLEventPump with specified name (default
+		 * no event). Event contains at least:
+		 *
+		 * - "id" as obtained from getProcessID()
+		 * - "desc" short string description of child (executable + pid)
+		 * - "state" @c state enum value, from Status.mState
+		 * - "data"	 if "state" is EXITED, exit code; if KILLED, on Posix,
+		 *   signal number
+		 * - "string" English text describing "state" and "data" (e.g. "exited
+		 *   with code 0")
+		 */
+		Optional<std::string> postend;
 	};
 	typedef LLSDParamAdapter<Params> LLSDOrParams;
 
@@ -462,6 +477,7 @@ private:
 	PIPETYPE* getPipePtr(std::string& error, FILESLOT slot);
 
 	std::string mDesc;
+	std::string mPostend;
 	apr_proc_t mProcess;
 	bool mAutokill;
 	Status mStatus;
diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp
index c67605cc0b..1a755c283c 100644
--- a/indra/llcommon/tests/llprocess_test.cpp
+++ b/indra/llcommon/tests/llprocess_test.cpp
@@ -1206,6 +1206,65 @@ namespace tut
         ensure("find(\"ghi\", 27)", childout.find("ghi", 27) == LLProcess::ReadPipe::npos);
     }
 
+    template<> template<>
+    void object::test<20>()
+    {
+        set_test_name("good postend");
+        PythonProcessLauncher py("postend",
+                                 "import sys\n"
+                                 "sys.exit(35)\n");
+        std::string pumpname("postend");
+        EventListener listener(LLEventPumps::instance().obtain(pumpname));
+        py.mParams.postend = pumpname;
+        py.launch();
+        LLProcess::id childid(py.mPy->getProcessID());
+        // Don't use waitfor(), which calls isRunning(); instead wait for an
+        // event on pumpname.
+        int i, timeout = 60;
+        for (i = 0; i < timeout && listener.mHistory.empty(); ++i)
+        {
+            yield();
+        }
+        ensure("no postend event", i < timeout);
+        ensure_equals("number of postend events", listener.mHistory.size(), 1);
+        LLSD postend(listener.mHistory.front());
+        ensure_equals("id",    postend["id"].asInteger(), childid);
+        ensure("desc empty", ! postend["desc"].asString().empty());
+        ensure_equals("state", postend["state"].asInteger(), LLProcess::EXITED);
+        ensure_equals("data",  postend["data"].asInteger(),  35);
+        std::string str(postend["string"]);
+        ensure_contains("string", str, "exited");
+        ensure_contains("string", str, "35");
+    }
+
+    template<> template<>
+    void object::test<21>()
+    {
+        set_test_name("bad postend");
+        std::string pumpname("postend");
+        EventListener listener(LLEventPumps::instance().obtain(pumpname));
+        LLProcess::Params params;
+        params.postend = pumpname;
+        LLProcessPtr child = LLProcess::create(params);
+        ensure("shouldn't have launched", ! child);
+        ensure_equals("number of postend events", listener.mHistory.size(), 1);
+        LLSD postend(listener.mHistory.front());
+        ensure("has id", ! postend.has("id"));
+        // Ha ha, in this case the implementation normally sets "desc" to
+        // params.executable. But as the nature of the problem is that
+        // params.executable is empty, expecting "desc" to be nonempty is a
+        // bit unreasonable!
+        //ensure("desc empty", ! postend["desc"].asString().empty());
+        ensure_equals("state", postend["state"].asInteger(), LLProcess::UNSTARTED);
+        ensure("has data", ! postend.has("data"));
+        std::string error(postend["string"]);
+        // All we get from canned parameter validation is a bool, so the
+        // "validation failed" message we ourselves generate can't mention
+        // "executable" by name. Just check that it's nonempty.
+        //ensure_contains("error", error, "executable");
+        ensure("string", ! error.empty());
+    }
+
     // TODO:
     // test EOF -- check logging