livie
/
phoenix-firestorm
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Pull in improvements to LLProcess termination via a commit from Nat Linden here: 4f39500cb4?at=default

master
Callum Prentice 2017-04-19 16:50:56 -07:00
parent c49eeb9a62
commit 9dd7c67012
3 changed files with 110 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
indra/llcommon/llprocess.cpp
View File

@ -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");
}

29
indra/llcommon/llprocess.h
View File

@ -167,6 +167,7 @@ public:
args("args"),
cwd("cwd"),
autokill("autokill", true),
attached("attached", true),
files("files"),
postend("postend"),
desc("desc")
@ -183,9 +184,31 @@ public:
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 @@ private:
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;

89
indra/llcommon/tests/llprocess_test.cpp
View File

@ -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(),