From 9dd7c67012e305fa61d20135e6082389e622c88a Mon Sep 17 00:00:00 2001
From: Callum Prentice <callum@gmail.com>
Date: Wed, 19 Apr 2017 16:50:56 -0700
Subject: [PATCH] Pull in improvements to LLProcess termination via a commit
from Nat Linden here:
https://bitbucket.org/rider_linden/doduo-viewer/commits/4f39500cb46e879dbb732e6547cc66f3ba39959e?at=default
---
indra/llcommon/llprocess.cpp | 12 ++--
indra/llcommon/llprocess.h | 29 +++++++-
indra/llcommon/tests/llprocess_test.cpp | 89 +++++++++++++++++++++----
3 files changed, 110 insertions(+), 20 deletions(-)
diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp
index 8c321d06b9e..5753efdc592 100644
--- a/indra/llcommon/llprocess.cpp
+++ b/indra/llcommon/llprocess.cpp
@@ -517,6 +517,10 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params)
LLProcess::LLProcess(const LLSDOrParams& params):
mAutokill(params.autokill),
+ // Because 'autokill' originally meant both 'autokill' and 'attached', to
+ // preserve existing semantics, we promise that mAttached defaults to the
+ // same setting as mAutokill.
+ mAttached(params.attached.isProvided()? params.attached : params.autokill),
mPipes(NSLOTS)
{
// Hmm, when you construct a ptr_vector with a size, it merely reserves
@@ -625,9 +629,9 @@ LLProcess::LLProcess(const LLSDOrParams& params):
// std handles and the like, and that's a bit more detachment than we
// want. autokill=false just means not to implicitly kill the child when
// the parent terminates!
-// chkapr(apr_procattr_detach_set(procattr, params.autokill? 0 : 1));
+// chkapr(apr_procattr_detach_set(procattr, mAutokill? 0 : 1));
- if (params.autokill)
+ if (mAutokill)
{
#if ! defined(APR_HAS_PROCATTR_AUTOKILL_SET)
// Our special preprocessor symbol isn't even defined -- wrong APR
@@ -696,7 +700,7 @@ LLProcess::LLProcess(const LLSDOrParams& params):
// take steps to terminate the child. This is all suspenders-and-belt: in
// theory our destructor should kill an autokill child, but in practice
// that doesn't always work (e.g. VWR-21538).
- if (params.autokill)
+ if (mAutokill)
{
/*==========================================================================*|
// NO: There may be an APR bug, not sure -- but at least on Mac, when
@@ -799,7 +803,7 @@ LLProcess::~LLProcess()
sProcessListener.dropPoll(*this);
}
- if (mAutokill)
+ if (mAttached)
{
kill("destructor");
}
diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h
index bfac4567a50..e3386ad88e4 100644
--- a/indra/llcommon/llprocess.h
+++ b/indra/llcommon/llprocess.h
@@ -167,6 +167,7 @@ class LL_COMMON_API LLProcess: public boost::noncopyable
args("args"),
cwd("cwd"),
autokill("autokill", true),
+ attached("attached", true),
files("files"),
postend("postend"),
desc("desc")
@@ -183,9 +184,31 @@ class LL_COMMON_API LLProcess: public boost::noncopyable
Multiple<std::string> args;
/// current working directory, if need it changed
Optional<std::string> cwd;
- /// implicitly kill process on destruction of LLProcess object
- /// (default true)
+ /// implicitly kill child process on termination of parent, whether
+ /// voluntary or crash (default true)
Optional<bool> autokill;
+ /// implicitly kill process on destruction of LLProcess object
+ /// (default same as autokill)
+ ///
+ /// Originally, 'autokill' conflated two concepts: kill child process on
+ /// - destruction of its LLProcess object, and
+ /// - termination of parent process, voluntary or otherwise.
+ ///
+ /// It's useful to tease these apart. Some child processes are sent a
+ /// "clean up and terminate" message before the associated LLProcess
+ /// object is destroyed. A child process launched with attached=false
+ /// has an extra time window from the destruction of its LLProcess
+ /// until parent-process termination in which to perform its own
+ /// orderly shutdown, yet autokill=true still guarantees that we won't
+ /// accumulate orphan instances of such processes indefinitely. With
+ /// attached=true, if a child process cannot clean up between the
+ /// shutdown message and LLProcess destruction (presumably very soon
+ /// thereafter), it's forcibly killed anyway -- which can lead to
+ /// distressing user-visible crash indications.
+ ///
+ /// (The usefulness of attached=true with autokill=false is less
+ /// clear, but we don't prohibit that combination.)
+ Optional<bool> attached;
/**
* Up to three FileParam items: for child stdin, stdout, stderr.
* Passing two FileParam entries means default treatment for stderr,
@@ -540,7 +563,7 @@ class LL_COMMON_API LLProcess: public boost::noncopyable
std::string mDesc;
std::string mPostend;
apr_proc_t mProcess;
- bool mAutokill;
+ bool mAutokill, mAttached;
Status mStatus;
// explicitly want this ptr_vector to be able to store NULLs
typedef boost::ptr_vector< boost::nullable<BasePipe> > PipeVector;
diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp
index 5ba343b183b..b27e125d2ec 100644
--- a/indra/llcommon/tests/llprocess_test.cpp
+++ b/indra/llcommon/tests/llprocess_test.cpp
@@ -788,6 +788,69 @@ namespace tut
template<> template<>
void object::test<10>()
+ {
+ set_test_name("attached=false");
+ // almost just like autokill=false, except set autokill=true with
+ // attached=false.
+ NamedTempFile from("from", "not started");
+ NamedTempFile to("to", "");
+ LLProcess::handle phandle(0);
+ {
+ PythonProcessLauncher py(get_test_name(),
+ "from __future__ import with_statement\n"
+ "import sys, time\n"
+ "with open(sys.argv[1], 'w') as f:\n"
+ " f.write('ok')\n"
+ "# wait for 'go' from test program\n"
+ "for i in xrange(60):\n"
+ " time.sleep(1)\n"
+ " with open(sys.argv[2]) as f:\n"
+ " go = f.read()\n"
+ " if go == 'go':\n"
+ " break\n"
+ "else:\n"
+ " with open(sys.argv[1], 'w') as f:\n"
+ " f.write('never saw go')\n"
+ " sys.exit(1)\n"
+ "# okay, saw 'go', write 'ack'\n"
+ "with open(sys.argv[1], 'w') as f:\n"
+ " f.write('ack')\n");
+ py.mParams.args.add(from.getName());
+ py.mParams.args.add(to.getName());
+ py.mParams.autokill = true;
+ py.mParams.attached = false;
+ py.launch();
+ // Capture handle for later
+ phandle = py.mPy->getProcessHandle();
+ // Wait for the script to wake up and do its first write
+ int i = 0, timeout = 60;
+ for ( ; i < timeout; ++i)
+ {
+ yield();
+ if (readfile(from.getName(), "from autokill script") == "ok")
+ break;
+ }
+ // If we broke this loop because of the counter, something's wrong
+ ensure("script never started", i < timeout);
+ // Now destroy the LLProcess, which should NOT kill the child!
+ }
+ // If the destructor killed the child anyway, give it time to die
+ yield(2);
+ // How do we know it's not terminated? By making it respond to
+ // a specific stimulus in a specific way.
+ {
+ std::ofstream outf(to.getName().c_str());
+ outf << "go";
+ } // flush and close.
+ // now wait for the script to terminate... one way or another.
+ waitfor(phandle, "autokill script");
+ // If the LLProcess destructor implicitly called kill(), the
+ // script could not have written 'ack' as we expect.
+ ensure_equals(get_test_name() + " script output", readfile(from.getName()), "ack");
+ }
+
+ template<> template<>
+ void object::test<11>()
{
set_test_name("'bogus' test");
CaptureLog recorder;
@@ -801,7 +864,7 @@ namespace tut
}
template<> template<>
- void object::test<11>()
+ void object::test<12>()
{
set_test_name("'file' test");
// Replace this test with one or more real 'file' tests when we
@@ -815,7 +878,7 @@ namespace tut
}
template<> template<>
- void object::test<12>()
+ void object::test<13>()
{
set_test_name("'tpipe' test");
// Replace this test with one or more real 'tpipe' tests when we
@@ -832,7 +895,7 @@ namespace tut
}
template<> template<>
- void object::test<13>()
+ void object::test<14>()
{
set_test_name("'npipe' test");
// Replace this test with one or more real 'npipe' tests when we
@@ -850,7 +913,7 @@ namespace tut
}
template<> template<>
- void object::test<14>()
+ void object::test<15>()
{
set_test_name("internal pipe name warning");
CaptureLog recorder;
@@ -914,7 +977,7 @@ namespace tut
} while (0)
template<> template<>
- void object::test<15>()
+ void object::test<16>()
{
set_test_name("get*Pipe() validation");
PythonProcessLauncher py(get_test_name(),
@@ -934,7 +997,7 @@ namespace tut
}
template<> template<>
- void object::test<16>()
+ void object::test<17>()
{
set_test_name("talk to stdin/stdout");
PythonProcessLauncher py(get_test_name(),
@@ -992,7 +1055,7 @@ namespace tut
}
template<> template<>
- void object::test<17>()
+ void object::test<18>()
{
set_test_name("listen for ReadPipe events");
PythonProcessLauncher py(get_test_name(),
@@ -1052,7 +1115,7 @@ namespace tut
}
template<> template<>
- void object::test<18>()
+ void object::test<19>()
{
set_test_name("ReadPipe \"eof\" event");
PythonProcessLauncher py(get_test_name(),
@@ -1078,7 +1141,7 @@ namespace tut
}
template<> template<>
- void object::test<19>()
+ void object::test<20>()
{
set_test_name("setLimit()");
PythonProcessLauncher py(get_test_name(),
@@ -1107,7 +1170,7 @@ namespace tut
}
template<> template<>
- void object::test<20>()
+ void object::test<21>()
{
set_test_name("peek() ReadPipe data");
PythonProcessLauncher py(get_test_name(),
@@ -1160,7 +1223,7 @@ namespace tut
}
template<> template<>
- void object::test<21>()
+ void object::test<22>()
{
set_test_name("bad postend");
std::string pumpname("postend");
@@ -1185,7 +1248,7 @@ namespace tut
}
template<> template<>
- void object::test<22>()
+ void object::test<23>()
{
set_test_name("good postend");
PythonProcessLauncher py(get_test_name(),
@@ -1241,7 +1304,7 @@ namespace tut
};
template<> template<>
- void object::test<23>()
+ void object::test<24>()
{
set_test_name("all data visible at postend");
PythonProcessLauncher py(get_test_name(),
--
GitLab