From f0dbb878337082d3f581874c12e6df2f4659a464 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 20 Jan 2012 18:10:40 -0500 Subject: Per Richard, replace LLProcessLauncher with LLProcess. LLProcessLauncher had the somewhat fuzzy mandate of (1) accumulating parameters with which to launch a child process and (2) sometimes tracking the lifespan of the ensuing child process. But a valid LLProcessLauncher object might or might not have ever been associated with an actual child process. LLProcess specifically tracks a child process. In effect, it's a fairly thin wrapper around a process HANDLE (on Windows) or pid_t (elsewhere), with lifespan management thrown in. A static LLProcess::create() method launches a new child; create() accepts an LLSD bundle with child parameters. So building up a parameter bundle is deferred to LLSD rather than conflated with the process management object. Reconcile all known LLProcessLauncher consumers in the viewer code base, notably the class unit tests. --- indra/llcommon/tests/llprocess_test.cpp | 706 ++++++++++++++++++++++++++++++++ 1 file changed, 706 insertions(+) create mode 100644 indra/llcommon/tests/llprocess_test.cpp (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp new file mode 100644 index 0000000000..55e22abd81 --- /dev/null +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -0,0 +1,706 @@ +/** + * @file llprocess_test.cpp + * @author Nat Goodspeed + * @date 2011-12-19 + * @brief Test for llprocess. + * + * $LicenseInfo:firstyear=2011&license=viewerlgpl$ + * Copyright (c) 2011, Linden Research, Inc. + * $/LicenseInfo$ + */ + +// Precompiled header +#include "linden_common.h" +// associated header +#include "llprocess.h" +// STL headers +#include +#include +// std headers +#include +// external library headers +#include "llapr.h" +#include "apr_thread_proc.h" +#include +#include +#include +#include +//#include +//#include +// other Linden headers +#include "../test/lltut.h" +#include "../test/manageapr.h" +#include "../test/namedtempfile.h" +#include "stringize.h" +#include "llsdutil.h" + +#if defined(LL_WINDOWS) +#define sleep(secs) _sleep((secs) * 1000) +#define EOL "\r\n" +#else +#define EOL "\n" +#include +#endif + +//namespace lambda = boost::lambda; + +// static instance of this manages APR init/cleanup +static ManageAPR manager; + +/***************************************************************************** +* Helpers +*****************************************************************************/ + +#define ensure_equals_(left, right) \ + ensure_equals(STRINGIZE(#left << " != " << #right), (left), (right)) + +#define aprchk(expr) aprchk_(#expr, (expr)) +static void aprchk_(const char* call, apr_status_t rv, apr_status_t expected=APR_SUCCESS) +{ + tut::ensure_equals(STRINGIZE(call << " => " << rv << ": " << manager.strerror(rv)), + rv, expected); +} + +/** + * Read specified file using std::getline(). It is assumed to be an error if + * the file is empty: don't use this function if that's an acceptable case. + * Last line will not end with '\n'; this is to facilitate the usual case of + * string compares with a single line of output. + * @param pathname The file to read. + * @param desc Optional description of the file for error message; + * defaults to "in " + */ +static std::string readfile(const std::string& pathname, const std::string& desc="") +{ + std::string use_desc(desc); + if (use_desc.empty()) + { + use_desc = STRINGIZE("in " << pathname); + } + std::ifstream inf(pathname.c_str()); + std::string output; + tut::ensure(STRINGIZE("No output " << use_desc), std::getline(inf, output)); + std::string more; + while (std::getline(inf, more)) + { + output += '\n' + more; + } + return output; +} + +/** + * Construct an LLProcess to run a Python script. + */ +struct PythonProcessLauncher +{ + /** + * @param desc Arbitrary description for error messages + * @param script Python script, any form acceptable to NamedTempFile, + * typically either a std::string or an expression of the form + * (lambda::_1 << "script content with " << variable_data) + */ + template + PythonProcessLauncher(const std::string& desc, const CONTENT& script): + mDesc(desc), + mScript("py", script) + { + const char* PYTHON(getenv("PYTHON")); + tut::ensure("Set $PYTHON to the Python interpreter", PYTHON); + + mParams["executable"] = PYTHON; + mParams["args"].append(mScript.getName()); + } + + /// Run Python script and wait for it to complete. + void run() + { + mPy = LLProcess::create(mParams); + tut::ensure(STRINGIZE("Couldn't launch " << mDesc << " script"), mPy); + // One of the irritating things about LLProcess is that + // 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); + } + } + + /** + * Run a Python script using LLProcess, expecting that it will + * write to the file passed as its sys.argv[1]. Retrieve that output. + * + * Until January 2012, LLProcess provided distressingly few + * mechanisms for a child process to communicate back to its caller -- + * not even its return code. We've introduced a convention by which we + * create an empty temp file, pass the name of that file to our child + * as sys.argv[1] and expect the script to write its output to that + * file. This function implements the C++ (parent process) side of + * that convention. + */ + std::string run_read() + { + NamedTempFile out("out", ""); // placeholder + // pass name of this temporary file to the script + mParams["args"].append(out.getName()); + run(); + // assuming the script wrote to that file, read it + return readfile(out.getName(), STRINGIZE("from " << mDesc << " script")); + } + + LLSD mParams; + LLProcessPtr mPy; + std::string mDesc; + NamedTempFile mScript; +}; + +/// convenience function for PythonProcessLauncher::run() +template +static void python(const std::string& desc, const CONTENT& script) +{ + PythonProcessLauncher py(desc, script); + py.run(); +} + +/// convenience function for PythonProcessLauncher::run_read() +template +static std::string python_out(const std::string& desc, const CONTENT& script) +{ + PythonProcessLauncher py(desc, script); + return py.run_read(); +} + +/// Create a temporary directory and clean it up later. +class NamedTempDir: public boost::noncopyable +{ +public: + // Use python() function to create a temp directory: I've found + // nothing in either Boost.Filesystem or APR quite like Python's + // tempfile.mkdtemp(). + // Special extra bonus: on Mac, mkdtemp() reports a pathname + // starting with /var/folders/something, whereas that's really a + // symlink to /private/var/folders/something. Have to use + // realpath() to compare properly. + NamedTempDir(): + mPath(python_out("mkdtemp()", + "from __future__ import with_statement\n" + "import os.path, sys, tempfile\n" + "with open(sys.argv[1], 'w') as f:\n" + " f.write(os.path.realpath(tempfile.mkdtemp()))\n")) + {} + + ~NamedTempDir() + { + aprchk(apr_dir_remove(mPath.c_str(), gAPRPoolp)); + } + + std::string getName() const { return mPath; } + +private: + std::string mPath; +}; + +/***************************************************************************** +* TUT +*****************************************************************************/ +namespace tut +{ + struct llprocess_data + { + LLAPRPool pool; + }; + typedef test_group llprocess_group; + typedef llprocess_group::object object; + llprocess_group llprocessgrp("llprocess"); + + struct Item + { + Item(): tries(0) {} + unsigned tries; + std::string which; + std::string what; + }; + +/*==========================================================================*| +#define tabent(symbol) { symbol, #symbol } + static struct ReasonCode + { + int code; + const char* name; + } reasons[] = + { + tabent(APR_OC_REASON_DEATH), + tabent(APR_OC_REASON_UNWRITABLE), + tabent(APR_OC_REASON_RESTART), + tabent(APR_OC_REASON_UNREGISTER), + tabent(APR_OC_REASON_LOST), + tabent(APR_OC_REASON_RUNNING) + }; +#undef tabent +|*==========================================================================*/ + + struct WaitInfo + { + WaitInfo(apr_proc_t* child_): + child(child_), + rv(-1), // we haven't yet called apr_proc_wait() + rc(0), + why(apr_exit_why_e(0)) + {} + apr_proc_t* child; // which subprocess + apr_status_t rv; // return from apr_proc_wait() + int rc; // child's exit code + apr_exit_why_e why; // APR_PROC_EXIT, APR_PROC_SIGNAL, APR_PROC_SIGNAL_CORE + }; + + void child_status_callback(int reason, void* data, int status) + { +/*==========================================================================*| + std::string reason_str; + BOOST_FOREACH(const ReasonCode& rcp, reasons) + { + if (reason == rcp.code) + { + reason_str = rcp.name; + break; + } + } + if (reason_str.empty()) + { + reason_str = STRINGIZE("unknown reason " << reason); + } + std::cout << "child_status_callback(" << reason_str << ")\n"; +|*==========================================================================*/ + + if (reason == APR_OC_REASON_DEATH || reason == APR_OC_REASON_LOST) + { + // Somewhat oddly, APR requires that you explicitly unregister + // even when it already knows the child has terminated. + apr_proc_other_child_unregister(data); + + WaitInfo* wi(static_cast(data)); + // It's just wrong to call apr_proc_wait() here. The only way APR + // knows to call us with APR_OC_REASON_DEATH is that it's already + // reaped this child process, so calling wait() will only produce + // "huh?" from the OS. We must rely on the status param passed in, + // which unfortunately comes straight from the OS wait() call. +// wi->rv = apr_proc_wait(wi->child, &wi->rc, &wi->why, APR_NOWAIT); + wi->rv = APR_CHILD_DONE; // fake apr_proc_wait() results +#if defined(LL_WINDOWS) + wi->why = APR_PROC_EXIT; + wi->rc = status; // no encoding on Windows (no signals) +#else // Posix + if (WIFEXITED(status)) + { + wi->why = APR_PROC_EXIT; + wi->rc = WEXITSTATUS(status); + } + else if (WIFSIGNALED(status)) + { + wi->why = APR_PROC_SIGNAL; + wi->rc = WTERMSIG(status); + } + else // uh, shouldn't happen? + { + wi->why = APR_PROC_EXIT; + wi->rc = status; // someone else will have to decode + } +#endif // Posix + } + } + + template<> template<> + void object::test<1>() + { + set_test_name("raw APR nonblocking I/O"); + + // Create a script file in a temporary place. + NamedTempFile script("py", + "import sys" EOL + "import time" EOL + EOL + "time.sleep(2)" EOL + "print >>sys.stdout, 'stdout after wait'" EOL + "sys.stdout.flush()" EOL + "time.sleep(2)" EOL + "print >>sys.stderr, 'stderr after wait'" EOL + "sys.stderr.flush()" EOL + ); + + // Arrange to track the history of our interaction with child: what we + // fetched, which pipe it came from, how many tries it took before we + // got it. + std::vector history; + history.push_back(Item()); + + // Run the child process. + apr_procattr_t *procattr = NULL; + aprchk(apr_procattr_create(&procattr, pool.getAPRPool())); + aprchk(apr_procattr_io_set(procattr, APR_CHILD_BLOCK, APR_CHILD_BLOCK, APR_CHILD_BLOCK)); + aprchk(apr_procattr_cmdtype_set(procattr, APR_PROGRAM_PATH)); + + std::vector argv; + apr_proc_t child; + argv.push_back("python"); + // Have to have a named copy of this std::string so its c_str() value + // will persist. + std::string scriptname(script.getName()); + argv.push_back(scriptname.c_str()); + argv.push_back(NULL); + + aprchk(apr_proc_create(&child, argv[0], + &argv[0], + NULL, // if we wanted to pass explicit environment + procattr, + pool.getAPRPool())); + + // We do not want this child process to outlive our APR pool. On + // destruction of the pool, forcibly kill the process. Tell APR to try + // SIGTERM and wait 3 seconds. If that didn't work, use SIGKILL. + apr_pool_note_subprocess(pool.getAPRPool(), &child, APR_KILL_AFTER_TIMEOUT); + + // arrange to call child_status_callback() + WaitInfo wi(&child); + apr_proc_other_child_register(&child, child_status_callback, &wi, child.in, pool.getAPRPool()); + + // TODO: + // Stuff child.in until it (would) block to verify EWOULDBLOCK/EAGAIN. + // Have child script clear it later, then write one more line to prove + // that it gets through. + + // Monitor two different output pipes. Because one will be closed + // before the other, keep them in a list so we can drop whichever of + // them is closed first. + typedef std::pair DescFile; + typedef std::list DescFileList; + DescFileList outfiles; + outfiles.push_back(DescFile("out", child.out)); + outfiles.push_back(DescFile("err", child.err)); + + while (! outfiles.empty()) + { + // This peculiar for loop is designed to let us erase(dfli). With + // a list, that invalidates only dfli itself -- but even so, we + // lose the ability to increment it for the next item. So at the + // top of every loop, while dfli is still valid, increment + // dflnext. Then before the next iteration, set dfli to dflnext. + for (DescFileList::iterator + dfli(outfiles.begin()), dflnext(outfiles.begin()), dflend(outfiles.end()); + dfli != dflend; dfli = dflnext) + { + // Only valid to increment dflnext once we're sure it's not + // already at dflend. + ++dflnext; + + char buf[4096]; + + apr_status_t rv = apr_file_gets(buf, sizeof(buf), dfli->second); + if (APR_STATUS_IS_EOF(rv)) + { +// std::cout << "(EOF on " << dfli->first << ")\n"; +// history.back().which = dfli->first; +// history.back().what = "*eof*"; +// history.push_back(Item()); + outfiles.erase(dfli); + continue; + } + if (rv == EWOULDBLOCK || rv == EAGAIN) + { +// std::cout << "(waiting; apr_file_gets(" << dfli->first << ") => " << rv << ": " << manager.strerror(rv) << ")\n"; + ++history.back().tries; + continue; + } + aprchk_("apr_file_gets(buf, sizeof(buf), dfli->second)", rv); + // Is it even possible to get APR_SUCCESS but read 0 bytes? + // Hope not, but defend against that anyway. + if (buf[0]) + { +// std::cout << dfli->first << ": " << buf; + history.back().which = dfli->first; + history.back().what.append(buf); + if (buf[strlen(buf) - 1] == '\n') + history.push_back(Item()); + else + { + // Just for pretty output... if we only read a partial + // line, terminate it. +// std::cout << "...\n"; + } + } + } + // Do this once per tick, as we expect the viewer will + apr_proc_other_child_refresh_all(APR_OC_REASON_RUNNING); + sleep(1); + } + apr_file_close(child.in); + apr_file_close(child.out); + apr_file_close(child.err); + + // Okay, we've broken the loop because our pipes are all closed. If we + // haven't yet called wait, give the callback one more chance. This + // models the fact that unlike this small test program, the viewer + // will still be running. + if (wi.rv == -1) + { + std::cout << "last gasp apr_proc_other_child_refresh_all()\n"; + apr_proc_other_child_refresh_all(APR_OC_REASON_RUNNING); + } + + if (wi.rv == -1) + { + std::cout << "child_status_callback(APR_OC_REASON_DEATH) wasn't called" << std::endl; + wi.rv = apr_proc_wait(wi.child, &wi.rc, &wi.why, APR_NOWAIT); + } +// std::cout << "child done: rv = " << rv << " (" << manager.strerror(rv) << "), why = " << why << ", rc = " << rc << '\n'; + aprchk_("apr_proc_wait(wi->child, &wi->rc, &wi->why, APR_NOWAIT)", wi.rv, APR_CHILD_DONE); + ensure_equals_(wi.why, APR_PROC_EXIT); + ensure_equals_(wi.rc, 0); + + // Beyond merely executing all the above successfully, verify that we + // obtained expected output -- and that we duly got control while + // waiting, proving the non-blocking nature of these pipes. + try + { + unsigned i = 0; + ensure("blocking I/O on child pipe (0)", history[i].tries); + ensure_equals_(history[i].which, "out"); + ensure_equals_(history[i].what, "stdout after wait" EOL); +// ++i; +// ensure_equals_(history[i].which, "out"); +// ensure_equals_(history[i].what, "*eof*"); + ++i; + ensure("blocking I/O on child pipe (1)", history[i].tries); + ensure_equals_(history[i].which, "err"); + ensure_equals_(history[i].what, "stderr after wait" EOL); +// ++i; +// ensure_equals_(history[i].which, "err"); +// ensure_equals_(history[i].what, "*eof*"); + } + catch (const failure&) + { + std::cout << "History:\n"; + BOOST_FOREACH(const Item& item, history) + { + std::string what(item.what); + if ((! what.empty()) && what[what.length() - 1] == '\n') + { + what.erase(what.length() - 1); + if ((! what.empty()) && what[what.length() - 1] == '\r') + { + what.erase(what.length() - 1); + what.append("\\r"); + } + what.append("\\n"); + } + std::cout << " " << item.which << ": '" << what << "' (" + << item.tries << " tries)\n"; + } + std::cout << std::flush; + // re-raise same error; just want to enrich the output + throw; + } + } + + template<> template<> + void object::test<2>() + { + set_test_name("setWorkingDirectory()"); + // We want to test setWorkingDirectory(). But what directory is + // guaranteed to exist on every machine, under every OS? Have to + // create one. Naturally, ensure we clean it up when done. + NamedTempDir tempdir; + PythonProcessLauncher py("getcwd()", + "from __future__ import with_statement\n" + "import os, sys\n" + "with open(sys.argv[1], 'w') as f:\n" + " f.write(os.getcwd())\n"); + // Before running, call setWorkingDirectory() + py.mParams["cwd"] = tempdir.getName(); + ensure_equals("os.getcwd()", py.run_read(), tempdir.getName()); + } + + template<> template<> + void object::test<3>() + { + set_test_name("arguments"); + PythonProcessLauncher py("args", + "from __future__ import with_statement\n" + "import sys\n" + // note nonstandard output-file arg! + "with open(sys.argv[3], 'w') as f:\n" + " for arg in sys.argv[1:]:\n" + " print >>f, arg\n"); + // We expect that PythonProcessLauncher has already appended + // its own NamedTempFile to mParams["args"] (sys.argv[0]). + py.mParams["args"].append("first arg"); // sys.argv[1] + py.mParams["args"].append("second arg"); // sys.argv[2] + // run_read() appends() one more argument, hence [3] + std::string output(py.run_read()); + boost::split_iterator + li(output, boost::first_finder("\n")), lend; + ensure("didn't get first arg", li != lend); + std::string arg(li->begin(), li->end()); + ensure_equals(arg, "first arg"); + ++li; + ensure("didn't get second arg", li != lend); + arg.assign(li->begin(), li->end()); + ensure_equals(arg, "second arg"); + ++li; + ensure("didn't get output filename?!", li != lend); + arg.assign(li->begin(), li->end()); + ensure("output filename empty?!", ! arg.empty()); + ++li; + ensure("too many args", li == lend); + } + + template<> template<> + void object::test<4>() + { + set_test_name("explicit kill()"); + PythonProcessLauncher py("kill()", + "from __future__ import with_statement\n" + "import sys, time\n" + "with open(sys.argv[1], 'w') as f:\n" + " f.write('ok')\n" + "# now sleep; expect caller to kill\n" + "time.sleep(120)\n" + "# if caller hasn't managed to kill by now, bad\n" + "with open(sys.argv[1], 'w') as f:\n" + " f.write('bad')\n"); + NamedTempFile out("out", "not started"); + py.mParams["args"].append(out.getName()); + py.mPy = LLProcess::create(py.mParams); + ensure("couldn't launch kill() script", py.mPy); + // Wait for the script to wake up and do its first write + int i = 0, timeout = 60; + for ( ; i < timeout; ++i) + { + sleep(1); + if (readfile(out.getName(), "from kill() script") == "ok") + break; + } + // If we broke this loop because of the counter, something's wrong + ensure("script never started", i < timeout); + // 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); + } + // 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. + ensure_equals("kill() script output", readfile(out.getName()), "ok"); + } + + template<> template<> + void object::test<5>() + { + set_test_name("implicit kill()"); + NamedTempFile out("out", "not started"); + LLProcess::id pid(0); + { + PythonProcessLauncher py("kill()", + "from __future__ import with_statement\n" + "import sys, time\n" + "with open(sys.argv[1], 'w') as f:\n" + " f.write('ok')\n" + "# now sleep; expect caller to kill\n" + "time.sleep(120)\n" + "# if caller hasn't managed to kill by now, bad\n" + "with open(sys.argv[1], 'w') as f:\n" + " f.write('bad')\n"); + py.mParams["args"].append(out.getName()); + py.mPy = LLProcess::create(py.mParams); + ensure("couldn't launch kill() script", py.mPy); + // Capture id for later + pid = py.mPy->getProcessID(); + // Wait for the script to wake up and do its first write + int i = 0, timeout = 60; + for ( ; i < timeout; ++i) + { + sleep(1); + if (readfile(out.getName(), "from kill() script") == "ok") + break; + } + // If we broke this loop because of the counter, something's wrong + ensure("script never started", i < timeout); + // Script has performed its first write and should now be sleeping. + // Destroy the LLProcess, which should kill the child. + } + // wait for the script to terminate... one way or another. + while (LLProcess::isRunning(pid)) + { + sleep(1); + } + // 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. + ensure_equals("kill() script output", readfile(out.getName()), "ok"); + } + + template<> template<> + void object::test<6>() + { + set_test_name("autokill"); + NamedTempFile from("from", "not started"); + NamedTempFile to("to", ""); + LLProcess::id pid(0); + { + PythonProcessLauncher py("autokill", + "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"].append(from.getName()); + py.mParams["args"].append(to.getName()); + py.mParams["autokill"] = false; + py.mPy = LLProcess::create(py.mParams); + ensure("couldn't launch kill() script", py.mPy); + // Capture id for later + pid = py.mPy->getProcessID(); + // Wait for the script to wake up and do its first write + int i = 0, timeout = 60; + for ( ; i < timeout; ++i) + { + sleep(1); + 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 + sleep(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. + while (LLProcess::isRunning(pid)) + { + sleep(1); + } + // 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"); + } +} // namespace tut -- cgit v1.3 From 47d94757075e338c480ba4d7d24948242a85a9bb Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sat, 21 Jan 2012 11:45:15 -0500 Subject: Convert LLProcess consumers from LLSD to LLProcess::Params block. Using a Params block gives compile-time checking against attribute typos. One might inadvertently set myLLSD["autofill"] = false and only discover it when things behave strangely at runtime; but trying to set myParams.autofill will produce a compile error. However, it's excellent that the same LLProcess::create() method can accept either LLProcess::Params or a properly-constructed LLSD block. --- indra/llcommon/tests/llprocess_test.cpp | 26 ++++++++--------- indra/llcommon/tests/llsdserialize_test.cpp | 6 ++-- indra/llplugin/llpluginprocessparent.cpp | 28 +++++++++--------- indra/llplugin/llpluginprocessparent.h | 12 ++++---- indra/newview/llexternaleditor.cpp | 33 ++++++++++++---------- indra/newview/llexternaleditor.h | 5 ++-- .../updater/llupdateinstaller.cpp | 12 ++++---- 7 files changed, 62 insertions(+), 60 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 55e22abd81..405540e436 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -107,8 +107,8 @@ struct PythonProcessLauncher const char* PYTHON(getenv("PYTHON")); tut::ensure("Set $PYTHON to the Python interpreter", PYTHON); - mParams["executable"] = PYTHON; - mParams["args"].append(mScript.getName()); + mParams.executable = PYTHON; + mParams.args.add(mScript.getName()); } /// Run Python script and wait for it to complete. @@ -142,13 +142,13 @@ struct PythonProcessLauncher { NamedTempFile out("out", ""); // placeholder // pass name of this temporary file to the script - mParams["args"].append(out.getName()); + mParams.args.add(out.getName()); run(); // assuming the script wrote to that file, read it return readfile(out.getName(), STRINGIZE("from " << mDesc << " script")); } - LLSD mParams; + LLProcess::Params mParams; LLProcessPtr mPy; std::string mDesc; NamedTempFile mScript; @@ -515,7 +515,7 @@ namespace tut "with open(sys.argv[1], 'w') as f:\n" " f.write(os.getcwd())\n"); // Before running, call setWorkingDirectory() - py.mParams["cwd"] = tempdir.getName(); + py.mParams.cwd = tempdir.getName(); ensure_equals("os.getcwd()", py.run_read(), tempdir.getName()); } @@ -531,9 +531,9 @@ namespace tut " for arg in sys.argv[1:]:\n" " print >>f, arg\n"); // We expect that PythonProcessLauncher has already appended - // its own NamedTempFile to mParams["args"] (sys.argv[0]). - py.mParams["args"].append("first arg"); // sys.argv[1] - py.mParams["args"].append("second arg"); // sys.argv[2] + // its own NamedTempFile to mParams.args (sys.argv[0]). + py.mParams.args.add("first arg"); // sys.argv[1] + py.mParams.args.add("second arg"); // sys.argv[2] // run_read() appends() one more argument, hence [3] std::string output(py.run_read()); boost::split_iterator @@ -568,7 +568,7 @@ namespace tut "with open(sys.argv[1], 'w') as f:\n" " f.write('bad')\n"); NamedTempFile out("out", "not started"); - py.mParams["args"].append(out.getName()); + py.mParams.args.add(out.getName()); py.mPy = LLProcess::create(py.mParams); ensure("couldn't launch kill() script", py.mPy); // Wait for the script to wake up and do its first write @@ -611,7 +611,7 @@ namespace tut "# if caller hasn't managed to kill by now, bad\n" "with open(sys.argv[1], 'w') as f:\n" " f.write('bad')\n"); - py.mParams["args"].append(out.getName()); + py.mParams.args.add(out.getName()); py.mPy = LLProcess::create(py.mParams); ensure("couldn't launch kill() script", py.mPy); // Capture id for later @@ -667,9 +667,9 @@ namespace tut "# okay, saw 'go', write 'ack'\n" "with open(sys.argv[1], 'w') as f:\n" " f.write('ack')\n"); - py.mParams["args"].append(from.getName()); - py.mParams["args"].append(to.getName()); - py.mParams["autokill"] = false; + py.mParams.args.add(from.getName()); + py.mParams.args.add(to.getName()); + py.mParams.autokill = false; py.mPy = LLProcess::create(py.mParams); ensure("couldn't launch kill() script", py.mPy); // Capture id for later diff --git a/indra/llcommon/tests/llsdserialize_test.cpp b/indra/llcommon/tests/llsdserialize_test.cpp index 7756ba6226..e625545763 100644 --- a/indra/llcommon/tests/llsdserialize_test.cpp +++ b/indra/llcommon/tests/llsdserialize_test.cpp @@ -1557,9 +1557,9 @@ namespace tut } #else // LL_DARWIN, LL_LINUX - LLSD params; - params["executable"] = PYTHON; - params["args"].append(scriptfile.getName()); + LLProcess::Params params; + params.executable = PYTHON; + params.args.add(scriptfile.getName()); LLProcessPtr py(LLProcess::create(params)); ensure(STRINGIZE("Couldn't launch " << desc << " script"), py); // Implementing timeout would mean messing with alarm() and diff --git a/indra/llplugin/llpluginprocessparent.cpp b/indra/llplugin/llpluginprocessparent.cpp index 9b225cabb8..f10eaee5b4 100644 --- a/indra/llplugin/llpluginprocessparent.cpp +++ b/indra/llplugin/llpluginprocessparent.cpp @@ -163,8 +163,8 @@ void LLPluginProcessParent::errorState(void) void LLPluginProcessParent::init(const std::string &launcher_filename, const std::string &plugin_dir, const std::string &plugin_filename, bool debug) { - mProcessParams["executable"] = launcher_filename; - mProcessParams["cwd"] = plugin_dir; + mProcessParams.executable = launcher_filename; + mProcessParams.cwd = plugin_dir; mPluginFile = plugin_filename; mPluginDir = plugin_dir; mCPUUsage = 0.0f; @@ -375,7 +375,7 @@ void LLPluginProcessParent::idle(void) // Launch the plugin process. // Only argument to the launcher is the port number we're listening on - mProcessParams["args"].append(stringize(mBoundPort)); + mProcessParams.args.add(stringize(mBoundPort)); if (! (mProcess = LLProcess::create(mProcessParams))) { errorState(); @@ -390,17 +390,17 @@ void LLPluginProcessParent::idle(void) // The command we're constructing would look like this on the command line: // osascript -e 'tell application "Terminal"' -e 'set win to do script "gdb -pid 12345"' -e 'do script "continue" in win' -e 'end tell' - LLSD params; - params["executable"] = "/usr/bin/osascript"; - params["args"].append("-e"); - params["args"].append("tell application \"Terminal\""); - params["args"].append("-e"); - params["args"].append(STRINGIZE("set win to do script \"gdb -pid " - << mProcess->getProcessID() << "\"")); - params["args"].append("-e"); - params["args"].append("do script \"continue\" in win"); - params["args"].append("-e"); - params["args"].append("end tell"); + LLProcess::Params params; + params.executable = "/usr/bin/osascript"; + params.args.add("-e"); + params.args.add("tell application \"Terminal\""); + params.args.add("-e"); + params.args.add(STRINGIZE("set win to do script \"gdb -pid " + << mProcess->getProcessID() << "\"")); + params.args.add("-e"); + params.args.add("do script \"continue\" in win"); + params.args.add("-e"); + params.args.add("end tell"); mDebugger = LLProcess::create(params); #endif diff --git a/indra/llplugin/llpluginprocessparent.h b/indra/llplugin/llpluginprocessparent.h index e8bcba75e0..990fc5cbae 100644 --- a/indra/llplugin/llpluginprocessparent.h +++ b/indra/llplugin/llpluginprocessparent.h @@ -140,27 +140,27 @@ private: }; EState mState; void setState(EState state); - + bool pluginLockedUp(); bool pluginLockedUpOrQuit(); bool accept(); - + LLSocket::ptr_t mListenSocket; LLSocket::ptr_t mSocket; U32 mBoundPort; - LLSD mProcessParams; + LLProcess::Params mProcessParams; LLProcessPtr mProcess; - + std::string mPluginFile; std::string mPluginDir; LLPluginProcessParentOwner *mOwner; - + typedef std::map sharedMemoryRegionsType; sharedMemoryRegionsType mSharedMemoryRegions; - + LLSD mMessageClassVersions; std::string mPluginVersionString; diff --git a/indra/newview/llexternaleditor.cpp b/indra/newview/llexternaleditor.cpp index ba58cd8067..3dfebad958 100644 --- a/indra/newview/llexternaleditor.cpp +++ b/indra/newview/llexternaleditor.cpp @@ -60,22 +60,22 @@ LLExternalEditor::EErrorCode LLExternalEditor::setCommand(const std::string& env } // Save command. - mProcessParams["executable"] = bin_path; - mProcessParams["args"].clear(); + mProcessParams = LLProcess::Params(); + mProcessParams.executable = bin_path; for (size_t i = 1; i < tokens.size(); ++i) { - mProcessParams["args"].append(tokens[i]); + mProcessParams.args.add(tokens[i]); } // Add the filename marker if missing. if (cmd.find(sFilenameMarker) == std::string::npos) { - mProcessParams["args"].append(sFilenameMarker); + mProcessParams.args.add(sFilenameMarker); llinfos << "Adding the filename marker (" << sFilenameMarker << ")" << llendl; } llinfos << "Setting command [" << bin_path; - BOOST_FOREACH(const std::string& arg, llsd::inArray(mProcessParams["args"])) + BOOST_FOREACH(const std::string& arg, mProcessParams.args) { llcont << " \"" << arg << "\""; } @@ -86,33 +86,36 @@ LLExternalEditor::EErrorCode LLExternalEditor::setCommand(const std::string& env LLExternalEditor::EErrorCode LLExternalEditor::run(const std::string& file_path) { - if (mProcessParams["executable"].asString().empty() || ! mProcessParams["args"].size()) + // LLInitParams type wrappers don't seem to have empty() or size() + // methods; try determining emptiness by comparing begin/end iterators. + if (std::string(mProcessParams.executable).empty() || + (mProcessParams.args.begin() == mProcessParams.args.end())) { llwarns << "Editor command not set" << llendl; return EC_NOT_SPECIFIED; } // Copy params block so we can replace sFilenameMarker - LLSD params(mProcessParams); + LLProcess::Params params; + params.executable = mProcessParams.executable; // Substitute the filename marker in the command with the actual passed file name. - LLSD& args(params["args"]); - for (LLSD::array_iterator ai(args.beginArray()), aend(args.endArray()); ai != aend; ++ai) + BOOST_FOREACH(const std::string& arg, mProcessParams.args) { - std::string sarg(*ai); - LLStringUtil::replaceString(sarg, sFilenameMarker, file_path); - *ai = sarg; + std::string fixed(arg); + LLStringUtil::replaceString(fixed, sFilenameMarker, file_path); + params.args.add(fixed); } // Run the editor. - llinfos << "Running editor command [" << params["executable"]; - BOOST_FOREACH(const std::string& arg, llsd::inArray(params["args"])) + llinfos << "Running editor command [" << std::string(params.executable); + BOOST_FOREACH(const std::string& arg, params.args) { llcont << " \"" << arg << "\""; } llcont << "]" << llendl; // Prevent killing the process in destructor. - params["autokill"] = false; + params.autokill = false; return LLProcess::create(params) ? EC_SUCCESS : EC_FAILED_TO_RUN; } diff --git a/indra/newview/llexternaleditor.h b/indra/newview/llexternaleditor.h index e81c360c24..fd2c25020c 100644 --- a/indra/newview/llexternaleditor.h +++ b/indra/newview/llexternaleditor.h @@ -27,7 +27,7 @@ #ifndef LL_LLEXTERNALEDITOR_H #define LL_LLEXTERNALEDITOR_H -#include "llsd.h" +#include "llprocess.h" /** * Usage: @@ -97,8 +97,7 @@ private: */ static const std::string sSetting; - - LLSD mProcessParams; + LLProcess::Params mProcessParams; }; #endif // LL_LLEXTERNALEDITOR_H diff --git a/indra/viewer_components/updater/llupdateinstaller.cpp b/indra/viewer_components/updater/llupdateinstaller.cpp index e99fd0af7e..2f87d59373 100644 --- a/indra/viewer_components/updater/llupdateinstaller.cpp +++ b/indra/viewer_components/updater/llupdateinstaller.cpp @@ -78,12 +78,12 @@ int ll_install_update(std::string const & script, llinfos << "UpdateInstaller: installing " << updatePath << " using " << actualScriptPath << LL_ENDL; - LLSD params; - params["executable"] = actualScriptPath; - params["args"].append(updatePath); - params["args"].append(ll_install_failed_marker_path()); - params["args"].append(boost::lexical_cast(required)); - params["autokill"] = false; + LLProcess::Params params; + params.executable = actualScriptPath; + params.args.add(updatePath); + params.args.add(ll_install_failed_marker_path()); + params.args.add(boost::lexical_cast(required)); + params.autokill = false; return LLProcess::create(params)? 0 : -1; } -- cgit v1.3 From 85581eefa63d8f8e8c5132c4cd7e137f6cb88869 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 30 Jan 2012 12:11:44 -0500 Subject: Expose 'handle' as well as 'id' on LLProcess objects. On Posix, these and the corresponding getProcessID()/getProcessHandle() accessors produce the same pid_t value; but on Windows, it's useful to distinguish an int-like 'id' useful to human log readers versus an opaque 'handle' for passing to platform-specific API functions. So make the distinction in a platform-independent way. --- indra/llcommon/llprocess.cpp | 50 ++++++++++++++++++++------------- indra/llcommon/llprocess.h | 42 +++++++++++++++++---------- indra/llcommon/tests/llprocess_test.cpp | 16 +++++------ 3 files changed, 66 insertions(+), 42 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 8c0e8fe65e..a7bafb8cb0 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -42,7 +42,7 @@ struct LLProcessError: public std::runtime_error LLProcessError(const std::string& msg): std::runtime_error(msg) {} }; -LLProcessPtr LLProcess::create(const LLSDParamAdapter& params) +LLProcessPtr LLProcess::create(const LLSDOrParams& params) { try { @@ -55,8 +55,9 @@ LLProcessPtr LLProcess::create(const LLSDParamAdapter& params) } } -LLProcess::LLProcess(const LLSDParamAdapter& params): +LLProcess::LLProcess(const LLSDOrParams& params): mProcessID(0), + mProcessHandle(0), mAutokill(params.autokill) { if (! params.validateBlock(true)) @@ -78,8 +79,18 @@ LLProcess::~LLProcess() bool LLProcess::isRunning(void) { - mProcessID = isRunning(mProcessID, mDesc); - return (mProcessID != 0); + mProcessHandle = isRunning(mProcessHandle, mDesc); + return (mProcessHandle != 0); +} + +LLProcess::id LLProcess::getProcessID() const +{ + return mProcessID; +} + +LLProcess::handle LLProcess::getProcessHandle() const +{ + return mProcessHandle; } std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params) @@ -122,7 +133,7 @@ static std::string WindowsErrorString(const std::string& operation); class LLJob: public LLSingleton { public: - void assignProcess(const std::string& prog, HANDLE hProcess) + void assignProcess(const std::string& prog, handle hProcess) { // If we never managed to initialize this Job Object, can't use it -- // but don't keep spamming the log, we already emitted warnings when @@ -164,10 +175,10 @@ private: } } - HANDLE mJob; + handle mJob; }; -void LLProcess::launch(const LLSDParamAdapter& params) +void LLProcess::launch(const LLSDOrParams& params) { PROCESS_INFORMATION pinfo; STARTUPINFOA sinfo = { sizeof(sinfo) }; @@ -201,28 +212,28 @@ void LLProcess::launch(const LLSDParamAdapter& params) throw LLProcessError(WindowsErrorString("CreateProcessA")); } - // foo = pinfo.dwProcessId; // get your pid here if you want to use it later on // CloseHandle(pinfo.hProcess); // stops leaks - nothing else - mProcessID = pinfo.hProcess; + mProcessID = pinfo.dwProcessId; + mProcessHandle = pinfo.hProcess; CloseHandle(pinfo.hThread); // stops leaks - nothing else - mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << pinfo.dwProcessId << ')'); - LL_INFOS("LLProcess") << "Launched " << params << " (" << pinfo.dwProcessId << ")" << LL_ENDL; + mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcessID << ')'); + LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcessID << ")" << LL_ENDL; // Now associate the new child process with our Job Object -- unless // autokill is false, i.e. caller asserts the child should persist. if (params.autokill) { - LLJob::instance().assignProcess(mDesc, mProcessID); + LLJob::instance().assignProcess(mDesc, mProcessHandle); } } -LLProcess::id LLProcess::isRunning(id handle, const std::string& desc) +LLProcess::handle LLProcess::isRunning(handle h, const std::string& desc) { - if (! handle) + if (! h) return 0; - DWORD waitresult = WaitForSingleObject(handle, 0); + DWORD waitresult = WaitForSingleObject(h, 0); if(waitresult == WAIT_OBJECT_0) { // the process has completed. @@ -233,16 +244,16 @@ LLProcess::id LLProcess::isRunning(id handle, const std::string& desc) return 0; } - return handle; + return h; } bool LLProcess::kill(void) { - if (! mProcessID) + if (! mProcessHandle) return false; LL_INFOS("LLProcess") << "killing " << mDesc << LL_ENDL; - TerminateProcess(mProcessID, 0); + TerminateProcess(mProcessHandle, 0); return ! isRunning(); } @@ -302,7 +313,7 @@ static bool reap_pid(pid_t pid) return false; } -void LLProcess::launch(const LLSDParamAdapter& params) +void LLProcess::launch(const LLSDOrParams& params) { // flush all buffers before the child inherits them ::fflush(NULL); @@ -359,6 +370,7 @@ void LLProcess::launch(const LLSDParamAdapter& params) // parent process mProcessID = child; + mProcessHandle = child; mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcessID << ')'); LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcessID << ")" << LL_ENDL; diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 51c42582ea..8a842589ec 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -35,7 +35,7 @@ #if LL_WINDOWS #define WIN32_LEAN_AND_MEAN -#include +#include // HANDLE (eye roll) #endif class LLProcess; @@ -79,6 +79,7 @@ public: /// implicitly kill process on destruction of LLProcess object Optional autokill; }; + typedef LLSDParamAdapter LLSDOrParams; /** * Factory accepting either plain LLSD::Map or Params block. @@ -91,7 +92,7 @@ public: * cwd (optional, string, dft no chdir): change to this directory before executing * autokill (optional, bool, dft true): implicit kill() on ~LLProcess */ - static LLProcessPtr create(const LLSDParamAdapter& params); + static LLProcessPtr create(const LLSDOrParams& params); virtual ~LLProcess(); // isRunning isn't const because, if child isn't running, it clears stored @@ -103,35 +104,46 @@ public: bool kill(void); #if LL_WINDOWS - typedef HANDLE id; + typedef int id; ///< as returned by getProcessID() + typedef HANDLE handle; ///< as returned by getProcessHandle() #else - typedef pid_t id; + typedef pid_t id; + typedef pid_t handle; #endif - /// Get platform-specific process ID - id getProcessID() const { return mProcessID; }; + /** + * Get an int-like id value. This is primarily intended for a human reader + * to differentiate processes. + */ + id getProcessID() const; + /** + * Get a "handle" of a kind that you might pass to platform-specific API + * functions to engage features not directly supported by LLProcess. + */ + handle getProcessHandle() const; /** - * Test if a process (id obtained from getProcessID()) is still - * running. Return is same nonzero id value if still running, else + * Test if a process (@c handle obtained from getProcessHandle()) is still + * running. Return same nonzero @c handle value if still running, else * zero, so you can test it like a bool. But if you want to update a * stored variable as a side effect, you can write code like this: * @code - * childpid = LLProcess::isRunning(childpid); + * hchild = LLProcess::isRunning(hchild); * @endcode * @note This method is intended as a unit-test hook, not as the first of - * a whole set of operations supported on freestanding @c id values. New - * functionality should be added as nonstatic members operating on - * mProcessID. + * a whole set of operations supported on freestanding @c handle values. + * New functionality should be added as nonstatic members operating on + * the same data as getProcessHandle(). */ - static id isRunning(id, const std::string& desc=""); + static handle isRunning(handle, const std::string& desc=""); private: /// constructor is private: use create() instead - LLProcess(const LLSDParamAdapter& params); - void launch(const LLSDParamAdapter& params); + LLProcess(const LLSDOrParams& params); + void launch(const LLSDOrParams& params); std::string mDesc; id mProcessID; + handle mProcessHandle; bool mAutokill; }; diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 405540e436..4ad45bdf27 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -599,7 +599,7 @@ namespace tut { set_test_name("implicit kill()"); NamedTempFile out("out", "not started"); - LLProcess::id pid(0); + LLProcess::handle phandle(0); { PythonProcessLauncher py("kill()", "from __future__ import with_statement\n" @@ -614,8 +614,8 @@ namespace tut py.mParams.args.add(out.getName()); py.mPy = LLProcess::create(py.mParams); ensure("couldn't launch kill() script", py.mPy); - // Capture id for later - pid = py.mPy->getProcessID(); + // 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) @@ -630,7 +630,7 @@ namespace tut // Destroy the LLProcess, which should kill the child. } // wait for the script to terminate... one way or another. - while (LLProcess::isRunning(pid)) + while (LLProcess::isRunning(phandle)) { sleep(1); } @@ -646,7 +646,7 @@ namespace tut set_test_name("autokill"); NamedTempFile from("from", "not started"); NamedTempFile to("to", ""); - LLProcess::id pid(0); + LLProcess::handle phandle(0); { PythonProcessLauncher py("autokill", "from __future__ import with_statement\n" @@ -672,8 +672,8 @@ namespace tut py.mParams.autokill = false; py.mPy = LLProcess::create(py.mParams); ensure("couldn't launch kill() script", py.mPy); - // Capture id for later - pid = py.mPy->getProcessID(); + // 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) @@ -695,7 +695,7 @@ namespace tut outf << "go"; } // flush and close. // now wait for the script to terminate... one way or another. - while (LLProcess::isRunning(pid)) + while (LLProcess::isRunning(phandle)) { sleep(1); } -- cgit v1.3 From aafb03b29f5166e8978931ad8b717be32d942836 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 7 Feb 2012 10:53:23 -0500 Subject: Convert LLProcess implementation from platform-specific to using APR. Include logic to engage Linden apr_procattr_autokill_set() extension: on Windows, magic CreateProcess() flag must be pushed down into apr_proc_create() level. When using an APR package without that extension, present implementation should lock (e.g.) SLVoice.exe lifespan to viewer's on Windows XP but probably won't on Windows 7: need magic flag on CreateProcess(). Using APR child-termination callback requires us to define state (e.g. LLProcess::RUNNING). Take the opportunity to present Status, capturing state and (if terminated) rc or signal number; but since most of the time all caller really wants is to log the outcome, also present status string, encapsulating logic to examine state and describe exited-with-rc vs. killed-by-signal. New Status logic may report clearer results in the case of a Windows child process killed by exception. Clarify that static LLProcess::isRunning(handle) overload is only for use when the original LLProcess object has been destroyed: really only for unit tests. We necessarily retain our original platform-specific implementations for just that one method. (Nonstatic isRunning() no longer calls static method.) Clarify log output from llprocess_test.cpp in a couple places. --- indra/llcommon/llprocess.cpp | 552 +++++++++++++++++++++++--------- indra/llcommon/llprocess.h | 64 +++- indra/llcommon/tests/llprocess_test.cpp | 6 +- 3 files changed, 455 insertions(+), 167 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 8611d67f25..bc27002701 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -30,11 +30,15 @@ #include "llsingleton.h" #include "llstring.h" #include "stringize.h" +#include "llapr.h" #include #include #include +static std::string empty; +static LLProcess::Status interpret_status(int status); + /// Need an exception to avoid constructing an invalid LLProcess object, but /// internal use only struct LLProcessError: public std::runtime_error @@ -55,9 +59,14 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params) } } +/// Call an apr function returning apr_status_t. On failure, log warning and +/// throw LLProcessError mentioning the function call that produced that +/// result. +#define chkapr(func) \ + if (ll_apr_warn_status(func)) \ + throw LLProcessError(#func " failed") + LLProcess::LLProcess(const LLSDOrParams& params): - mProcessID(0), - mProcessHandle(0), mAutokill(params.autokill) { if (! params.validateBlock(true)) @@ -66,31 +75,298 @@ LLProcess::LLProcess(const LLSDOrParams& params): << LLSDNotationStreamer(params))); } - launch(params); + apr_procattr_t *procattr = NULL; + chkapr(apr_procattr_create(&procattr, gAPRPoolp)); + + // For which of stdin, stdout, stderr should we create a pipe to the + // child? In the viewer, there are only a couple viable + // apr_procattr_io_set() alternatives: inherit the viewer's own stdxxx + // handle (APR_NO_PIPE, e.g. for stdout, stderr), or create a pipe that's + // blocking on the child end but nonblocking at the viewer end + // (APR_CHILD_BLOCK). The viewer can't block for anything: the parent end + // MUST be nonblocking. As the APR documentation itself points out, it + // makes very little sense to set nonblocking I/O for the child end of a + // pipe: only a specially-written child could deal with that. + // Other major options could include explicitly creating a single APR pipe + // and passing it as both stdout and stderr (apr_procattr_child_out_set(), + // apr_procattr_child_err_set()), or accepting a filename, opening it and + // passing that apr_file_t (simple <, >, 2> redirect emulation). +// chkapr(apr_procattr_io_set(procattr, APR_CHILD_BLOCK, APR_CHILD_BLOCK, APR_CHILD_BLOCK)); + chkapr(apr_procattr_io_set(procattr, APR_NO_PIPE, APR_NO_PIPE, APR_NO_PIPE)); + + // Thumbs down on implicitly invoking the shell to invoke the child. From + // our point of view, the other major alternative to APR_PROGRAM_PATH + // would be APR_PROGRAM_ENV: still copy environment, but require full + // executable pathname. I don't see a downside to searching the PATH, + // though: if our caller wants (e.g.) a specific Python interpreter, s/he + // can still pass the full pathname. + chkapr(apr_procattr_cmdtype_set(procattr, APR_PROGRAM_PATH)); + // YES, do extra work if necessary to report child exec() failures back to + // parent process. + chkapr(apr_procattr_error_check_set(procattr, 1)); + // Do not start a non-autokill child in detached state. On Posix + // platforms, this setting attempts to daemonize the new child, closing + // 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)); + + if (params.autokill) + { +#if defined(APR_HAS_PROCATTR_AUTOKILL_SET) + apr_status_t ok = apr_procattr_autokill_set(procattr, 1); +# if LL_WINDOWS + // As of 2012-02-02, we only expect this to be implemented on Windows. + // Avoid spamming the log with warnings we fully expect. + ll_apr_warn_status(ok); +# endif // LL_WINDOWS +#else + LL_WARNS("LLProcess") << "This version of APR lacks Linden apr_procattr_autokill_set() extension" << LL_ENDL; +#endif + } + + // Have to instantiate named std::strings for string params items so their + // c_str() values persist. + std::string cwd(params.cwd); + if (! cwd.empty()) + { + chkapr(apr_procattr_dir_set(procattr, cwd.c_str())); + } + + // create an argv vector for the child process + std::vector argv; + + // add the executable path + std::string executable(params.executable); + argv.push_back(executable.c_str()); + + // and any arguments + std::vector args(params.args.begin(), params.args.end()); + BOOST_FOREACH(const std::string& arg, args) + { + argv.push_back(arg.c_str()); + } + + // terminate with a null pointer + argv.push_back(NULL); + + // Launch! The NULL would be the environment block, if we were passing one. + chkapr(apr_proc_create(&mProcess, argv[0], &argv[0], NULL, procattr, gAPRPoolp)); + + // arrange to call status_callback() + apr_proc_other_child_register(&mProcess, &LLProcess::status_callback, this, mProcess.in, + gAPRPoolp); + mStatus.mState = RUNNING; + + mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcess.pid << ')'); + LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcess.pid << ")" << LL_ENDL; + + // Unless caller explicitly turned off autokill (child should persist), + // 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) + { + // Tie the lifespan of this child process to the lifespan of our APR + // pool: on destruction of the pool, forcibly kill the process. Tell + // APR to try SIGTERM and wait 3 seconds. If that didn't work, use + // SIGKILL. + apr_pool_note_subprocess(gAPRPoolp, &mProcess, APR_KILL_AFTER_TIMEOUT); + + // On Windows, associate the new child process with our Job Object. + autokill(); + } } LLProcess::~LLProcess() { + // Only in state RUNNING are we registered for callback. In UNSTARTED we + // haven't yet registered. And since receiving the callback is the only + // way we detect child termination, we only change from state RUNNING at + // the same time we unregister. + if (mStatus.mState == RUNNING) + { + // We're still registered for a callback: unregister. Do it before + // we even issue the kill(): even if kill() somehow prompted an + // instantaneous callback (unlikely), this object is going away! Any + // information updated in this object by such a callback is no longer + // available to any consumer anyway. + apr_proc_other_child_unregister(this); + } + if (mAutokill) { - kill(); + kill("destructor"); + } +} + +bool LLProcess::kill(const std::string& who) +{ + if (isRunning()) + { + LL_INFOS("LLProcess") << who << " killing " << mDesc << LL_ENDL; + +#if LL_WINDOWS + int sig = -1; +#else // Posix + int sig = SIGTERM; +#endif + + ll_apr_warn_status(apr_proc_kill(&mProcess, sig)); } + + return ! isRunning(); } bool LLProcess::isRunning(void) { - mProcessHandle = isRunning(mProcessHandle, mDesc); - return (mProcessHandle != 0); + return getStatus().mState == RUNNING; +} + +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; +} + +std::string LLProcess::getStatusString() +{ + return getStatusString(getStatus()); +} + +std::string LLProcess::getStatusString(const Status& status) +{ + return getStatusString(mDesc, status); +} + +//static +std::string LLProcess::getStatusString(const std::string& desc, const Status& status) +{ + if (status.mState == UNSTARTED) + return desc + " was never launched"; + + if (status.mState == RUNNING) + return desc + " running"; + + if (status.mState == EXITED) + return STRINGIZE(desc << " exited with code " << status.mData); + + if (status.mState == KILLED) +#if LL_WINDOWS + return STRINGIZE(desc << " killed with exception " << std::hex << status.mData); +#else + return STRINGIZE(desc << " killed by signal " << status.mData); +#endif + + + return STRINGIZE(desc << " in unknown state " << status.mState << " (" << status.mData << ")"); +} + +// Classic-C-style APR callback +void LLProcess::status_callback(int reason, void* data, int status) +{ + // Our only role is to bounce this static method call back into object + // space. + static_cast(data)->handle_status(reason, status); +} + +#define tabent(symbol) { symbol, #symbol } +static struct ReasonCode +{ + int code; + const char* name; +} reasons[] = +{ + tabent(APR_OC_REASON_DEATH), + tabent(APR_OC_REASON_UNWRITABLE), + tabent(APR_OC_REASON_RESTART), + tabent(APR_OC_REASON_UNREGISTER), + tabent(APR_OC_REASON_LOST), + tabent(APR_OC_REASON_RUNNING) +}; +#undef tabent + +// Object-oriented callback +void LLProcess::handle_status(int reason, int status) +{ + { + // This odd appearance of LL_DEBUGS is just to bracket a lookup that will + // only be performed if in fact we're going to produce the log message. + LL_DEBUGS("LLProcess") << empty; + std::string reason_str; + BOOST_FOREACH(const ReasonCode& rcp, reasons) + { + if (reason == rcp.code) + { + reason_str = rcp.name; + break; + } + } + if (reason_str.empty()) + { + reason_str = STRINGIZE("unknown reason " << reason); + } + LL_CONT << mDesc << ": handle_status(" << reason_str << ", " << status << ")" << LL_ENDL; + } + + if (! (reason == APR_OC_REASON_DEATH || reason == APR_OC_REASON_LOST)) + { + // We're only interested in the call when the child terminates. + return; + } + + // Somewhat oddly, APR requires that you explicitly unregister even when + // 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); + // 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 + // KILLED; refine below. + mStatus.mState = EXITED; + +// wi->rv = apr_proc_wait(wi->child, &wi->rc, &wi->why, APR_NOWAIT); + // It's just wrong to call apr_proc_wait() here. The only way APR knows to + // call us with APR_OC_REASON_DEATH is that it's already reaped this child + // process, so calling wait() will only produce "huh?" from the OS. We + // must rely on the status param passed in, which unfortunately comes + // straight from the OS wait() call, which means we have to decode it by + // hand. + mStatus = interpret_status(status); + LL_INFOS("LLProcess") << getStatusString() << LL_ENDL; } LLProcess::id LLProcess::getProcessID() const { - return mProcessID; + return mProcess.pid; } LLProcess::handle LLProcess::getProcessHandle() const { - return mProcessHandle; +#if LL_WINDOWS + return mProcess.hproc; +#else + return mProcess.pid; +#endif } std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params) @@ -178,77 +454,15 @@ private: LLProcess::handle mJob; }; -void LLProcess::launch(const LLSDOrParams& params) +void LLProcess::autokill() { - PROCESS_INFORMATION pinfo; - STARTUPINFOA sinfo = { sizeof(sinfo) }; - - // LLProcess::create()'s caller passes a Unix-style array of strings for - // command-line arguments. Our caller can and should expect that these will be - // passed to the child process as individual arguments, regardless of content - // (e.g. embedded spaces). But because Windows invokes any child process with - // a single command-line string, this means we must quote each argument behind - // the scenes. - std::string args = LLStringUtil::quote(params.executable); - BOOST_FOREACH(const std::string& arg, params.args) - { - args += " "; - args += LLStringUtil::quote(arg); - } - - // So retarded. Windows requires that the second parameter to - // CreateProcessA be a writable (non-const) string... - std::vector args2(args.begin(), args.end()); - args2.push_back('\0'); - - // Convert wrapper to a real std::string so we can use c_str(); but use a - // named variable instead of a temporary so c_str() pointer remains valid. - std::string cwd(params.cwd); - const char * working_directory = 0; - if (! cwd.empty()) - working_directory = cwd.c_str(); - - // It's important to pass CREATE_BREAKAWAY_FROM_JOB because Windows 7 et - // al. tend to implicitly launch new processes already bound to a job. From - // http://msdn.microsoft.com/en-us/library/windows/desktop/ms681949%28v=vs.85%29.aspx : - // "The process must not already be assigned to a job; if it is, the - // function fails with ERROR_ACCESS_DENIED." ... - // "If the process is being monitored by the Program Compatibility - // Assistant (PCA), it is placed into a compatibility job. Therefore, the - // process must be created using CREATE_BREAKAWAY_FROM_JOB before it can - // be placed in another job." - if( ! CreateProcessA(NULL, // lpApplicationName - &args2[0], // lpCommandLine - NULL, // lpProcessAttributes - NULL, // lpThreadAttributes - FALSE, // bInheritHandles - CREATE_BREAKAWAY_FROM_JOB, // dwCreationFlags - NULL, // lpEnvironment - working_directory, // lpCurrentDirectory - &sinfo, // lpStartupInfo - &pinfo ) ) // lpProcessInformation - { - throw LLProcessError(WindowsErrorString("CreateProcessA")); - } - - // CloseHandle(pinfo.hProcess); // stops leaks - nothing else - mProcessID = pinfo.dwProcessId; - mProcessHandle = pinfo.hProcess; - CloseHandle(pinfo.hThread); // stops leaks - nothing else - - mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcessID << ')'); - LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcessID << ")" << LL_ENDL; - - // Now associate the new child process with our Job Object -- unless - // autokill is false, i.e. caller asserts the child should persist. - if (params.autokill) - { - LLJob::instance().assignProcess(mDesc, mProcessHandle); -} + LLJob::instance().assignProcess(mDesc, mProcess.hproc); } LLProcess::handle LLProcess::isRunning(handle h, const std::string& desc) { + // This direct Windows implementation is because we have no access to the + // apr_proc_t struct: we expect it's been destroyed. if (! h) return 0; @@ -258,22 +472,44 @@ LLProcess::handle LLProcess::isRunning(handle h, const std::string& desc) // the process has completed. if (! desc.empty()) { - LL_INFOS("LLProcess") << desc << " terminated" << LL_ENDL; + DWORD status = 0; + if (! GetExitCodeProcess(h, &status)) + { + LL_WARNS("LLProcess") << desc << " terminated, but " + << WindowsErrorString("GetExitCodeProcess()") << LL_ENDL; + } + { + LL_INFOS("LLProcess") << getStatusString(desc, interpret_status(status)) + << LL_ENDL; + } } + CloseHandle(h); return 0; } return h; } -bool LLProcess::kill(void) +static LLProcess::Status interpret_status(int status) { - if (! mProcessHandle) - return false; + LLProcess::Status result; + + // This bit of code is cribbed from apr/threadproc/win32/proc.c, a + // function (unfortunately static) called why_from_exit_code(): + /* See WinNT.h STATUS_ACCESS_VIOLATION and family for how + * this class of failures was determined + */ + if ((status & 0xFFFF0000) == 0xC0000000) + { + result.mState = KILLED; + } + else + { + result.mState = EXITED; + } + result.mData = status; - LL_INFOS("LLProcess") << "killing " << mDesc << LL_ENDL; - TerminateProcess(mProcessHandle, 0); - return ! isRunning(); + return result; } /// GetLastError()/FormatMessage() boilerplate @@ -315,98 +551,91 @@ static std::string WindowsErrorString(const std::string& operation) #include #include +void LLProcess::autokill() +{ + // What we ought to do here is to: + // 1. create a unique process group and run all autokill children in that + // group (see https://jira.secondlife.com/browse/SWAT-563); + // 2. figure out a way to intercept control when the viewer exits -- + // gracefully or not; + // 3. when the viewer exits, kill off the aforementioned process group. + + // It's point 2 that's troublesome. Although I've seen some signal- + // handling logic in the Posix viewer code, I haven't yet found any bit of + // code that's run no matter how the viewer exits (a try/finally for the + // whole process, as it were). +} + // Attempt to reap a process ID -- returns true if the process has exited and been reaped, false otherwise. -static bool reap_pid(pid_t pid) +static bool reap_pid(pid_t pid, LLProcess::Status* pstatus=NULL) { - pid_t wait_result = ::waitpid(pid, NULL, WNOHANG); + LLProcess::Status dummy; + if (! pstatus) + { + // If caller doesn't want to see Status, give us a target anyway so we + // don't have to have a bunch of conditionals. + pstatus = &dummy; + } + + int status = 0; + pid_t wait_result = ::waitpid(pid, &status, WNOHANG); if (wait_result == pid) { + *pstatus = interpret_status(status); return true; } - if (wait_result == -1 && errno == ECHILD) + if (wait_result == 0) { - // No such process -- this may mean we're ignoring SIGCHILD. - return true; + pstatus->mState = LLProcess::RUNNING; + pstatus->mData = 0; + return false; } - - return false; -} -void LLProcess::launch(const LLSDOrParams& params) -{ - // flush all buffers before the child inherits them - ::fflush(NULL); + // Clear caller's Status block; caller must interpret UNSTARTED to mean + // "if this PID was ever valid, it no longer is." + *pstatus = LLProcess::Status(); - pid_t child = vfork(); - if (child == 0) + // We've dealt with the success cases: we were able to reap the child + // (wait_result == pid) or it's still running (wait_result == 0). It may + // be that the child terminated but didn't hang around long enough for us + // to reap. In that case we still have no Status to report, but we can at + // least state that it's not running. + if (wait_result == -1 && errno == ECHILD) { - // child process - - std::string cwd(params.cwd); - if (! cwd.empty()) - { - // change to the desired child working directory - if (::chdir(cwd.c_str())) - { - // chdir failed - LL_WARNS("LLProcess") << "could not chdir(\"" << cwd << "\")" << LL_ENDL; - // pointless to throw; this is child process... - _exit(248); - } - } - - // create an argv vector for the child process - std::vector fake_argv; - - // add the executable path - std::string executable(params.executable); - fake_argv.push_back(executable.c_str()); - - // and any arguments - std::vector args(params.args.begin(), params.args.end()); - BOOST_FOREACH(const std::string& arg, args) - { - fake_argv.push_back(arg.c_str()); - } - - // terminate with a null pointer - fake_argv.push_back(NULL); - - ::execv(executable.c_str(), const_cast(&fake_argv[0])); - - // If we reach this point, the exec failed. - LL_WARNS("LLProcess") << "failed to launch: "; - BOOST_FOREACH(const char* arg, fake_argv) - { - LL_CONT << arg << ' '; - } - LL_CONT << LL_ENDL; - // Use _exit() instead of exit() per the vfork man page. Exit with a - // distinctive rc: someday soon we'll be able to retrieve it, and it - // would be nice to be able to tell that the child process failed! - _exit(249); + // No such process -- this may mean we're ignoring SIGCHILD. + return true; } - // parent process - mProcessID = child; - mProcessHandle = child; - - mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcessID << ')'); - LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcessID << ")" << LL_ENDL; + // Uh, should never happen?! + LL_WARNS("LLProcess") << "LLProcess::reap_pid(): waitpid(" << pid << ") returned " + << wait_result << "; not meaningful?" << LL_ENDL; + // If caller is looping until this pid terminates, and if we can't find + // out, better to break the loop than to claim it's still running. + return true; } LLProcess::id LLProcess::isRunning(id pid, const std::string& desc) { + // This direct Posix implementation is because we have no access to the + // apr_proc_t struct: we expect it's been destroyed. if (! pid) return 0; // Check whether the process has exited, and reap it if it has. - if(reap_pid(pid)) + LLProcess::Status status; + if(reap_pid(pid, &status)) { // the process has exited. if (! desc.empty()) { - LL_INFOS("LLProcess") << desc << " terminated" << LL_ENDL; + std::string statstr(desc + " apparently terminated: no status available"); + // We don't just pass UNSTARTED to getStatusString() because, in + // the context of reap_pid(), that state has special meaning. + if (status.mState != UNSTARTED) + { + statstr = getStatusString(desc, status); + } + LL_INFOS("LLProcess") << statstr << LL_ENDL; } return 0; } @@ -414,18 +643,27 @@ LLProcess::id LLProcess::isRunning(id pid, const std::string& desc) return pid; } -bool LLProcess::kill(void) +static LLProcess::Status interpret_status(int status) { - if (! mProcessID) - return false; + LLProcess::Status result; - // Try to kill the process. We'll do approximately the same thing whether - // the kill returns an error or not, so we ignore the result. - LL_INFOS("LLProcess") << "killing " << mDesc << LL_ENDL; - (void)::kill(mProcessID, SIGTERM); + if (WIFEXITED(status)) + { + result.mState = LLProcess::EXITED; + result.mData = WEXITSTATUS(status); + } + else if (WIFSIGNALED(status)) + { + result.mState = LLProcess::KILLED; + result.mData = WTERMSIG(status); + } + else // uh, shouldn't happen? + { + result.mState = LLProcess::EXITED; + result.mData = status; // someone else will have to decode + } - // This will have the side-effect of reaping the zombie if the process has exited. - return ! isRunning(); + return result; } /*==========================================================================*| diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 8a842589ec..689f8aedab 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -29,6 +29,7 @@ #include "llinitparam.h" #include "llsdparam.h" +#include "apr_thread_proc.h" #include #include #include // std::ostream @@ -95,13 +96,52 @@ public: static LLProcessPtr create(const LLSDOrParams& params); virtual ~LLProcess(); - // isRunning isn't const because, if child isn't running, it clears stored - // process ID + // isRunning() isn't const because, when child terminates, it sets stored + // Status bool isRunning(void); - + + /** + * State of child process + */ + enum state + { + UNSTARTED, ///< initial value, invisible to consumer + RUNNING, ///< child process launched + EXITED, ///< child process terminated voluntarily + KILLED ///< child process terminated involuntarily + }; + + /** + * Status info + */ + struct Status + { + Status(): + mState(UNSTARTED), + mData(0) + {} + + state mState; ///< @see state + /** + * - for mState == EXITED: mData is exit() code + * - for mState == KILLED: mData is signal number (Posix) + * - otherwise: mData is undefined + */ + int mData; + }; + + /// Status query + Status getStatus(); + /// English Status string query, for logging etc. + std::string getStatusString(); + /// English Status string query for previously-captured Status + std::string getStatusString(const Status& status); + /// static English Status string query + static std::string getStatusString(const std::string& desc, const Status& status); + // Attempt to kill the process -- returns true if the process is no longer running when it returns. // Note that even if this returns false, the process may exit some time after it's called. - bool kill(void); + bool kill(const std::string& who=""); #if LL_WINDOWS typedef int id; ///< as returned by getProcessID() @@ -133,18 +173,28 @@ public: * a whole set of operations supported on freestanding @c handle values. * New functionality should be added as nonstatic members operating on * the same data as getProcessHandle(). + * + * In particular, if child termination is detected by static isRunning() + * rather than by nonstatic isRunning(), the LLProcess object won't be + * aware of the child's changed status and may encounter OS errors trying + * to obtain it. static isRunning() is only intended for after the + * launching LLProcess object has been destroyed. */ static handle isRunning(handle, const std::string& desc=""); private: /// constructor is private: use create() instead LLProcess(const LLSDOrParams& params); - void launch(const LLSDOrParams& params); + void autokill(); + // Classic-C-style APR callback + static void status_callback(int reason, void* data, int status); + // Object-oriented callback + void handle_status(int reason, int status); std::string mDesc; - id mProcessID; - handle mProcessHandle; + apr_proc_t mProcess; bool mAutokill; + Status mStatus; }; /// for logging diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 4ad45bdf27..60ed12ad6a 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -630,7 +630,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)) + while (LLProcess::isRunning(phandle, "kill() script")) { sleep(1); } @@ -643,7 +643,7 @@ namespace tut template<> template<> void object::test<6>() { - set_test_name("autokill"); + set_test_name("autokill=false"); NamedTempFile from("from", "not started"); NamedTempFile to("to", ""); LLProcess::handle phandle(0); @@ -695,7 +695,7 @@ namespace tut outf << "go"; } // flush and close. // now wait for the script to terminate... one way or another. - while (LLProcess::isRunning(phandle)) + while (LLProcess::isRunning(phandle, "autokill script")) { sleep(1); } -- cgit v1.3 From 32e11494ffde368b2ac166e16dc294d66a18492f Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 7 Feb 2012 12:28:57 -0500 Subject: Use os.path.normcase(os.path.normpath()) when comparing directories. Once again we've been bitten by comparison failure between "c:\somepath" and "C:\somepath". Normalize paths in both Python helper scripts to make that comparison more robust. --- indra/llcommon/tests/llprocess_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 60ed12ad6a..6d2292fb88 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -186,7 +186,7 @@ public: "from __future__ import with_statement\n" "import os.path, sys, tempfile\n" "with open(sys.argv[1], 'w') as f:\n" - " f.write(os.path.realpath(tempfile.mkdtemp()))\n")) + " f.write(os.path.normcase(os.path.normpath(os.path.realpath(tempfile.mkdtemp()))))\n")) {} ~NamedTempDir() @@ -513,7 +513,7 @@ namespace tut "from __future__ import with_statement\n" "import os, sys\n" "with open(sys.argv[1], 'w') as f:\n" - " f.write(os.getcwd())\n"); + " f.write(os.path.normcase(os.path.normpath(os.getcwd())))\n"); // Before running, call setWorkingDirectory() py.mParams.cwd = tempdir.getName(); ensure_equals("os.getcwd()", py.run_read(), tempdir.getName()); -- cgit v1.3 From d4f887e43ccf0a8b7a84ebbfe6889462a1d9c25f Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 13 Feb 2012 16:18:46 -0500 Subject: Add unit tests for LLProcess::Status functionality. --- indra/llcommon/tests/llprocess_test.cpp | 46 +++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 6d2292fb88..711715e373 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -555,6 +555,41 @@ namespace tut template<> template<> void object::test<4>() + { + set_test_name("exit(0)"); + PythonProcessLauncher py("exit(0)", + "import sys\n" + "sys.exit(0)\n"); + py.run(); + ensure_equals("Status.mState", py.mPy->getStatus().mState, LLProcess::EXITED); + ensure_equals("Status.mData", py.mPy->getStatus().mData, 0); + } + + template<> template<> + void object::test<5>() + { + set_test_name("exit(2)"); + PythonProcessLauncher py("exit(2)", + "import sys\n" + "sys.exit(2)\n"); + py.run(); + ensure_equals("Status.mState", py.mPy->getStatus().mState, LLProcess::EXITED); + ensure_equals("Status.mData", py.mPy->getStatus().mData, 2); + } + + template<> template<> + void object::test<6>() + { + set_test_name("syntax_error:"); + PythonProcessLauncher py("syntax_error:", + "syntax_error:\n"); + py.run(); + ensure_equals("Status.mState", py.mPy->getStatus().mState, LLProcess::EXITED); + ensure_equals("Status.mData", py.mPy->getStatus().mData, 1); + } + + template<> template<> + void object::test<7>() { set_test_name("explicit kill()"); PythonProcessLauncher py("kill()", @@ -588,6 +623,13 @@ namespace tut { sleep(1); } +#if LL_WINDOWS + ensure_equals("Status.mState", py.mPy->getStatus().mState, LLProcess::EXITED); + ensure_equals("Status.mData", py.mPy->getStatus().mData, -1); +#else + ensure_equals("Status.mState", py.mPy->getStatus().mState, LLProcess::KILLED); + ensure_equals("Status.mData", py.mPy->getStatus().mData, SIGTERM); +#endif // 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. @@ -595,7 +637,7 @@ namespace tut } template<> template<> - void object::test<5>() + void object::test<8>() { set_test_name("implicit kill()"); NamedTempFile out("out", "not started"); @@ -641,7 +683,7 @@ namespace tut } template<> template<> - void object::test<6>() + void object::test<9>() { set_test_name("autokill=false"); NamedTempFile from("from", "not started"); -- cgit v1.3 From aae61392be822218cabcab91d95eb1e75d471764 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 13 Feb 2012 17:38:25 -0500 Subject: 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. --- indra/llcommon/llprocess.cpp | 96 ++++++++++++++++++++++++++------- indra/llcommon/llprocess.h | 20 ++++--- indra/llcommon/tests/llprocess_test.cpp | 42 +++++++++------ 3 files changed, 114 insertions(+), 44 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') 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 +#include #include #include @@ -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 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 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"); -- cgit v1.3 From e239cad1f509e3d96011acb61614f2481c46af38 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 15 Feb 2012 10:07:09 -0500 Subject: Preliminary pipe support for LLProcess. Add LLProcess::FileParam to specify how to construct each child's standard file slot, with lots of comments about features designed but not yet implemented. The point is to design it with enough flexibility to be able to extend to foreseeable use cases. Add LLProcess::Params::files to collect up to 3 FileParam items. Naturally this extends the accepted LLSD syntax as well. Implement type="" (child inherits parent file descriptor) and "pipe" (parent constructs anonymous pipe to pass to child). Add LLProcess::FILESLOT enum, plus methods: getReadPipe(FILESLOT), getOptReadPipe(FILESLOT) getWritePipe(), getOptWritePipe() getPipeName(FILESLOT): placeholder implementation for now Add LLProcess::ReadPipe and WritePipe classes, as returned by get*Pipe(). WritePipe supports get_ostream() method for streaming to child stdin. ReadPipe supports get_istream() method for reading from child stdout/stderr. It also provides getPump() returning LLEventPump& so interested parties can listen for arrival of new data on the aforementioned std::istream. For "pipe" slots, instantiate appropriate *Pipe class. ReadPipe and WritePipe classes are pure virtual bases for ReadPipeImpl and WritePipeImpl, respectively: all implementation data are hidden in the latter classes, visible only in llprocess.cpp. In fact each *PipeImpl class registers itself for "mainloop" ticks, attempting nonblocking I/O to the underlying apr_file_t on each tick. Data are buffered in a boost::asio::streambuf, which bridges between std::[io]stream and the APR I/O calls. Sanity-test ReadPipeImpl by using a pipe to absorb the Python "SyntaxError" output from the successful syntax_error test, rather than alarming the user. Add first few unit tests for validating FileParam. More tests coming! --- indra/llcommon/llprocess.cpp | 321 ++++++++++++++++++++++++++++++-- indra/llcommon/llprocess.h | 235 ++++++++++++++++++++++- indra/llcommon/tests/llprocess_test.cpp | 171 ++++++++++++++++- 3 files changed, 695 insertions(+), 32 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index b13e8eb8e0..55eb7e69d3 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -26,6 +26,7 @@ #include "linden_common.h" #include "llprocess.h" +#include "llsdutil.h" #include "llsdserialize.h" #include "llsingleton.h" #include "llstring.h" @@ -36,19 +37,20 @@ #include #include +#include +#include #include #include +#include +#include +#include +#include +#include +static const char* whichfile[] = { "stdin", "stdout", "stderr" }; static std::string empty; static LLProcess::Status interpret_status(int status); -/// Need an exception to avoid constructing an invalid LLProcess object, but -/// internal use only -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 @@ -117,6 +119,154 @@ private: }; static LLProcessListener sProcessListener; +LLProcess::BasePipe::~BasePipe() {} + +class WritePipeImpl: public LLProcess::WritePipe +{ +public: + WritePipeImpl(const std::string& desc, apr_file_t* pipe): + mDesc(desc), + mPipe(pipe), + // Essential to initialize our std::ostream with our special streambuf! + mStream(&mStreambuf) + { + mConnection = LLEventPumps::instance().obtain("mainloop") + .listen(LLEventPump::inventName("WritePipe"), + boost::bind(&WritePipeImpl::tick, this, _1)); + } + + virtual std::ostream& get_ostream() { return mStream; } + + bool tick(const LLSD&) + { + // If there's anything to send, try to send it. + if (mStreambuf.size()) + { + // Copy data out from mStreambuf to a flat, contiguous buffer to + // write -- but only up to a certain size. + std::streamsize total(mStreambuf.size()); + std::streamsize bufsize((std::min)(4096, total)); + boost::asio::streambuf::const_buffers_type bufs = mStreambuf.data(); + std::vector buffer( + boost::asio::buffers_begin(bufs), + boost::asio::buffers_begin(bufs) + bufsize); + apr_size_t written(bufsize); + ll_apr_warn_status(apr_file_write(mPipe, &buffer[0], &written)); + // 'written' is modified to reflect the number of bytes actually + // written. Since they've been sent, remove them from the + // streambuf so we don't keep trying to send them. This could be + // anywhere from 0 up to mStreambuf.size(); anything we haven't + // yet sent, we'll try again next tick() call. + mStreambuf.consume(written); + LL_DEBUGS("LLProcess") << "wrote " << written << " of " << bufsize + << " bytes to " << mDesc + << " (original " << total << "), " + << mStreambuf.size() << " remaining" << LL_ENDL; + } + return false; + } + +private: + std::string mDesc; + apr_file_t* mPipe; + LLTempBoundListener mConnection; + boost::asio::streambuf mStreambuf; + std::ostream mStream; +}; + +class ReadPipeImpl: public LLProcess::ReadPipe +{ +public: + ReadPipeImpl(const std::string& desc, apr_file_t* pipe): + mDesc(desc), + mPipe(pipe), + // Essential to initialize our std::istream with our special streambuf! + mStream(&mStreambuf), + mPump("ReadPipe"), + // use funky syntax to call max() to avoid blighted max() macros + mLimit((std::numeric_limits::max)()) + { + mConnection = LLEventPumps::instance().obtain("mainloop") + .listen(LLEventPump::inventName("ReadPipe"), + boost::bind(&ReadPipeImpl::tick, this, _1)); + } + + // Much of the implementation is simply connecting the abstract virtual + // methods with implementation data concealed from the base class. + virtual std::istream& get_istream() { return mStream; } + virtual LLEventPump& getPump() { return mPump; } + virtual void setLimit(size_t limit) { mLimit = limit; } + virtual size_t getLimit() const { return mLimit; } + +private: + bool tick(const LLSD&) + { + // Allocate a buffer and try, every time, to read into it. + std::vector buffer(4096); + apr_size_t gotten(buffer.size()); + apr_status_t err = apr_file_read(mPipe, &buffer[0], &gotten); + if (err == APR_EOF) + { + // Handle EOF specially: it's part of normal-case processing. + LL_DEBUGS("LLProcess") << "EOF on " << mDesc << LL_ENDL; + // We won't need any more tick() calls. + mConnection.disconnect(); + } + else if (! ll_apr_warn_status(err)) // validate anything but EOF + { + // 'gotten' was modified to reflect the number of bytes actually + // received. If nonzero, add them to the streambuf and notify + // interested parties. + if (gotten) + { + boost::asio::streambuf::mutable_buffers_type mbufs = mStreambuf.prepare(gotten); + std::copy(buffer.begin(), buffer.begin() + gotten, + boost::asio::buffers_begin(mbufs)); + // Don't forget to "commit" the data! The sequence (prepare(), + // commit()) is obviously intended to allow us to allocate + // buffer space, then read directly into some portion of it, + // then commit only as much as we managed to obtain. But the + // only official (documented) way I can find to populate a + // mutable_buffers_type is to use buffers_begin(). It Would Be + // Nice if we were permitted to directly read into + // mutable_buffers_type (not to mention writing directly from + // const_buffers_type in WritePipeImpl; APR even supports an + // apr_file_writev() function for writing from discontiguous + // buffers) -- but as of 2012-02-14, this copying appears to + // be the safest tactic. + mStreambuf.commit(gotten); + LL_DEBUGS("LLProcess") << "read " << gotten << " of " << buffer.size() + << " bytes from " << mDesc << ", new total " + << mStreambuf.size() << LL_ENDL; + + // Now that we've received new data, publish it on our + // LLEventPump as advertised. Constrain it by mLimit. + std::streamsize datasize((std::min)(mLimit, mStreambuf.size())); + boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data(); + mPump.post(LLSDMap("data", LLSD::String( + boost::asio::buffers_begin(cbufs), + boost::asio::buffers_begin(cbufs) + datasize))); + } + } + return false; + } + + std::string mDesc; + apr_file_t* mPipe; + LLTempBoundListener mConnection; + boost::asio::streambuf mStreambuf; + std::istream mStream; + LLEventStream mPump; + size_t mLimit; +}; + +/// Need an exception to avoid constructing an invalid LLProcess object, but +/// internal use only +struct LLProcessError: public std::runtime_error +{ + LLProcessError(const std::string& msg): std::runtime_error(msg) {} +}; + LLProcessPtr LLProcess::create(const LLSDOrParams& params) { try @@ -134,12 +284,23 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params) /// throw LLProcessError mentioning the function call that produced that /// result. #define chkapr(func) \ - if (ll_apr_warn_status(func)) \ - throw LLProcessError(#func " failed") + if (ll_apr_warn_status(func)) \ + throw LLProcessError(#func " failed") LLProcess::LLProcess(const LLSDOrParams& params): - mAutokill(params.autokill) + mAutokill(params.autokill), + mPipes(NSLOTS) { + // Hmm, when you construct a ptr_vector with a size, it merely reserves + // space, it doesn't actually make it that big. Explicitly make it bigger. + // Because of ptr_vector's odd semantics, have to push_back(0) the right + // number of times! resize() wants to default-construct new BasePipe + // instances, which fails because it's pure virtual. But because of the + // constructor call, these push_back() calls should require no new + // allocation. + for (size_t i = 0; i < mPipes.capacity(); ++i) + mPipes.push_back(0); + if (! params.validateBlock(true)) { throw LLProcessError(STRINGIZE("not launched: failed parameter validation\n" @@ -154,16 +315,46 @@ LLProcess::LLProcess(const LLSDOrParams& params): // apr_procattr_io_set() alternatives: inherit the viewer's own stdxxx // handle (APR_NO_PIPE, e.g. for stdout, stderr), or create a pipe that's // blocking on the child end but nonblocking at the viewer end - // (APR_CHILD_BLOCK). The viewer can't block for anything: the parent end - // MUST be nonblocking. As the APR documentation itself points out, it - // makes very little sense to set nonblocking I/O for the child end of a - // pipe: only a specially-written child could deal with that. + // (APR_CHILD_BLOCK). // Other major options could include explicitly creating a single APR pipe // and passing it as both stdout and stderr (apr_procattr_child_out_set(), // apr_procattr_child_err_set()), or accepting a filename, opening it and // passing that apr_file_t (simple <, >, 2> redirect emulation). -// chkapr(apr_procattr_io_set(procattr, APR_CHILD_BLOCK, APR_CHILD_BLOCK, APR_CHILD_BLOCK)); - chkapr(apr_procattr_io_set(procattr, APR_NO_PIPE, APR_NO_PIPE, APR_NO_PIPE)); + std::vector fparams(params.files.begin(), params.files.end()); + // By default, pass APR_NO_PIPE for each slot. + std::vector select(LL_ARRAY_SIZE(whichfile), APR_NO_PIPE); + for (size_t i = 0; i < (std::min)(LL_ARRAY_SIZE(whichfile), fparams.size()); ++i) + { + if (std::string(fparams[i].type).empty()) // inherit our file descriptor + { + select[i] = APR_NO_PIPE; + } + else if (std::string(fparams[i].type) == "pipe") // anonymous pipe + { + if (! std::string(fparams[i].name).empty()) + { + LL_WARNS("LLProcess") << "For " << std::string(params.executable) + << ": internal names for reusing pipes ('" + << std::string(fparams[i].name) << "' for " << whichfile[i] + << ") are not yet supported -- creating distinct pipe" + << LL_ENDL; + } + // The viewer can't block for anything: the parent end MUST be + // nonblocking. As the APR documentation itself points out, it + // makes very little sense to set nonblocking I/O for the child + // end of a pipe: only a specially-written child could deal with + // that. + select[i] = APR_CHILD_BLOCK; + } + else + { + throw LLProcessError(STRINGIZE("For " << std::string(params.executable) + << ": unsupported FileParam for " << whichfile[i] + << ": type='" << std::string(fparams[i].type) + << "', name='" << std::string(fparams[i].name) << "'")); + } + } + chkapr(apr_procattr_io_set(procattr, select[STDIN], select[STDOUT], select[STDERR])); // Thumbs down on implicitly invoking the shell to invoke the child. From // our point of view, the other major alternative to APR_PROGRAM_PATH @@ -251,6 +442,27 @@ LLProcess::LLProcess(const LLSDOrParams& params): // On Windows, associate the new child process with our Job Object. autokill(); } + + // Instantiate the proper pipe I/O machinery + // want to be able to point to apr_proc_t::in, out, err by index + typedef apr_file_t* apr_proc_t::*apr_proc_file_ptr; + static apr_proc_file_ptr members[] = + { &apr_proc_t::in, &apr_proc_t::out, &apr_proc_t::err }; + for (size_t i = 0; i < NSLOTS; ++i) + { + if (select[i] != APR_CHILD_BLOCK) + continue; + if (i == STDIN) + { + mPipes.replace(i, new WritePipeImpl(whichfile[i], mProcess.*(members[i]))); + } + else + { + mPipes.replace(i, new ReadPipeImpl(whichfile[i], mProcess.*(members[i]))); + } + LL_DEBUGS("LLProcess") << "Instantiating " << typeid(mPipes[i]).name() + << "('" << whichfile[i] << "')" << LL_ENDL; + } } LLProcess::~LLProcess() @@ -428,6 +640,83 @@ LLProcess::handle LLProcess::getProcessHandle() const #endif } +std::string LLProcess::getPipeName(FILESLOT) +{ + // LLProcess::FileParam::type "npipe" is not yet implemented + return ""; +} + +template +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(&mPipes[slot]); + if (! ppipe) + { + error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a " << typeid(PIPETYPE).name()); + return NULL; + } + + error.clear(); + return ppipe; +} + +template +PIPETYPE& LLProcess::getPipe(FILESLOT slot) +{ + std::string error; + PIPETYPE* wp = getPipePtr(error, slot); + if (! wp) + { + throw NoPipe(error); + } + return *wp; +} + +template +boost::optional LLProcess::getOptPipe(FILESLOT slot) +{ + std::string error; + PIPETYPE* wp = getPipePtr(error, slot); + if (! wp) + { + LL_DEBUGS("LLProcess") << error << LL_ENDL; + return boost::optional(); + } + return *wp; +} + +LLProcess::WritePipe& LLProcess::getWritePipe(FILESLOT slot) +{ + return getPipe(slot); +} + +boost::optional LLProcess::getOptWritePipe(FILESLOT slot) +{ + return getOptPipe(slot); +} + +LLProcess::ReadPipe& LLProcess::getReadPipe(FILESLOT slot) +{ + return getPipe(slot); +} + +boost::optional LLProcess::getOptReadPipe(FILESLOT slot) +{ + return getOptPipe(slot); +} + std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params) { std::string cwd(params.cwd); diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index b95ae55701..448a88f4c0 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -31,8 +31,11 @@ #include "llsdparam.h" #include "apr_thread_proc.h" #include +#include +#include #include #include // std::ostream +#include #if LL_WINDOWS #define WIN32_LEAN_AND_MEAN @@ -43,6 +46,8 @@ #endif #endif +class LLEventPump; + class LLProcess; /// LLProcess instances are created on the heap by static factory methods and /// managed by ref-counted pointers. @@ -64,6 +69,87 @@ class LL_COMMON_API LLProcess: public boost::noncopyable { LOG_CLASS(LLProcess); public: + /** + * Specify what to pass for each of child stdin, stdout, stderr. + * @see LLProcess::Params::files. + */ + struct FileParam: public LLInitParam::Block + { + /** + * type of file handle to pass to child process + * + * - "" (default): let the child inherit the same file handle used by + * this process. For instance, if passed as stdout, child stdout + * will be interleaved with stdout from this process. In this case, + * @a name is moot and should be left "". + * + * - "file": open an OS filesystem file with the specified @a name. + * Not yet implemented. + * + * - "pipe" or "tpipe" or "npipe": depends on @a name + * + * - @a name.empty(): construct an OS pipe used only for this slot + * of the forthcoming child process. + * + * - ! @a name.empty(): in a global registry, find or create (using + * the specified @a name) an OS pipe. The point of the (purely + * internal) @a name is that passing the same @a name in more than + * one slot for a given LLProcess -- or for slots in different + * LLProcess instances -- means the same pipe. For example, you + * might pass the same @a name value as both stdout and stderr to + * make the child process produce both on the same actual pipe. Or + * you might pass the same @a name as the stdout for one LLProcess + * and the stdin for another to connect the two child processes. + * Use LLProcess::getPipeName() to generate a unique name + * guaranteed not to already exist in the registry. Not yet + * implemented. + * + * The difference between "pipe", "tpipe" and "npipe" is as follows. + * + * - "pipe": direct LLProcess to monitor the parent end of the pipe, + * pumping nonblocking I/O every frame. The expectation (at least + * for stdout or stderr) is that the caller will listen for + * incoming data and consume it as it arrives. It's important not + * to neglect such a pipe, because it's buffered in viewer memory. + * If you suspect the child may produce a great volume of output + * between viewer frames, consider directing the child to write to + * a filesystem file instead, then read the file later. + * + * - "tpipe": do not engage LLProcess machinery to monitor the + * parent end of the pipe. A "tpipe" is used only to connect + * different child processes. As such, it makes little sense to + * pass an empty @a name. Not yet implemented. + * + * - "npipe": like "tpipe", but use an OS named pipe with a + * generated name. Note that @a name is the @em internal name of + * the pipe in our global registry -- it doesn't necessarily have + * anything to do with the pipe's name in the OS filesystem. Use + * LLProcess::getPipeName() to obtain the named pipe's OS + * filesystem name, e.g. to pass it as the @a name to another + * LLProcess instance using @a type "file". This supports usage + * like bash's <(subcommand...) or >(subcommand...) + * constructs. Not yet implemented. + * + * In all cases the open mode (read, write) is determined by the child + * slot you're filling. Child stdin means select the "read" end of a + * pipe, or open a filesystem file for reading; child stdout or stderr + * means select the "write" end of a pipe, or open a filesystem file + * for writing. + * + * Confusion such as passing the same pipe as the stdin of two + * processes (rather than stdout for one and stdin for the other) is + * explicitly permitted: it's up to the caller to construct meaningful + * LLProcess pipe graphs. + */ + Optional type; + Optional name; + + FileParam(const std::string& tp="", const std::string& nm=""): + type("type", tp), + name("name", nm) + {} + }; + /// Param block definition struct Params: public LLInitParam::Block { @@ -71,7 +157,8 @@ public: executable("executable"), args("args"), cwd("cwd"), - autokill("autokill", true) + autokill("autokill", true), + files("files") {} /// pathname of executable @@ -87,19 +174,22 @@ public: Optional cwd; /// implicitly kill process on destruction of LLProcess object Optional autokill; + /** + * Up to three FileParam items: for child stdin, stdout, stderr. + * Passing two FileParam entries means default treatment for stderr, + * and so forth. + * + * @note While it's theoretically plausible to pass additional open + * file handles to a child specifically written to expect them, our + * underlying implementation library doesn't support that. + */ + Multiple files; }; typedef LLSDParamAdapter LLSDOrParams; /** * Factory accepting either plain LLSD::Map or Params block. * MAY RETURN DEFAULT-CONSTRUCTED LLProcessPtr if params invalid! - * - * Redundant with Params definition above? - * - * executable (required, string): executable pathname - * args (optional, string array): extra command-line arguments - * cwd (optional, string, dft no chdir): change to this directory before executing - * autokill (optional, bool, dft true): implicit kill() on ~LLProcess */ static LLProcessPtr create(const LLSDOrParams& params); virtual ~LLProcess(); @@ -190,6 +280,125 @@ public: */ static handle isRunning(handle, const std::string& desc=""); + /// Provide symbolic access to child's file slots + enum FILESLOT { STDIN=0, STDOUT=1, STDERR=2, NSLOTS=3 }; + + /** + * For a pipe constructed with @a type "npipe", obtain the generated OS + * filesystem name for the specified pipe. Otherwise returns the empty + * string. @see LLProcess::FileParam::type + */ + std::string getPipeName(FILESLOT); + + /// base of ReadPipe, WritePipe + class BasePipe + { + public: + virtual ~BasePipe() = 0; + }; + + /// As returned by getWritePipe() or getOptWritePipe() + class WritePipe: public BasePipe + { + public: + /** + * Get ostream& on which to write to child's stdin. + * + * @usage + * @code + * myProcess->getWritePipe().get_ostream() << "Hello, child!" << std::endl; + * @endcode + */ + virtual std::ostream& get_ostream() = 0; + }; + + /// As returned by getReadPipe() or getOptReadPipe() + class ReadPipe: public BasePipe + { + public: + /** + * Get istream& on which to read from child's stdout or stderr. + * + * @usage + * @code + * std::string stuff; + * myProcess->getReadPipe().get_istream() >> stuff; + * @endcode + * + * You should be sure in advance that the ReadPipe in question can + * fill the request. @see getPump() + */ + virtual std::istream& get_istream() = 0; + + /** + * Get LLEventPump& on which to listen for incoming data. The posted + * LLSD::Map event will contain a key "data" whose value is an + * LLSD::String containing (part of) the data accumulated in the + * buffer. + * + * If the child sends "abc", and this ReadPipe posts "data"="abc", but + * you don't consume it by reading the std::istream returned by + * get_istream(), and the child next sends "def", ReadPipe will post + * "data"="abcdef". + */ + virtual LLEventPump& getPump() = 0; + + /** + * Set maximum length of buffer data that will be posted in the LLSD + * announcing arrival of new data from the child. If you call + * setLimit(5), and the child sends "abcdef", the LLSD event will + * contain "data"="abcde". However, you may still read the entire + * "abcdef" from get_istream(): this limit affects only the size of + * the data posted with the LLSD event. If you don't call this method, + * all pending data will be posted. + */ + virtual void setLimit(size_t limit) = 0; + + /** + * Query the current setLimit() limit. + */ + virtual size_t getLimit() const = 0; + }; + + /// Exception thrown by getWritePipe(), getReadPipe() if you didn't ask to + /// create a pipe at the corresponding FILESLOT. + struct NoPipe: public std::runtime_error + { + NoPipe(const std::string& what): std::runtime_error(what) {} + }; + + /** + * Get a reference to the (only) WritePipe for this LLProcess. @a slot, if + * specified, must be STDIN. Throws NoPipe if you did not request a "pipe" + * for child stdin. Use this method when you know how you created the + * LLProcess in hand. + */ + WritePipe& getWritePipe(FILESLOT slot=STDIN); + + /** + * Get a boost::optional to the (only) WritePipe for this + * LLProcess. @a slot, if specified, must be STDIN. The return value is + * empty if you did not request a "pipe" for child stdin. Use this method + * for inspecting an LLProcess you did not create. + */ + boost::optional getOptWritePipe(FILESLOT slot=STDIN); + + /** + * Get a reference to one of the ReadPipes for this LLProcess. @a slot, if + * specified, must be STDOUT or STDERR. Throws NoPipe if you did not + * request a "pipe" for child stdout or stderr. Use this method when you + * know how you created the LLProcess in hand. + */ + ReadPipe& getReadPipe(FILESLOT slot); + + /** + * Get a boost::optional to one of the ReadPipes for this + * LLProcess. @a slot, if specified, must be STDOUT or STDERR. The return + * value is empty if you did not request a "pipe" for child stdout or + * stderr. Use this method for inspecting an LLProcess you did not create. + */ + boost::optional getOptReadPipe(FILESLOT slot); + private: /// constructor is private: use create() instead LLProcess(const LLSDOrParams& params); @@ -198,11 +407,21 @@ private: static void status_callback(int reason, void* data, int status); // Object-oriented callback void handle_status(int reason, int status); + // implementation for get[Opt][Read|Write]Pipe() + template + PIPETYPE& getPipe(FILESLOT slot); + template + boost::optional getOptPipe(FILESLOT slot); + template + PIPETYPE* getPipePtr(std::string& error, FILESLOT slot); std::string mDesc; apr_proc_t mProcess; bool mAutokill; Status mStatus; + // explicitly want this ptr_vector to be able to store NULLs + typedef boost::ptr_vector< boost::nullable > PipeVector; + PipeVector mPipes; }; /// for logging diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 8c21be196b..d4e9977e63 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -34,6 +34,7 @@ #include "stringize.h" #include "llsdutil.h" #include "llevents.h" +#include "llerrorcontrol.h" #if defined(LL_WINDOWS) #define sleep(secs) _sleep((secs) * 1000) @@ -92,12 +93,18 @@ static std::string readfile(const std::string& pathname, const std::string& desc /// Looping on LLProcess::isRunning() must now be accompanied by pumping /// "mainloop" -- otherwise the status won't update and you get an infinite /// loop. +void yield(int seconds=1) +{ + // This function simulates waiting for another viewer frame + sleep(seconds); + LLEventPumps::instance().obtain("mainloop").post(LLSD()); +} + void waitfor(LLProcess& proc) { while (proc.isRunning()) { - sleep(1); - LLEventPumps::instance().obtain("mainloop").post(LLSD()); + yield(); } } @@ -105,8 +112,7 @@ void waitfor(LLProcess::handle h, const std::string& desc) { while (LLProcess::isRunning(h, desc)) { - sleep(1); - LLEventPumps::instance().obtain("mainloop").post(LLSD()); + yield(); } } @@ -219,6 +225,68 @@ private: std::string mPath; }; +// statically reference the function in test.cpp... it's short, we could +// replicate, but better to reuse +extern void wouldHaveCrashed(const std::string& message); + +/** + * Capture log messages. This is adapted (simplified) from the one in + * llerror_test.cpp. Sigh, should've broken that out into a separate header + * file, but time for this project is short... + */ +class TestRecorder : public LLError::Recorder +{ +public: + TestRecorder(): + // Mostly what we're trying to accomplish by saving and resetting + // LLError::Settings is to bypass the default RecordToStderr and + // RecordToWinDebug Recorders. As these are visible only inside + // llerror.cpp, we can't just call LLError::removeRecorder() with + // each. For certain tests we need to produce, capture and examine + // DEBUG log messages -- but we don't want to spam the user's console + // with that output. If it turns out that saveAndResetSettings() has + // some bad effect, give up and just let the DEBUG level log messages + // display. + mOldSettings(LLError::saveAndResetSettings()) + { + LLError::setFatalFunction(wouldHaveCrashed); + LLError::setDefaultLevel(LLError::LEVEL_DEBUG); + LLError::addRecorder(this); + } + + ~TestRecorder() + { + LLError::removeRecorder(this); + LLError::restoreSettings(mOldSettings); + } + + void recordMessage(LLError::ELevel level, + const std::string& message) + { + mMessages.push_back(message); + } + + /// Don't assume the message we want is necessarily the LAST log message + /// emitted by the underlying code; search backwards through all messages + /// for the sought string. + std::string messageWith(const std::string& search) + { + for (std::list::const_reverse_iterator rmi(mMessages.rbegin()), + rmend(mMessages.rend()); + rmi != rmend; ++rmi) + { + if (rmi->find(search) != std::string::npos) + return *rmi; + } + // failed to find any such message + return std::string(); + } + + typedef std::list MessageList; + MessageList mMessages; + LLError::Settings* mOldSettings; +}; + /***************************************************************************** * TUT *****************************************************************************/ @@ -602,9 +670,19 @@ namespace tut set_test_name("syntax_error:"); PythonProcessLauncher py("syntax_error:", "syntax_error:\n"); + py.mParams.files.add(LLProcess::FileParam()); // inherit stdin + py.mParams.files.add(LLProcess::FileParam()); // inherit stdout + py.mParams.files.add(LLProcess::FileParam("pipe")); // pipe for stderr py.run(); ensure_equals("Status.mState", py.mPy->getStatus().mState, LLProcess::EXITED); ensure_equals("Status.mData", py.mPy->getStatus().mData, 1); + std::istream& rpipe(py.mPy->getReadPipe(LLProcess::STDERR).get_istream()); + std::vector buffer(4096); + rpipe.read(&buffer[0], buffer.size()); + std::streamsize got(rpipe.gcount()); + ensure("Nothing read from stderr pipe", got); + std::string data(&buffer[0], got); + ensure("Didn't find 'SyntaxError:'", data.find("\nSyntaxError:") != std::string::npos); } template<> template<> @@ -629,7 +707,7 @@ namespace tut int i = 0, timeout = 60; for ( ; i < timeout; ++i) { - sleep(1); + yield(); if (readfile(out.getName(), "from kill() script") == "ok") break; } @@ -678,7 +756,7 @@ namespace tut int i = 0, timeout = 60; for ( ; i < timeout; ++i) { - sleep(1); + yield(); if (readfile(out.getName(), "from kill() script") == "ok") break; } @@ -733,7 +811,7 @@ namespace tut int i = 0, timeout = 60; for ( ; i < timeout; ++i) { - sleep(1); + yield(); if (readfile(from.getName(), "from autokill script") == "ok") break; } @@ -742,7 +820,7 @@ namespace tut // Now destroy the LLProcess, which should NOT kill the child! } // If the destructor killed the child anyway, give it time to die - sleep(2); + yield(2); // How do we know it's not terminated? By making it respond to // a specific stimulus in a specific way. { @@ -755,4 +833,81 @@ namespace tut // script could not have written 'ack' as we expect. ensure_equals("autokill script output", readfile(from.getName()), "ack"); } + + template<> template<> + void object::test<10>() + { + set_test_name("'bogus' test"); + TestRecorder recorder; + PythonProcessLauncher py("'bogus' test", + "print 'Hello world'\n"); + py.mParams.files.add(LLProcess::FileParam("bogus")); + py.mPy = LLProcess::create(py.mParams); + ensure("should have rejected 'bogus'", ! py.mPy); + std::string message(recorder.messageWith("bogus")); + ensure("did not log 'bogus' type", ! message.empty()); + ensure_contains("did not name 'stdin'", message, "stdin"); + } + + template<> template<> + void object::test<11>() + { + set_test_name("'file' test"); + // Replace this test with one or more real 'file' tests when we + // implement 'file' support + PythonProcessLauncher py("'file' test", + "print 'Hello world'\n"); + py.mParams.files.add(LLProcess::FileParam()); + py.mParams.files.add(LLProcess::FileParam("file")); + py.mPy = LLProcess::create(py.mParams); + ensure("should have rejected 'file'", ! py.mPy); + } + + template<> template<> + void object::test<12>() + { + set_test_name("'tpipe' test"); + // Replace this test with one or more real 'tpipe' tests when we + // implement 'tpipe' support + TestRecorder recorder; + PythonProcessLauncher py("'tpipe' test", + "print 'Hello world'\n"); + py.mParams.files.add(LLProcess::FileParam()); + py.mParams.files.add(LLProcess::FileParam("tpipe")); + py.mPy = LLProcess::create(py.mParams); + ensure("should have rejected 'tpipe'", ! py.mPy); + std::string message(recorder.messageWith("tpipe")); + ensure("did not log 'tpipe' type", ! message.empty()); + ensure_contains("did not name 'stdout'", message, "stdout"); + } + + template<> template<> + void object::test<13>() + { + set_test_name("'npipe' test"); + // Replace this test with one or more real 'npipe' tests when we + // implement 'npipe' support + TestRecorder recorder; + PythonProcessLauncher py("'npipe' test", + "print 'Hello world'\n"); + py.mParams.files.add(LLProcess::FileParam()); + py.mParams.files.add(LLProcess::FileParam()); + py.mParams.files.add(LLProcess::FileParam("npipe")); + py.mPy = LLProcess::create(py.mParams); + ensure("should have rejected 'npipe'", ! py.mPy); + std::string message(recorder.messageWith("npipe")); + ensure("did not log 'npipe' type", ! message.empty()); + ensure_contains("did not name 'stderr'", message, "stderr"); + } + + // TODO: + // test "pipe" with nonempty name (should log & continue) + // test pipe for just stderr (tests for get[Opt]ReadPipe(), get[Opt]WritePipe()) + // test pipe for stdin, stdout (etc.) + // test getWritePipe().get_ostream(), getReadPipe().get_istream() + // test listening on getReadPipe().getPump(), disconnecting + // test setLimit(), getLimit() + // test EOF -- check logging + // test get(Read|Write)Pipe(3), unmonitored slot, getReadPipe(1), getWritePipe(0) + } // namespace tut -- cgit v1.3 From c6ccdb5b5088f3ab24bb89d8c56049c7a17a663e Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 15 Feb 2012 12:11:38 -0500 Subject: Add tests for LLProcess::get[Opt][Read|Write]Pipe() validations. --- indra/llcommon/tests/llprocess_test.cpp | 90 +++++++++++++++++++++++++++++++-- 1 file changed, 87 insertions(+), 3 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index d4e9977e63..06f9f3827f 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -900,14 +900,98 @@ namespace tut ensure_contains("did not name 'stderr'", message, "stderr"); } + template<> template<> + void object::test<14>() + { + set_test_name("internal pipe name warning"); + TestRecorder recorder; + PythonProcessLauncher py("pipe warning", + "import sys\n" + "sys.exit(7)\n"); + py.mParams.files.add(LLProcess::FileParam("pipe", "somename")); + py.run(); // verify that it did launch anyway + ensure_equals("Status.mState", py.mPy->getStatus().mState, LLProcess::EXITED); + ensure_equals("Status.mData", py.mPy->getStatus().mData, 7); + std::string message(recorder.messageWith("not yet supported")); + ensure("did not log pipe name warning", ! message.empty()); + ensure_contains("log message did not mention internal pipe name", + message, "somename"); + } + +#define TEST_getPipe(PROCESS, GETPIPE, GETOPTPIPE, VALID, NOPIPE, BADPIPE) \ + do \ + { \ + std::string threw; \ + /* Both the following calls should work. */ \ + (PROCESS).GETPIPE(VALID); \ + ensure(#GETOPTPIPE "(" #VALID ") failed", (PROCESS).GETOPTPIPE(VALID)); \ + /* pass obviously bogus PIPESLOT */ \ + CATCH_IN(threw, LLProcess::NoPipe, (PROCESS).GETPIPE(LLProcess::FILESLOT(4))); \ + ensure_contains("didn't reject bad slot", threw, "no slot"); \ + ensure_contains("didn't mention bad slot num", threw, "4"); \ + EXPECT_FAIL_WITH_LOG(threw, (PROCESS).GETOPTPIPE(LLProcess::FILESLOT(4))); \ + /* pass NOPIPE */ \ + CATCH_IN(threw, LLProcess::NoPipe, (PROCESS).GETPIPE(NOPIPE)); \ + ensure_contains("didn't reject non-pipe", threw, "not a monitored"); \ + EXPECT_FAIL_WITH_LOG(threw, (PROCESS).GETOPTPIPE(NOPIPE)); \ + /* pass BADPIPE: FILESLOT isn't empty but wrong direction */ \ + CATCH_IN(threw, LLProcess::NoPipe, (PROCESS).GETPIPE(BADPIPE)); \ + /* sneaky: GETPIPE is getReadPipe or getWritePipe */ \ + /* so skip "get" to obtain ReadPipe or WritePipe :-P */ \ + ensure_contains("didn't reject wrong pipe", threw, (#GETPIPE)+3); \ + EXPECT_FAIL_WITH_LOG(threw, (PROCESS).GETOPTPIPE(BADPIPE)); \ + } while (0) + +/// For expecting exceptions. Execute CODE, catch EXCEPTION, store its what() +/// in std::string THREW, ensure it's not empty (i.e. EXCEPTION did happen). +#define CATCH_IN(THREW, EXCEPTION, CODE) \ + do \ + { \ + (THREW).clear(); \ + try \ + { \ + CODE; \ + } \ + catch (const EXCEPTION& e) \ + { \ + (THREW) = e.what(); \ + } \ + ensure("failed to throw " #EXCEPTION ": " #CODE, ! (THREW).empty()); \ + } while (0) + +#define EXPECT_FAIL_WITH_LOG(EXPECT, CODE) \ + do \ + { \ + TestRecorder recorder; \ + ensure(#CODE " succeeded", ! (CODE)); \ + ensure("wrong log message", ! recorder.messageWith(EXPECT).empty()); \ + } while (0) + + template<> template<> + void object::test<15>() + { + set_test_name("get*Pipe() validation"); + PythonProcessLauncher py("just stderr", + "print 'this output is expected'\n"); + py.mParams.files.add(LLProcess::FileParam("pipe")); // pipe for stdin + py.mParams.files.add(LLProcess::FileParam()); // inherit stdout + py.mParams.files.add(LLProcess::FileParam("pipe")); // pipe for stderr + py.run(); + TEST_getPipe(*py.mPy, getWritePipe, getOptWritePipe, + LLProcess::STDIN, // VALID + LLProcess::STDOUT, // NOPIPE + LLProcess::STDERR); // BADPIPE + TEST_getPipe(*py.mPy, getReadPipe, getOptReadPipe, + LLProcess::STDERR, // VALID + LLProcess::STDOUT, // NOPIPE + LLProcess::STDIN); // BADPIPE + } + // TODO: - // test "pipe" with nonempty name (should log & continue) - // test pipe for just stderr (tests for get[Opt]ReadPipe(), get[Opt]WritePipe()) // test pipe for stdin, stdout (etc.) // test getWritePipe().get_ostream(), getReadPipe().get_istream() // test listening on getReadPipe().getPump(), disconnecting // test setLimit(), getLimit() // test EOF -- check logging - // test get(Read|Write)Pipe(3), unmonitored slot, getReadPipe(1), getWritePipe(0) } // namespace tut -- cgit v1.3 From 10ab4adc86207f86df30ab23d8858c23e7f550ea Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 15 Feb 2012 13:44:43 -0500 Subject: Fix llprocess_test.cpp's exception catching for Linux. In the course of re-enabling the indra/test tests last year, Log generalized a workaround I'd introduced in llsdmessage_test.cpp. In Linux viewer land, a test program trying to catch an expected exception can't seem to catch it by its specific class (across the libllcommon.so boundary), but must instead catch std::runtime_error and validate the typeid().name() string. Log added a macro for this idiom in llevents_tut.cpp. Generalize that macro further for normal-case processing as well, move it to a header file of its own and use it in all known places -- plus the new exception-catching tests in llprocess_test.cpp. --- indra/llcommon/tests/llprocess_test.cpp | 6 +-- indra/llmessage/tests/llsdmessage_test.cpp | 36 ++----------- indra/test/catch_and_store_what_in.h | 86 ++++++++++++++++++++++++++++++ indra/test/llevents_tut.cpp | 69 +++--------------------- 4 files changed, 98 insertions(+), 99 deletions(-) create mode 100644 indra/test/catch_and_store_what_in.h (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 06f9f3827f..a901c577d6 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -31,6 +31,7 @@ #include "../test/lltut.h" #include "../test/manageapr.h" #include "../test/namedtempfile.h" +#include "../test/catch_and_store_what_in.h" #include "stringize.h" #include "llsdutil.h" #include "llevents.h" @@ -952,10 +953,7 @@ namespace tut { \ CODE; \ } \ - catch (const EXCEPTION& e) \ - { \ - (THREW) = e.what(); \ - } \ + CATCH_AND_STORE_WHAT_IN(THREW, EXCEPTION) \ ensure("failed to throw " #EXCEPTION ": " #CODE, ! (THREW).empty()); \ } while (0) diff --git a/indra/llmessage/tests/llsdmessage_test.cpp b/indra/llmessage/tests/llsdmessage_test.cpp index 0f2c069303..6871ac0d52 100644 --- a/indra/llmessage/tests/llsdmessage_test.cpp +++ b/indra/llmessage/tests/llsdmessage_test.cpp @@ -42,6 +42,7 @@ // external library headers // other Linden headers #include "../test/lltut.h" +#include "../test/catch_and_store_what_in.h" #include "llsdserialize.h" #include "llevents.h" #include "stringize.h" @@ -72,43 +73,14 @@ namespace tut template<> template<> void llsdmessage_object::test<1>() { - bool threw = false; + std::string threw; // This should fail... try { LLSDMessage localListener; } - catch (const LLEventPump::DupPumpName&) - { - threw = true; - } - catch (const std::runtime_error& ex) - { - // This clause is because on Linux, on the viewer side, for this - // one test program (though not others!), the - // LLEventPump::DupPumpName exception isn't caught by the clause - // above. Warn the user... - std::cerr << "Failed to catch " << typeid(ex).name() << std::endl; - // But if the expected exception was thrown, allow the test to - // succeed anyway. Not sure how else to handle this odd case. - if (std::string(typeid(ex).name()) == typeid(LLEventPump::DupPumpName).name()) - { - threw = true; - } - else - { - // We don't even recognize this exception. Let it propagate - // out to TUT to fail the test. - throw; - } - } - catch (...) - { - std::cerr << "Utterly failed to catch expected exception!" << std::endl; - // This case is full of fail. We HAVE to address it. - throw; - } - ensure("second LLSDMessage should throw", threw); + CATCH_AND_STORE_WHAT_IN(threw, LLEventPump::DupPumpName) + ensure("second LLSDMessage should throw", ! threw.empty()); } template<> template<> diff --git a/indra/test/catch_and_store_what_in.h b/indra/test/catch_and_store_what_in.h new file mode 100644 index 0000000000..59f8cc0085 --- /dev/null +++ b/indra/test/catch_and_store_what_in.h @@ -0,0 +1,86 @@ +/** + * @file catch_and_store_what_in.h + * @author Nat Goodspeed + * @date 2012-02-15 + * @brief CATCH_AND_STORE_WHAT_IN() macro + * + * $LicenseInfo:firstyear=2012&license=viewerlgpl$ + * Copyright (c) 2012, Linden Research, Inc. + * $/LicenseInfo$ + */ + +#if ! defined(LL_CATCH_AND_STORE_WHAT_IN_H) +#define LL_CATCH_AND_STORE_WHAT_IN_H + +/** + * Idiom useful for test programs: catch an expected exception, store its + * what() string in a specified std::string variable. From there the caller + * can do things like: + * @code + * ensure("expected exception not thrown", ! string.empty()); + * @endcode + * or + * @code + * ensure_contains("exception doesn't mention blah", string, "blah"); + * @endcode + * etc. + * + * The trouble is that when linking to a dynamic libllcommon.so on Linux, we + * generally fail to catch the specific exception. Oddly, we can catch it as + * std::runtime_error and validate its typeid().name(), so we do -- but that's + * a lot of boilerplate per test. Encapsulate with this macro. Usage: + * + * @code + * std::string threw; + * try + * { + * some_call_that_should_throw_Foo(); + * } + * CATCH_AND_STORE_WHAT_IN(threw, Foo) + * ensure("some_call_that_should_throw_Foo() didn't throw", ! threw.empty()); + * @endcode + */ +#define CATCH_AND_STORE_WHAT_IN(THREW, EXCEPTION) \ +catch (const EXCEPTION& ex) \ +{ \ + (THREW) = ex.what(); \ +} \ +CATCH_MISSED_LINUX_EXCEPTION(THREW, EXCEPTION) + +#ifndef LL_LINUX +#define CATCH_MISSED_LINUX_EXCEPTION(THREW, EXCEPTION) \ + /* only needed on Linux */ +#else // LL_LINUX + +#define CATCH_MISSED_LINUX_EXCEPTION(THREW, EXCEPTION) \ +catch (const std::runtime_error& ex) \ +{ \ + /* This clause is needed on Linux, on the viewer side, because */ \ + /* the exception isn't caught by catch (const EXCEPTION&). */ \ + /* But if the expected exception was thrown, allow the test to */ \ + /* succeed anyway. Not sure how else to handle this odd case. */ \ + if (std::string(typeid(ex).name()) == typeid(EXCEPTION).name()) \ + { \ + /* std::cerr << "Caught " << typeid(ex).name() */ \ + /* << " with Linux workaround" << std::endl; */ \ + (THREW) = ex.what(); \ + /*std::cout << ex.what() << std::endl;*/ \ + } \ + else \ + { \ + /* We don't even recognize this exception. Let it propagate */ \ + /* out to TUT to fail the test. */ \ + throw; \ + } \ +} \ +catch (...) \ +{ \ + std::cerr << "Failed to catch expected exception " \ + << #EXCEPTION << "!" << std::endl; \ + /* This indicates a problem in the test that should be addressed. */ \ + throw; \ +} + +#endif // LL_LINUX + +#endif /* ! defined(LL_CATCH_AND_STORE_WHAT_IN_H) */ diff --git a/indra/test/llevents_tut.cpp b/indra/test/llevents_tut.cpp index 4699bb1827..ca4c74099f 100644 --- a/indra/test/llevents_tut.cpp +++ b/indra/test/llevents_tut.cpp @@ -49,46 +49,12 @@ #include // other Linden headers #include "lltut.h" +#include "catch_and_store_what_in.h" #include "stringize.h" #include "tests/listener.h" using boost::assign::list_of; -#ifdef LL_LINUX -#define CATCH_MISSED_LINUX_EXCEPTION(exception, threw) \ -catch (const std::runtime_error& ex) \ -{ \ - /* This clause is needed on Linux, on the viewer side, because the */ \ - /* exception isn't caught by the clause above. Warn the user... */ \ - std::cerr << "Failed to catch " << typeid(ex).name() << std::endl; \ - /* But if the expected exception was thrown, allow the test to */ \ - /* succeed anyway. Not sure how else to handle this odd case. */ \ - /* This approach is also used in llsdmessage_test.cpp. */ \ - if (std::string(typeid(ex).name()) == typeid(exception).name()) \ - { \ - threw = ex.what(); \ - /*std::cout << ex.what() << std::endl;*/ \ - } \ - else \ - { \ - /* We don't even recognize this exception. Let it propagate */ \ - /* out to TUT to fail the test. */ \ - throw; \ - } \ -} \ -catch (...) \ -{ \ - std::cerr << "Utterly failed to catch expected exception " << #exception << "!" << \ - std::endl; \ - /* This indicates a problem in the test that should be addressed. */ \ - throw; \ -} - -#else // LL_LINUX -#define CATCH_MISSED_LINUX_EXCEPTION(exception, threw) \ - /* Not needed on other platforms */ -#endif // LL_LINUX - template T make(const T& value) { @@ -178,11 +144,7 @@ void events_object::test<1>() per_frame.listen(listener0.getName(), // note bug, dup name boost::bind(&Listener::call, boost::ref(listener1), _1)); } - catch (const LLEventPump::DupListenerName& e) - { - threw = e.what(); - } - CATCH_MISSED_LINUX_EXCEPTION(LLEventPump::DupListenerName, threw) + CATCH_AND_STORE_WHAT_IN(threw, LLEventPump::DupListenerName) ensure_equals(threw, std::string("DupListenerName: " "Attempt to register duplicate listener name '") + @@ -388,12 +350,7 @@ void events_object::test<7>() // after "Mary" and "checked" -- whoops! make(list_of("Mary")("checked"))); } - catch (const LLEventPump::Cycle& e) - { - threw = e.what(); - // std::cout << "Caught: " << e.what() << '\n'; - } - CATCH_MISSED_LINUX_EXCEPTION(LLEventPump::Cycle, threw) + CATCH_AND_STORE_WHAT_IN(threw, LLEventPump::Cycle) // Obviously the specific wording of the exception text can // change; go ahead and change the test to match. // Establish that it contains: @@ -426,12 +383,7 @@ void events_object::test<7>() make(list_of("shoelaces")), make(list_of("yellow"))); } - catch (const LLEventPump::OrderChange& e) - { - threw = e.what(); - // std::cout << "Caught: " << e.what() << '\n'; - } - CATCH_MISSED_LINUX_EXCEPTION(LLEventPump::OrderChange, threw) + CATCH_AND_STORE_WHAT_IN(threw, LLEventPump::OrderChange) // Same remarks about the specific wording of the exception. Just // ensure that it contains enough information to clarify the // problem and what must be done to resolve it. @@ -459,12 +411,7 @@ void events_object::test<8>() // then another with a duplicate name. LLEventStream bob2("bob"); } - catch (const LLEventPump::DupPumpName& e) - { - threw = e.what(); - // std::cout << "Caught: " << e.what() << '\n'; - } - CATCH_MISSED_LINUX_EXCEPTION(LLEventPump::DupPumpName, threw) + CATCH_AND_STORE_WHAT_IN(threw, LLEventPump::DupPumpName) ensure("Caught DupPumpName", !threw.empty()); } // delete first 'bob' LLEventStream bob("bob"); // should work, previous one unregistered @@ -505,11 +452,7 @@ void events_object::test<9>() LLListenerOrPumpName empty; empty(17); } - catch (const LLListenerOrPumpName::Empty& e) - { - threw = e.what(); - } - CATCH_MISSED_LINUX_EXCEPTION(LLListenerOrPumpName::Empty, threw) + CATCH_AND_STORE_WHAT_IN(threw, LLListenerOrPumpName::Empty) ensure("threw Empty", !threw.empty()); } -- cgit v1.3 From 56d931216e67a3e59199669bba022c65a9617bb5 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 15 Feb 2012 15:47:03 -0500 Subject: Add LLProcess::ReadPipe::size(), peek(), contains(). Also add "len" key to event data on LLProcess::getPump(). If you've used setLimit(), event["data"].length() may not reflect the length of the accumulated data in the ReadPipe. Add unit test with stdin/stdout handshake with child process. --- indra/llcommon/llprocess.cpp | 31 +++++++++++++++++++----- indra/llcommon/llprocess.h | 25 +++++++++++++++++++ indra/llcommon/tests/llprocess_test.cpp | 43 +++++++++++++++++++++++++++++++-- 3 files changed, 91 insertions(+), 8 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index d7c297b952..1481bf571f 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -197,6 +197,25 @@ public: virtual LLEventPump& getPump() { return mPump; } virtual void setLimit(size_t limit) { mLimit = limit; } virtual size_t getLimit() const { return mLimit; } + virtual std::size_t size() { return mStreambuf.size(); } + + virtual std::string peek(std::size_t offset=0, + std::size_t len=(std::numeric_limits::max)()) + { + // Constrain caller's offset and len to overlap actual buffer content. + std::size_t real_offset = (std::min)(mStreambuf.size(), offset); + std::size_t real_end = (std::min)(mStreambuf.size(), real_offset + len); + boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data(); + return std::string(boost::asio::buffers_begin(cbufs) + real_offset, + boost::asio::buffers_begin(cbufs) + real_end); + } + + virtual bool contains(const std::string& seek, std::size_t offset=0) + { + // There may be a more efficient way to search mStreambuf contents, + // but this is far the easiest... + return peek(offset).find(seek) != std::string::npos; + } private: bool tick(const LLSD&) @@ -240,12 +259,13 @@ private: << mStreambuf.size() << LL_ENDL; // Now that we've received new data, publish it on our - // LLEventPump as advertised. Constrain it by mLimit. + // LLEventPump as advertised. Constrain it by mLimit. But show + // listener the actual accumulated buffer size, regardless of + // mLimit. std::size_t datasize((std::min)(mLimit, mStreambuf.size())); - boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data(); - mPump.post(LLSDMap("data", LLSD::String( - boost::asio::buffers_begin(cbufs), - boost::asio::buffers_begin(cbufs) + datasize))); + mPump.post(LLSDMap + ("data", peek(0, datasize)) + ("len", LLSD::Integer(mStreambuf.size()))); } } return false; @@ -985,5 +1005,4 @@ void LLProcess::reap(void) } } |*==========================================================================*/ - #endif // Posix diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 448a88f4c0..bf0517600d 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -330,6 +330,31 @@ public: */ virtual std::istream& get_istream() = 0; + /** + * Get accumulated buffer length. + * Often we need to refrain from actually reading the std::istream + * returned by get_istream() until we've accumulated enough data to + * make it worthwhile. For instance, if we're expecting a number from + * the child, but the child happens to flush "12" before emitting + * "3\n", get_istream() >> myint could return 12 rather than 123! + */ + virtual std::size_t size() = 0; + + /** + * Peek at accumulated buffer data without consuming it. Optional + * parameters give you substr() functionality. + * + * @note You can discard buffer data using get_istream().ignore(n). + */ + virtual std::string peek(std::size_t offset=0, + std::size_t len=(std::numeric_limits::max)()) = 0; + + /** + * Search accumulated buffer data without retrieving it. Optional + * offset allows you to start at specified position. + */ + virtual bool contains(const std::string& seek, std::size_t offset=0) = 0; + /** * Get LLEventPump& on which to listen for incoming data. The posted * LLSD::Map event will contain a key "data" whose value is an diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index a901c577d6..2db17cae97 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -919,6 +919,7 @@ namespace tut message, "somename"); } + /*-------------- support for "get*Pipe() validation" test --------------*/ #define TEST_getPipe(PROCESS, GETPIPE, GETOPTPIPE, VALID, NOPIPE, BADPIPE) \ do \ { \ @@ -985,11 +986,49 @@ namespace tut LLProcess::STDIN); // BADPIPE } + template<> template<> + void object::test<16>() + { + set_test_name("talk to stdin/stdout"); + PythonProcessLauncher py("stdin/stdout", + "import sys, time\n" + "print 'ok'\n" + "sys.stdout.flush()\n" + "# wait for 'go' from test program\n" + "go = sys.stdin.readline()\n" + "if go != 'go\\n':\n" + " sys.exit('expected \"go\", saw %r' % go)\n" + "print 'ack'\n"); + py.mParams.files.add(LLProcess::FileParam("pipe")); // stdin + py.mParams.files.add(LLProcess::FileParam("pipe")); // stdout + py.mPy = LLProcess::create(py.mParams); + ensure("couldn't launch stdin/stdout script", py.mPy); + LLProcess::ReadPipe& childout(py.mPy->getReadPipe(LLProcess::STDOUT)); + int i, timeout = 60; + for (i = 0; i < timeout && py.mPy->isRunning() && childout.size() < 3; ++i) + { + yield(); + } + ensure("script never started", i < timeout); + std::string line; + std::getline(childout.get_istream(), line); + ensure_equals("bad wakeup from stdin/stdout script", line, "ok"); + py.mPy->getWritePipe().get_ostream() << "go" << std::endl; + for (i = 0; i < timeout && py.mPy->isRunning() && ! childout.contains("\n"); ++i) + { + yield(); + } + ensure("script never replied", childout.contains("\n")); + std::getline(childout.get_istream(), line); + ensure_equals("child didn't ack", line, "ack"); + ensure_equals("bad child termination", py.mPy->getStatus().mState, LLProcess::EXITED); + ensure_equals("bad child exit code", py.mPy->getStatus().mData, 0); + } + // TODO: - // test pipe for stdin, stdout (etc.) - // test getWritePipe().get_ostream(), getReadPipe().get_istream() // test listening on getReadPipe().getPump(), disconnecting // test setLimit(), getLimit() // test EOF -- check logging + // test peek() with substr } // namespace tut -- cgit v1.3 From fc6d70db8771320f3b1136e76383fc85ddf1b6b2 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 15 Feb 2012 20:57:25 -0500 Subject: Don't be confused by "\r\n" line endings on pipe on Windows. These are all very well when we just want to dump the output to a log, or whatever, but in a unit-test context it matters for comparison. --- indra/llcommon/tests/llprocess_test.cpp | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 2db17cae97..6d6b888471 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -288,6 +288,22 @@ public: LLError::Settings* mOldSettings; }; +std::string getline(std::istream& in) +{ + std::string line; + std::getline(in, line); + // Blur the distinction between "\r\n" and plain "\n". std::getline() will + // have eaten the "\n", but we could still end up with a trailing "\r". + std::string::size_type lastpos = line.find_last_not_of("\r"); + if (lastpos != std::string::npos) + { + // Found at least one character that's not a trailing '\r'. SKIP OVER + // IT and then erase the rest of the line. + line.erase(lastpos+1); + } + return line; +} + /***************************************************************************** * TUT *****************************************************************************/ @@ -1010,17 +1026,15 @@ namespace tut yield(); } ensure("script never started", i < timeout); - std::string line; - std::getline(childout.get_istream(), line); - ensure_equals("bad wakeup from stdin/stdout script", line, "ok"); + ensure_equals("bad wakeup from stdin/stdout script", + getline(childout.get_istream()), "ok"); py.mPy->getWritePipe().get_ostream() << "go" << std::endl; for (i = 0; i < timeout && py.mPy->isRunning() && ! childout.contains("\n"); ++i) { yield(); } ensure("script never replied", childout.contains("\n")); - std::getline(childout.get_istream(), line); - ensure_equals("child didn't ack", line, "ack"); + ensure_equals("child didn't ack", getline(childout.get_istream()), "ack"); ensure_equals("bad child termination", py.mPy->getStatus().mState, LLProcess::EXITED); ensure_equals("bad child exit code", py.mPy->getStatus().mData, 0); } -- cgit v1.3 From 85057908c3f7e48f1dc086ea1c82e672674b2596 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 15 Feb 2012 21:55:53 -0500 Subject: Add unit test for listening on LLProcess::ReadPipe::getPump(). --- indra/llcommon/tests/llprocess_test.cpp | 89 ++++++++++++++++++++++++++++++++- 1 file changed, 88 insertions(+), 1 deletion(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 6d6b888471..31bc833a1d 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -1028,6 +1028,7 @@ namespace tut ensure("script never started", i < timeout); ensure_equals("bad wakeup from stdin/stdout script", getline(childout.get_istream()), "ok"); + // important to get the implicit flush from std::endl py.mPy->getWritePipe().get_ostream() << "go" << std::endl; for (i = 0; i < timeout && py.mPy->isRunning() && ! childout.contains("\n"); ++i) { @@ -1039,8 +1040,94 @@ namespace tut ensure_equals("bad child exit code", py.mPy->getStatus().mData, 0); } + struct EventListener: public boost::noncopyable + { + EventListener(LLEventPump& pump) + { + mConnection = + pump.listen("EventListener", boost::bind(&EventListener::tick, this, _1)); + } + + bool tick(const LLSD& data) + { + mHistory.push_back(data); + return false; + } + + std::list mHistory; + LLTempBoundListener mConnection; + }; + + static bool ack(std::ostream& out, const LLSD& data) + { + out << "continue" << std::endl; + return false; + } + + template<> template<> + void object::test<17>() + { + set_test_name("listen for ReadPipe events"); + PythonProcessLauncher py("ReadPipe listener", + "import sys\n" + "sys.stdout.write('abc')\n" + "sys.stdout.flush()\n" + "sys.stdin.readline()\n" + "sys.stdout.write('def')\n" + "sys.stdout.flush()\n" + "sys.stdin.readline()\n" + "sys.stdout.write('ghi\\n')\n" + "sys.stdout.flush()\n" + "sys.stdin.readline()\n" + "sys.stdout.write('second line\\n')\n"); + py.mParams.files.add(LLProcess::FileParam("pipe")); // stdin + py.mParams.files.add(LLProcess::FileParam("pipe")); // stdout + py.mPy = LLProcess::create(py.mParams); + ensure("couldn't launch ReadPipe listener script", py.mPy); + std::ostream& childin(py.mPy->getWritePipe(LLProcess::STDIN).get_ostream()); + LLProcess::ReadPipe& childout(py.mPy->getReadPipe(LLProcess::STDOUT)); + // listen for incoming data on childout + EventListener listener(childout.getPump()); + // also listen with a function that prompts the child to continue + // every time we see output + LLTempBoundListener connection( + childout.getPump().listen("ack", boost::bind(ack, boost::ref(childin), _1))); + int i, timeout = 60; + // wait through stuttering first line + for (i = 0; i < timeout && py.mPy->isRunning() && ! childout.contains("\n"); ++i) + { + yield(); + } + ensure("couldn't get first line", i < timeout); + // disconnect from listener + listener.mConnection.disconnect(); + // finish out the run + for (i = 0; i < timeout && py.mPy->isRunning(); ++i) + { + yield(); + } + ensure("child took too long to terminate", i < timeout); + // now verify history + std::list::const_iterator li(listener.mHistory.begin()), + lend(listener.mHistory.end()); + ensure("no events", li != lend); + ensure_equals("history[0]", (*li)["data"].asString(), "abc"); + ensure_equals("history[0] len", (*li)["len"].asInteger(), 3); + ++li; + ensure("only 1 event", li != lend); + ensure_equals("history[1]", (*li)["data"].asString(), "abcdef"); + ensure_equals("history[0] len", (*li)["len"].asInteger(), 6); + ++li; + ensure("only 2 events", li != lend); + ensure_equals("history[2]", (*li)["data"].asString(), "abcdefghi" EOL); + ensure_equals("history[0] len", (*li)["len"].asInteger(), 9 + sizeof(EOL) - 1); + ++li; + // We DO NOT expect a whole new event for the second line because we + // disconnected. + ensure("more than 3 events", li == lend); + } + // TODO: - // test listening on getReadPipe().getPump(), disconnecting // test setLimit(), getLimit() // test EOF -- check logging // test peek() with substr -- cgit v1.3 From e92c3113545dd60fb76e115da201163e340c730c Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 Feb 2012 16:05:04 -0500 Subject: Add LLProcess::ReadPipe::find() methods, with corresponding npos. If it's useful to have contains() to tell you whether incoming data contains a particular substring, and if it's useful for contains() and peek() to accept an offset within that data, then it's useful to allow you to get the offset of a desired substring within that data. But of course a find() returning offset needs something like std::string::npos for "not found"; borrow that convention. Support both find(const std::string&) and find(char); the latter permits a more efficient implementation. In fact, make find(string) recognize a string of length 1 and leverage the find(char) implementation. Given that, reimplement contains(mumble) as shorthand for find(mumble) != npos. Implement find() overloads using std::search() and std::find() on boost::asio::streambuf character iterators, rather than copying to std::string and then using string search like previous contains() implementation. Reimplement WritePipeImpl::tick() and ReadPipeImpl::tick() to write/read directly from/to boost::asio::streambuf data, instead of copying to/from a temporary flat buffer. As long as ReadPipeImpl::tick() keeps successfully filling buffers, keep reading. Previous implementation would only handle a long child write over successive tick() calls. Stop on read error or when we come up short. --- indra/llcommon/llprocess.cpp | 259 ++++++++++++++++++++++---------- indra/llcommon/llprocess.h | 37 ++++- indra/llcommon/tests/llprocess_test.cpp | 2 + 3 files changed, 210 insertions(+), 88 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 1481bf571f..aa22b3f805 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -120,9 +120,12 @@ private: static LLProcessListener sProcessListener; LLProcess::BasePipe::~BasePipe() {} +const LLProcess::BasePipe::size_type + LLProcess::BasePipe::npos((std::numeric_limits::max)()); class WritePipeImpl: public LLProcess::WritePipe { + LOG_CLASS(WritePipeImpl); public: WritePipeImpl(const std::string& desc, apr_file_t* pipe): mDesc(desc), @@ -139,30 +142,53 @@ public: bool tick(const LLSD&) { + typedef boost::asio::streambuf::const_buffers_type const_buffer_sequence; // If there's anything to send, try to send it. - if (mStreambuf.size()) + std::size_t total(mStreambuf.size()), consumed(0); + if (total) { - // Copy data out from mStreambuf to a flat, contiguous buffer to - // write -- but only up to a certain size. - std::size_t total(mStreambuf.size()); - std::size_t bufsize((std::min)(std::size_t(4096), total)); - boost::asio::streambuf::const_buffers_type bufs = mStreambuf.data(); - std::vector buffer( - boost::asio::buffers_begin(bufs), - boost::asio::buffers_begin(bufs) + bufsize); - apr_size_t written(bufsize); - ll_apr_warn_status(apr_file_write(mPipe, &buffer[0], &written)); - // 'written' is modified to reflect the number of bytes actually - // written. Since they've been sent, remove them from the + const_buffer_sequence bufs = mStreambuf.data(); + // In general, our streambuf might contain a number of different + // physical buffers; iterate over those. + for (const_buffer_sequence::const_iterator bufi(bufs.begin()), bufend(bufs.end()); + bufi != bufend; ++bufi) + { + // http://www.boost.org/doc/libs/1_49_0_beta1/doc/html/boost_asio/reference/buffer.html#boost_asio.reference.buffer.accessing_buffer_contents + std::size_t towrite(boost::asio::buffer_size(*bufi)); + apr_size_t written(towrite); + apr_status_t err = apr_file_write(mPipe, + boost::asio::buffer_cast(*bufi), + &written); + // EAGAIN is exactly what we want from a nonblocking pipe. + // Rather than waiting for data, it should return immediately. + if (! (err == APR_SUCCESS || APR_STATUS_IS_EAGAIN(err))) + { + LL_WARNS("LLProcess") << "apr_file_write(" << towrite << ") on " << mDesc + << " got " << err << ":" << LL_ENDL; + ll_apr_warn_status(err); + } + + // 'written' is modified to reflect the number of bytes actually + // written. Make sure we consume those later. (Don't consume them + // now, that would invalidate the buffer iterator sequence!) + consumed += written; + LL_DEBUGS("LLProcess") << "wrote " << written << " of " << towrite + << " bytes to " << mDesc + << " (original " << total << ")" << LL_ENDL; + + // The parent end of this pipe is nonblocking. If we weren't able + // to write everything we wanted, don't keep banging on it -- that + // won't change until the child reads some. Wait for next tick(). + if (written < towrite) + break; + } + // In all, we managed to write 'consumed' bytes. Remove them from the // streambuf so we don't keep trying to send them. This could be - // anywhere from 0 up to mStreambuf.size(); anything we haven't - // yet sent, we'll try again next tick() call. - mStreambuf.consume(written); - LL_DEBUGS("LLProcess") << "wrote " << written << " of " << bufsize - << " bytes to " << mDesc - << " (original " << total << "), " - << mStreambuf.size() << " remaining" << LL_ENDL; + // anywhere from 0 up to mStreambuf.size(); anything we haven't yet + // sent, we'll try again later. + mStreambuf.consume(consumed); } + return false; } @@ -176,6 +202,7 @@ private: class ReadPipeImpl: public LLProcess::ReadPipe { + LOG_CLASS(ReadPipeImpl); public: ReadPipeImpl(const std::string& desc, apr_file_t* pipe): mDesc(desc), @@ -184,7 +211,7 @@ public: mStream(&mStreambuf), mPump("ReadPipe"), // use funky syntax to call max() to avoid blighted max() macros - mLimit((std::numeric_limits::max)()) + mLimit(npos) { mConnection = LLEventPumps::instance().obtain("mainloop") .listen(LLEventPump::inventName("ReadPipe"), @@ -195,79 +222,149 @@ public: // methods with implementation data concealed from the base class. virtual std::istream& get_istream() { return mStream; } virtual LLEventPump& getPump() { return mPump; } - virtual void setLimit(size_t limit) { mLimit = limit; } - virtual size_t getLimit() const { return mLimit; } - virtual std::size_t size() { return mStreambuf.size(); } + virtual void setLimit(size_type limit) { mLimit = limit; } + virtual size_type getLimit() const { return mLimit; } + virtual size_type size() const { return mStreambuf.size(); } - virtual std::string peek(std::size_t offset=0, - std::size_t len=(std::numeric_limits::max)()) + virtual std::string peek(size_type offset=0, size_type len=npos) const { // Constrain caller's offset and len to overlap actual buffer content. - std::size_t real_offset = (std::min)(mStreambuf.size(), offset); - std::size_t real_end = (std::min)(mStreambuf.size(), real_offset + len); + std::size_t real_offset = (std::min)(mStreambuf.size(), std::size_t(offset)); + std::size_t real_end = (std::min)(mStreambuf.size(), std::size_t(real_offset + len)); boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data(); return std::string(boost::asio::buffers_begin(cbufs) + real_offset, boost::asio::buffers_begin(cbufs) + real_end); } - virtual bool contains(const std::string& seek, std::size_t offset=0) + virtual size_type find(const std::string& seek, size_type offset=0) const { - // There may be a more efficient way to search mStreambuf contents, - // but this is far the easiest... - return peek(offset).find(seek) != std::string::npos; + // If we're passing a string of length 1, use find(char), which can + // use an O(n) std::find() rather than the O(n^2) std::search(). + if (seek.length() == 1) + { + return find(seek[0], offset); + } + + // If offset is beyond the whole buffer, can't even construct a valid + // iterator range; can't possibly find the string we seek. + if (offset > mStreambuf.size()) + { + return npos; + } + + boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data(); + boost::asio::buffers_iterator + begin(boost::asio::buffers_begin(cbufs)), + end (boost::asio::buffers_end(cbufs)), + found(std::search(begin + offset, end, seek.begin(), seek.end())); + return (found == end)? npos : (found - begin); } -private: - bool tick(const LLSD&) + virtual size_type find(char seek, size_type offset=0) const { - // Allocate a buffer and try, every time, to read into it. - std::vector buffer(4096); - apr_size_t gotten(buffer.size()); - apr_status_t err = apr_file_read(mPipe, &buffer[0], &gotten); - if (err == APR_EOF) + // If offset is beyond the whole buffer, can't even construct a valid + // iterator range; can't possibly find the char we seek. + if (offset > mStreambuf.size()) { - // Handle EOF specially: it's part of normal-case processing. - LL_DEBUGS("LLProcess") << "EOF on " << mDesc << LL_ENDL; - // We won't need any more tick() calls. - mConnection.disconnect(); + return npos; } - else if (! ll_apr_warn_status(err)) // validate anything but EOF + + boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data(); + boost::asio::buffers_iterator + begin(boost::asio::buffers_begin(cbufs)), + end (boost::asio::buffers_end(cbufs)), + found(std::find(begin + offset, end, seek)); + return (found == end)? npos : (found - begin); + } + +private: + bool tick(const LLSD&) + { + typedef boost::asio::streambuf::mutable_buffers_type mutable_buffer_sequence; + // Try, every time, to read into our streambuf. In fact, we have no + // idea how much data the child might be trying to send: keep trying + // until we're convinced we've temporarily exhausted the pipe. + bool exhausted = false; + std::size_t committed(0); + do { - // 'gotten' was modified to reflect the number of bytes actually - // received. If nonzero, add them to the streambuf and notify - // interested parties. - if (gotten) + // attempt to read an arbitrary size + mutable_buffer_sequence bufs = mStreambuf.prepare(4096); + // In general, the mutable_buffer_sequence returned by prepare() might + // contain a number of different physical buffers; iterate over those. + std::size_t tocommit(0); + for (mutable_buffer_sequence::const_iterator bufi(bufs.begin()), bufend(bufs.end()); + bufi != bufend; ++bufi) { - boost::asio::streambuf::mutable_buffers_type mbufs = mStreambuf.prepare(gotten); - std::copy(buffer.begin(), buffer.begin() + gotten, - boost::asio::buffers_begin(mbufs)); - // Don't forget to "commit" the data! The sequence (prepare(), - // commit()) is obviously intended to allow us to allocate - // buffer space, then read directly into some portion of it, - // then commit only as much as we managed to obtain. But the - // only official (documented) way I can find to populate a - // mutable_buffers_type is to use buffers_begin(). It Would Be - // Nice if we were permitted to directly read into - // mutable_buffers_type (not to mention writing directly from - // const_buffers_type in WritePipeImpl; APR even supports an - // apr_file_writev() function for writing from discontiguous - // buffers) -- but as of 2012-02-14, this copying appears to - // be the safest tactic. - mStreambuf.commit(gotten); - LL_DEBUGS("LLProcess") << "read " << gotten << " of " << buffer.size() - << " bytes from " << mDesc << ", new total " - << mStreambuf.size() << LL_ENDL; - - // Now that we've received new data, publish it on our - // LLEventPump as advertised. Constrain it by mLimit. But show - // listener the actual accumulated buffer size, regardless of - // mLimit. - std::size_t datasize((std::min)(mLimit, mStreambuf.size())); - mPump.post(LLSDMap - ("data", peek(0, datasize)) - ("len", LLSD::Integer(mStreambuf.size()))); + // http://www.boost.org/doc/libs/1_49_0_beta1/doc/html/boost_asio/reference/buffer.html#boost_asio.reference.buffer.accessing_buffer_contents + std::size_t toread(boost::asio::buffer_size(*bufi)); + apr_size_t gotten(toread); + apr_status_t err = apr_file_read(mPipe, + boost::asio::buffer_cast(*bufi), + &gotten); + // EAGAIN is exactly what we want from a nonblocking pipe. + // Rather than waiting for data, it should return immediately. + if (! (err == APR_SUCCESS || APR_STATUS_IS_EAGAIN(err))) + { + // Handle EOF specially: it's part of normal-case processing. + if (err == APR_EOF) + { + LL_DEBUGS("LLProcess") << "EOF on " << mDesc << LL_ENDL; + } + else + { + LL_WARNS("LLProcess") << "apr_file_read(" << toread << ") on " << mDesc + << " got " << err << ":" << LL_ENDL; + ll_apr_warn_status(err); + } + // Either way, though, we won't need any more tick() calls. + mConnection.disconnect(); + exhausted = true; // also break outer retry loop + break; + } + + // 'gotten' was modified to reflect the number of bytes actually + // received. Make sure we commit those later. (Don't commit them + // now, that would invalidate the buffer iterator sequence!) + tocommit += gotten; + LL_DEBUGS("LLProcess") << "read " << gotten << " of " << toread + << " bytes from " << mDesc << LL_ENDL; + + // The parent end of this pipe is nonblocking. If we weren't even + // able to fill this buffer, don't loop to try to fill the next -- + // that won't change until the child writes more. Wait for next + // tick(). + if (gotten < toread) + { + // break outer retry loop too + exhausted = true; + break; + } } + + // Don't forget to "commit" the data! + mStreambuf.commit(tocommit); + committed += tocommit; + + // 'exhausted' is set when we can't fill any one buffer of the + // mutable_buffer_sequence established by the current prepare() + // call -- whether due to error or not enough bytes. That is, + // 'exhausted' is still false when we've filled every physical + // buffer in the mutable_buffer_sequence. In that case, for all we + // know, the child might have still more data pending -- go for it! + } while (! exhausted); + + if (committed) + { + // If we actually received new data, publish it on our LLEventPump + // as advertised. Constrain it by mLimit. But show listener the + // actual accumulated buffer size, regardless of mLimit. + size_type datasize((std::min)(mLimit, size_type(mStreambuf.size()))); + mPump.post(LLSDMap + ("data", peek(0, datasize)) + ("len", LLSD::Integer(mStreambuf.size()))); } + return false; } @@ -277,7 +374,7 @@ private: boost::asio::streambuf mStreambuf; std::istream mStream; LLEventStream mPump; - size_t mLimit; + size_type mLimit; }; /// Need an exception to avoid constructing an invalid LLProcess object, but @@ -472,16 +569,18 @@ LLProcess::LLProcess(const LLSDOrParams& params): { if (select[i] != APR_CHILD_BLOCK) continue; + std::string desc(STRINGIZE(mDesc << ' ' << whichfile[i])); + apr_file_t* pipe(mProcess.*(members[i])); if (i == STDIN) { - mPipes.replace(i, new WritePipeImpl(whichfile[i], mProcess.*(members[i]))); + mPipes.replace(i, new WritePipeImpl(desc, pipe)); } else { - mPipes.replace(i, new ReadPipeImpl(whichfile[i], mProcess.*(members[i]))); + mPipes.replace(i, new ReadPipeImpl(desc, pipe)); } LL_DEBUGS("LLProcess") << "Instantiating " << typeid(mPipes[i]).name() - << "('" << whichfile[i] << "')" << LL_ENDL; + << "('" << desc << "')" << LL_ENDL; } } diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index bf0517600d..2c6951b562 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -295,6 +295,9 @@ public: { public: virtual ~BasePipe() = 0; + + typedef std::size_t size_type; + static const size_type npos; }; /// As returned by getWritePipe() or getOptWritePipe() @@ -338,7 +341,7 @@ public: * the child, but the child happens to flush "12" before emitting * "3\n", get_istream() >> myint could return 12 rather than 123! */ - virtual std::size_t size() = 0; + virtual size_type size() const = 0; /** * Peek at accumulated buffer data without consuming it. Optional @@ -346,14 +349,32 @@ public: * * @note You can discard buffer data using get_istream().ignore(n). */ - virtual std::string peek(std::size_t offset=0, - std::size_t len=(std::numeric_limits::max)()) = 0; + virtual std::string peek(size_type offset=0, size_type len=npos) const = 0; + + /** + * Detect presence of a substring (or char) in accumulated buffer data + * without retrieving it. Optional offset allows you to search from + * specified position. + */ + template + bool contains(SEEK seek, size_type offset=0) const + { return find(seek, offset) != npos; } + + /** + * Search for a substring in accumulated buffer data without + * retrieving it. Returns size_type position at which found, or npos + * meaning not found. Optional offset allows you to search from + * specified position. + */ + virtual size_type find(const std::string& seek, size_type offset=0) const = 0; /** - * Search accumulated buffer data without retrieving it. Optional - * offset allows you to start at specified position. + * Search for a char in accumulated buffer data without retrieving it. + * Returns size_type position at which found, or npos meaning not + * found. Optional offset allows you to search from specified + * position. */ - virtual bool contains(const std::string& seek, std::size_t offset=0) = 0; + virtual size_type find(char seek, size_type offset=0) const = 0; /** * Get LLEventPump& on which to listen for incoming data. The posted @@ -377,12 +398,12 @@ public: * the data posted with the LLSD event. If you don't call this method, * all pending data will be posted. */ - virtual void setLimit(size_t limit) = 0; + virtual void setLimit(size_type limit) = 0; /** * Query the current setLimit() limit. */ - virtual size_t getLimit() const = 0; + virtual size_type getLimit() const = 0; }; /// Exception thrown by getWritePipe(), getReadPipe() if you didn't ask to diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 31bc833a1d..d7bda34923 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -1131,5 +1131,7 @@ namespace tut // test setLimit(), getLimit() // test EOF -- check logging // test peek() with substr + // test contains(char) + // test find(string, offset), find(char, offset), offset <, =, > size() } // namespace tut -- cgit v1.3 From 4ecf9d6a2d981ef27c7b5bddc4807c67d12d5984 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 Feb 2012 16:40:14 -0500 Subject: Add unit test for LLProcess::ReadPipe::setLimit(). --- indra/llcommon/tests/llprocess_test.cpp | 62 +++++++++++++++++++++++---------- 1 file changed, 44 insertions(+), 18 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index d7bda34923..29233d4415 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -140,11 +140,17 @@ struct PythonProcessLauncher mParams.args.add(mScript.getName()); } - /// Run Python script and wait for it to complete. - void run() + /// Launch Python script; verify that it launched + void launch() { mPy = LLProcess::create(mParams); tut::ensure(STRINGIZE("Couldn't launch " << mDesc << " script"), mPy); + } + + /// Run Python script and wait for it to complete. + void run() + { + launch(); // One of the irritating things about LLProcess is that // there's no API to wait for the child to terminate -- but given // its use in our graphics-intensive interactive viewer, it's @@ -718,8 +724,7 @@ namespace tut " f.write('bad')\n"); NamedTempFile out("out", "not started"); py.mParams.args.add(out.getName()); - py.mPy = LLProcess::create(py.mParams); - ensure("couldn't launch kill() script", py.mPy); + py.launch(); // Wait for the script to wake up and do its first write int i = 0, timeout = 60; for ( ; i < timeout; ++i) @@ -765,8 +770,7 @@ namespace tut "with open(sys.argv[1], 'w') as f:\n" " f.write('bad')\n"); py.mParams.args.add(out.getName()); - py.mPy = LLProcess::create(py.mParams); - ensure("couldn't launch kill() script", py.mPy); + py.launch(); // Capture handle for later phandle = py.mPy->getProcessHandle(); // Wait for the script to wake up and do its first write @@ -820,8 +824,7 @@ namespace tut py.mParams.args.add(from.getName()); py.mParams.args.add(to.getName()); py.mParams.autokill = false; - py.mPy = LLProcess::create(py.mParams); - ensure("couldn't launch kill() script", py.mPy); + py.launch(); // Capture handle for later phandle = py.mPy->getProcessHandle(); // Wait for the script to wake up and do its first write @@ -1017,8 +1020,7 @@ namespace tut "print 'ack'\n"); py.mParams.files.add(LLProcess::FileParam("pipe")); // stdin py.mParams.files.add(LLProcess::FileParam("pipe")); // stdout - py.mPy = LLProcess::create(py.mParams); - ensure("couldn't launch stdin/stdout script", py.mPy); + py.launch(); LLProcess::ReadPipe& childout(py.mPy->getReadPipe(LLProcess::STDOUT)); int i, timeout = 60; for (i = 0; i < timeout && py.mPy->isRunning() && childout.size() < 3; ++i) @@ -1082,8 +1084,7 @@ namespace tut "sys.stdout.write('second line\\n')\n"); py.mParams.files.add(LLProcess::FileParam("pipe")); // stdin py.mParams.files.add(LLProcess::FileParam("pipe")); // stdout - py.mPy = LLProcess::create(py.mParams); - ensure("couldn't launch ReadPipe listener script", py.mPy); + py.launch(); std::ostream& childin(py.mPy->getWritePipe(LLProcess::STDIN).get_ostream()); LLProcess::ReadPipe& childout(py.mPy->getReadPipe(LLProcess::STDOUT)); // listen for incoming data on childout @@ -1102,11 +1103,7 @@ namespace tut // disconnect from listener listener.mConnection.disconnect(); // finish out the run - for (i = 0; i < timeout && py.mPy->isRunning(); ++i) - { - yield(); - } - ensure("child took too long to terminate", i < timeout); + waitfor(*py.mPy); // now verify history std::list::const_iterator li(listener.mHistory.begin()), lend(listener.mHistory.end()); @@ -1127,8 +1124,37 @@ namespace tut ensure("more than 3 events", li == lend); } + template<> template<> + void object::test<18>() + { + set_test_name("setLimit()"); + PythonProcessLauncher py("setLimit()", + "import sys\n" + "print sys.argv[1]\n"); + std::string abc("abcdefghijklmnopqrstuvwxyz"); + py.mParams.args.add(abc); + py.mParams.files.add(LLProcess::FileParam()); // stdin + py.mParams.files.add(LLProcess::FileParam("pipe")); // stdout + py.launch(); + LLProcess::ReadPipe& childout(py.mPy->getReadPipe(LLProcess::STDOUT)); + // listen for incoming data on childout + EventListener listener(childout.getPump()); + // but set limit + childout.setLimit(10); + ensure_equals("getLimit() after setlimit(10)", childout.getLimit(), 10); + // okay, pump I/O to pick up output from child + waitfor(*py.mPy); + ensure("no events", ! listener.mHistory.empty()); + // For all we know, that data could have arrived in several different + // bursts... probably not, but anyway, only check the last one. + ensure_equals("event[\"len\"]", + listener.mHistory.back()["len"].asInteger(), + abc.length() + sizeof(EOL) - 1); + ensure_equals("length of setLimit(10) data", + listener.mHistory.back()["data"].asString().length(), 10); + } + // TODO: - // test setLimit(), getLimit() // test EOF -- check logging // test peek() with substr // test contains(char) -- cgit v1.3 From a06ba836c76ea8b35aeca9d09bd7d3b043a4c962 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 Feb 2012 17:35:34 -0500 Subject: Fix bug in LLProcess::ReadPipe::peek() substring computation. Add unit tests for peek() with substring args, reimplemented contains(), various forms of find(). (yay unit tests) --- indra/llcommon/llprocess.cpp | 3 +- indra/llcommon/tests/llprocess_test.cpp | 61 +++++++++++++++++++++++++++++---- 2 files changed, 57 insertions(+), 7 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index aa22b3f805..add1649ba5 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -230,7 +230,8 @@ public: { // Constrain caller's offset and len to overlap actual buffer content. std::size_t real_offset = (std::min)(mStreambuf.size(), std::size_t(offset)); - std::size_t real_end = (std::min)(mStreambuf.size(), std::size_t(real_offset + len)); + size_type want_end = (len == npos)? npos : (real_offset + len); + std::size_t real_end = (std::min)(mStreambuf.size(), std::size_t(want_end)); boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data(); return std::string(boost::asio::buffers_begin(cbufs) + real_offset, boost::asio::buffers_begin(cbufs) + real_end); diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 29233d4415..e5d873c8ee 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -1130,7 +1130,7 @@ namespace tut set_test_name("setLimit()"); PythonProcessLauncher py("setLimit()", "import sys\n" - "print sys.argv[1]\n"); + "sys.stdout.write(sys.argv[1])\n"); std::string abc("abcdefghijklmnopqrstuvwxyz"); py.mParams.args.add(abc); py.mParams.files.add(LLProcess::FileParam()); // stdin @@ -1148,16 +1148,65 @@ namespace tut // For all we know, that data could have arrived in several different // bursts... probably not, but anyway, only check the last one. ensure_equals("event[\"len\"]", - listener.mHistory.back()["len"].asInteger(), - abc.length() + sizeof(EOL) - 1); + listener.mHistory.back()["len"].asInteger(), abc.length()); ensure_equals("length of setLimit(10) data", listener.mHistory.back()["data"].asString().length(), 10); } + template<> template<> + void object::test<19>() + { + set_test_name("peek() ReadPipe data"); + PythonProcessLauncher py("peek() ReadPipe", + "import sys\n" + "sys.stdout.write(sys.argv[1])\n"); + std::string abc("abcdefghijklmnopqrstuvwxyz"); + py.mParams.args.add(abc); + py.mParams.files.add(LLProcess::FileParam()); // stdin + py.mParams.files.add(LLProcess::FileParam("pipe")); // stdout + py.launch(); + LLProcess::ReadPipe& childout(py.mPy->getReadPipe(LLProcess::STDOUT)); + // okay, pump I/O to pick up output from child + waitfor(*py.mPy); + // peek() with substr args + ensure_equals("peek()", childout.peek(), abc); + ensure_equals("peek(23)", childout.peek(23), abc.substr(23)); + ensure_equals("peek(5, 3)", childout.peek(5, 3), abc.substr(5, 3)); + ensure_equals("peek(27, 2)", childout.peek(27, 2), ""); + ensure_equals("peek(23, 5)", childout.peek(23, 5), "xyz"); + // contains() -- we don't exercise as thoroughly as find() because the + // contains() implementation is trivially (and visibly) based on find() + ensure("contains(\":\")", ! childout.contains(":")); + ensure("contains(':')", ! childout.contains(':')); + ensure("contains(\"d\")", childout.contains("d")); + ensure("contains('d')", childout.contains("d")); + ensure("contains(\"klm\")", childout.contains("klm")); + ensure("contains(\"klx\")", ! childout.contains("klx")); + // find() + ensure("find(\":\")", childout.find(":") == LLProcess::ReadPipe::npos); + ensure("find(':')", childout.find(':') == LLProcess::ReadPipe::npos); + ensure_equals("find(\"d\")", childout.find("d"), 3); + ensure_equals("find('d')", childout.find("d"), 3); + ensure_equals("find(\"d\", 3)", childout.find("d", 3), 3); + ensure_equals("find('d', 3)", childout.find("d", 3), 3); + ensure("find(\"d\", 4)", childout.find("d", 4) == LLProcess::ReadPipe::npos); + ensure("find('d', 4)", childout.find('d', 4) == LLProcess::ReadPipe::npos); + // The case of offset == end and offset > end are different. In the + // first case, we can form a valid (albeit empty) iterator range and + // search that. In the second, guard logic in the implementation must + // realize we can't form a valid iterator range. + ensure("find(\"d\", 26)", childout.find("d", 26) == LLProcess::ReadPipe::npos); + ensure("find('d', 26)", childout.find('d', 26) == LLProcess::ReadPipe::npos); + ensure("find(\"d\", 27)", childout.find("d", 27) == LLProcess::ReadPipe::npos); + ensure("find('d', 27)", childout.find('d', 27) == LLProcess::ReadPipe::npos); + ensure_equals("find(\"ghi\")", childout.find("ghi"), 6); + ensure_equals("find(\"ghi\", 6)", childout.find("ghi"), 6); + ensure("find(\"ghi\", 7)", childout.find("ghi", 7) == LLProcess::ReadPipe::npos); + ensure("find(\"ghi\", 26)", childout.find("ghi", 26) == LLProcess::ReadPipe::npos); + ensure("find(\"ghi\", 27)", childout.find("ghi", 27) == LLProcess::ReadPipe::npos); + } + // TODO: // test EOF -- check logging - // test peek() with substr - // test contains(char) - // test find(string, offset), find(char, offset), offset <, =, > size() } // namespace tut -- cgit v1.3 From f52cf4be7003f18813da31b25d204f10eb36db17 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 Feb 2012 21:10:06 -0500 Subject: Fix typos in a few LLProcess::ReadPipe::find() unit tests. The typos didn't make for invalid tests, but they made a few tests redundant while leaving other (subtly different) cases untested. --- indra/llcommon/tests/llprocess_test.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index e5d873c8ee..c67605cc0b 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -1179,16 +1179,16 @@ namespace tut ensure("contains(\":\")", ! childout.contains(":")); ensure("contains(':')", ! childout.contains(':')); ensure("contains(\"d\")", childout.contains("d")); - ensure("contains('d')", childout.contains("d")); + ensure("contains('d')", childout.contains('d')); ensure("contains(\"klm\")", childout.contains("klm")); ensure("contains(\"klx\")", ! childout.contains("klx")); // find() ensure("find(\":\")", childout.find(":") == LLProcess::ReadPipe::npos); ensure("find(':')", childout.find(':') == LLProcess::ReadPipe::npos); ensure_equals("find(\"d\")", childout.find("d"), 3); - ensure_equals("find('d')", childout.find("d"), 3); + ensure_equals("find('d')", childout.find('d'), 3); ensure_equals("find(\"d\", 3)", childout.find("d", 3), 3); - ensure_equals("find('d', 3)", childout.find("d", 3), 3); + ensure_equals("find('d', 3)", childout.find('d', 3), 3); ensure("find(\"d\", 4)", childout.find("d", 4) == LLProcess::ReadPipe::npos); ensure("find('d', 4)", childout.find('d', 4) == LLProcess::ReadPipe::npos); // The case of offset == end and offset > end are different. In the -- cgit v1.3 From 8b5d5f9652499103b966524e1c0ceef869e29eeb Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 20 Feb 2012 12:40:38 -0500 Subject: 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. --- indra/llcommon/llprocess.cpp | 118 ++++++++++++++++++++------------ indra/llcommon/llprocess.h | 18 ++++- indra/llcommon/tests/llprocess_test.cpp | 59 ++++++++++++++++ 3 files changed, 151 insertions(+), 44 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') 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 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(&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(&mPipes[slot]); + if (! ppipe) + { + error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a " << typeid(PIPETYPE).name()); + return NULL; + } + + error.clear(); + return ppipe; } template PIPETYPE& LLProcess::getPipe(FILESLOT slot) { - std::string error; - PIPETYPE* wp = getPipePtr(error, slot); - if (! wp) - { - throw NoPipe(error); - } - return *wp; + std::string error; + PIPETYPE* wp = getPipePtr(error, slot); + if (! wp) + { + throw NoPipe(error); + } + return *wp; } template boost::optional LLProcess::getOptPipe(FILESLOT slot) { - std::string error; - PIPETYPE* wp = getPipePtr(error, slot); - if (! wp) - { - LL_DEBUGS("LLProcess") << error << LL_ENDL; - return boost::optional(); - } - return *wp; + std::string error; + PIPETYPE* wp = getPipePtr(error, slot); + if (! wp) + { + LL_DEBUGS("LLProcess") << error << LL_ENDL; + return boost::optional(); + } + return *wp; } LLProcess::WritePipe& LLProcess::getWritePipe(FILESLOT slot) { - return getPipe(slot); + return getPipe(slot); } boost::optional LLProcess::getOptWritePipe(FILESLOT slot) { - return getOptPipe(slot); + return getOptPipe(slot); } LLProcess::ReadPipe& LLProcess::getReadPipe(FILESLOT slot) { - return getPipe(slot); + return getPipe(slot); } boost::optional LLProcess::getOptReadPipe(FILESLOT slot) { - return getOptPipe(slot); + return getOptPipe(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 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 postend; }; typedef LLSDParamAdapter 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 -- cgit v1.3 From 999484a60896b11df1af9a44e58ccae6fa6ecbed Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 20 Feb 2012 14:22:32 -0500 Subject: Let LLProcess consumer specify desired description for logging. If caller runs (e.g.) a Python script, it's not very helpful to a human log reader to keep seeing LLProcess instances logged as /pathname/to/python (pid). If caller is aware, the code can at least use the script name as the desc -- or maybe even a hint as to the script's purpose. If caller doesn't explicitly pass a desc, at least shorten to just the basename of the executable. --- indra/llcommon/llprocess.cpp | 30 +++++++++++++++++++++++++++--- indra/llcommon/llprocess.h | 10 +++++++++- indra/llcommon/tests/llprocess_test.cpp | 8 +++----- 3 files changed, 39 insertions(+), 9 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 9799ed1938..b4c6a647d7 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -50,6 +50,7 @@ static const char* whichfile[] = { "stdin", "stdout", "stderr" }; static std::string empty; static LLProcess::Status interpret_status(int status); +static std::string getDesc(const LLProcess::Params& params); /** * Ref-counted "mainloop" listener. As long as there are still outstanding @@ -404,7 +405,7 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params) LLEventPumps::instance().obtain(params.postend) .post(LLSDMap // no "id" - ("desc", std::string(params.executable)) + ("desc", getDesc(params)) ("state", LLProcess::UNSTARTED) // no "data" ("string", e.what()) @@ -561,8 +562,8 @@ LLProcess::LLProcess(const LLSDOrParams& params): sProcessListener.addPoll(*this); mStatus.mState = RUNNING; - mDesc = STRINGIZE(LLStringUtil::quote(params.executable) << " (" << mProcess.pid << ')'); - LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcess.pid << ")" << LL_ENDL; + mDesc = STRINGIZE(getDesc(params) << " (" << mProcess.pid << ')'); + LL_INFOS("LLProcess") << mDesc << ": launched " << params << LL_ENDL; // Unless caller explicitly turned off autokill (child should persist), // take steps to terminate the child. This is all suspenders-and-belt: in @@ -604,6 +605,29 @@ LLProcess::LLProcess(const LLSDOrParams& params): } } +// Helper to obtain a description string, given a Params block +static std::string getDesc(const LLProcess::Params& params) +{ + // If caller specified a description string, by all means use it. + std::string desc(params.desc); + if (! desc.empty()) + return desc; + + // Caller didn't say. Use the executable name -- but use just the filename + // part. On Mac, for instance, full pathnames get cumbersome. + // If there are Linden utility functions to manipulate pathnames, I + // haven't found them -- and for this usage, Boost.Filesystem seems kind + // of heavyweight. + std::string executable(params.executable); + std::string::size_type delim = executable.find_last_of("\\/"); + // If executable contains no pathname delimiters, return the whole thing. + if (delim == std::string::npos) + return executable; + + // Return just the part beyond the last delimiter. + return executable.substr(delim + 1); +} + LLProcess::~LLProcess() { // Only in state RUNNING are we registered for callback. In UNSTARTED we diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 96a3dce5b3..d005847e18 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -159,7 +159,8 @@ public: cwd("cwd"), autokill("autokill", true), files("files"), - postend("postend") + postend("postend"), + desc("desc") {} /// pathname of executable @@ -199,6 +200,13 @@ public: * with code 0") */ Optional postend; + /** + * Description of child process for logging purposes. It need not be + * unique; the logged description string will contain the PID as well. + * If this is omitted, a description will be derived from the + * executable name. + */ + Optional desc; }; typedef LLSDParamAdapter LLSDOrParams; diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 1a755c283c..fe599e7892 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -136,6 +136,7 @@ struct PythonProcessLauncher const char* PYTHON(getenv("PYTHON")); tut::ensure("Set $PYTHON to the Python interpreter", PYTHON); + mParams.desc = desc + " script"; mParams.executable = PYTHON; mParams.args.add(mScript.getName()); } @@ -1244,17 +1245,14 @@ namespace tut std::string pumpname("postend"); EventListener listener(LLEventPumps::instance().obtain(pumpname)); LLProcess::Params params; + params.desc = "bad postend"; 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("desc", postend["desc"].asString(), std::string(params.desc)); ensure_equals("state", postend["state"].asInteger(), LLProcess::UNSTARTED); ensure("has data", ! postend.has("data")); std::string error(postend["string"]); -- cgit v1.3 From 14ddc6474a0ae83db8d034b00138289fb15e41b7 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 23 Feb 2012 13:41:26 -0500 Subject: Tighten up LLProcess pipe support, per Richard's code review. Clarify wording in some of the doc comments; be a bit more explicit about some of the parameter fields. Make some query methods 'const'. Change default LLProcess::ReadPipe::getLimit() value to 0: don't post any incoming data with notification event unless caller requests it. But do post pertinent FILESLOT in case caller reuses same listener for both stdout and stderr. Use more idiomatic, readable syntax for accessing LLProcess::Params data. --- indra/llcommon/llprocess.cpp | 119 +++++++++++++++++++------------- indra/llcommon/llprocess.h | 63 +++++++++++------ indra/llcommon/tests/llprocess_test.cpp | 4 +- 3 files changed, 116 insertions(+), 70 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index b4c6a647d7..3b17b819bd 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -47,11 +47,18 @@ #include #include -static const char* whichfile[] = { "stdin", "stdout", "stderr" }; +static const char* whichfile_[] = { "stdin", "stdout", "stderr" }; static std::string empty; static LLProcess::Status interpret_status(int status); static std::string getDesc(const LLProcess::Params& params); +static std::string whichfile(LLProcess::FILESLOT index) +{ + if (index < LL_ARRAY_SIZE(whichfile_)) + return whichfile_[index]; + return STRINGIZE("file slot " << index); +} + /** * Ref-counted "mainloop" listener. As long as there are still outstanding * LLProcess objects, keep listening on "mainloop" so we can keep polling APR @@ -122,6 +129,7 @@ static LLProcessListener sProcessListener; LLProcess::BasePipe::~BasePipe() {} const LLProcess::BasePipe::size_type + // use funky syntax to call max() to avoid blighted max() macros LLProcess::BasePipe::npos((std::numeric_limits::max)()); class WritePipeImpl: public LLProcess::WritePipe @@ -205,14 +213,14 @@ class ReadPipeImpl: public LLProcess::ReadPipe { LOG_CLASS(ReadPipeImpl); public: - ReadPipeImpl(const std::string& desc, apr_file_t* pipe): + ReadPipeImpl(const std::string& desc, apr_file_t* pipe, LLProcess::FILESLOT index): mDesc(desc), mPipe(pipe), + mIndex(index), // Essential to initialize our std::istream with our special streambuf! mStream(&mStreambuf), mPump("ReadPipe", true), // tweak name as needed to avoid collisions - // use funky syntax to call max() to avoid blighted max() macros - mLimit(npos) + mLimit(0) { mConnection = LLEventPumps::instance().obtain("mainloop") .listen(LLEventPump::inventName("ReadPipe"), @@ -364,7 +372,10 @@ private: size_type datasize((std::min)(mLimit, size_type(mStreambuf.size()))); mPump.post(LLSDMap ("data", peek(0, datasize)) - ("len", LLSD::Integer(mStreambuf.size()))); + ("len", LLSD::Integer(mStreambuf.size())) + ("index", LLSD::Integer(mIndex)) + ("name", whichfile(mIndex)) + ("desc", mDesc)); } return false; @@ -372,6 +383,7 @@ private: std::string mDesc; apr_file_t* mPipe; + LLProcess::FILESLOT mIndex; LLTempBoundListener mConnection; boost::asio::streambuf mStreambuf; std::istream mStream; @@ -400,7 +412,7 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params) // 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()) + if (params.postend.isProvided()) { LLEventPumps::instance().obtain(params.postend) .post(LLSDMap @@ -458,22 +470,24 @@ LLProcess::LLProcess(const LLSDOrParams& params): // and passing it as both stdout and stderr (apr_procattr_child_out_set(), // apr_procattr_child_err_set()), or accepting a filename, opening it and // passing that apr_file_t (simple <, >, 2> redirect emulation). - std::vector fparams(params.files.begin(), params.files.end()); - // By default, pass APR_NO_PIPE for each slot. - std::vector select(LL_ARRAY_SIZE(whichfile), APR_NO_PIPE); - for (size_t i = 0; i < (std::min)(LL_ARRAY_SIZE(whichfile), fparams.size()); ++i) + std::vector select; + BOOST_FOREACH(const FileParam& fparam, params.files) { - if (std::string(fparams[i].type).empty()) // inherit our file descriptor + // Every iteration, we're going to append an item to 'select'. At the + // top of the loop, its size() is, in effect, an index. Use that to + // pick a string description for messages. + std::string which(whichfile(FILESLOT(select.size()))); + if (fparam.type().empty()) // inherit our file descriptor { - select[i] = APR_NO_PIPE; + select.push_back(APR_NO_PIPE); } - else if (std::string(fparams[i].type) == "pipe") // anonymous pipe + else if (fparam.type() == "pipe") // anonymous pipe { - if (! std::string(fparams[i].name).empty()) + if (! fparam.name().empty()) { - LL_WARNS("LLProcess") << "For " << std::string(params.executable) + LL_WARNS("LLProcess") << "For " << params.executable() << ": internal names for reusing pipes ('" - << std::string(fparams[i].name) << "' for " << whichfile[i] + << fparam.name() << "' for " << which << ") are not yet supported -- creating distinct pipe" << LL_ENDL; } @@ -482,16 +496,21 @@ LLProcess::LLProcess(const LLSDOrParams& params): // makes very little sense to set nonblocking I/O for the child // end of a pipe: only a specially-written child could deal with // that. - select[i] = APR_CHILD_BLOCK; + select.push_back(APR_CHILD_BLOCK); } else { - throw LLProcessError(STRINGIZE("For " << std::string(params.executable) - << ": unsupported FileParam for " << whichfile[i] - << ": type='" << std::string(fparams[i].type) - << "', name='" << std::string(fparams[i].name) << "'")); + throw LLProcessError(STRINGIZE("For " << params.executable() + << ": unsupported FileParam for " << which + << ": type='" << fparam.type() + << "', name='" << fparam.name() << "'")); } } + // By default, pass APR_NO_PIPE for unspecified slots. + while (select.size() < NSLOTS) + { + select.push_back(APR_NO_PIPE); + } chkapr(apr_procattr_io_set(procattr, select[STDIN], select[STDOUT], select[STDERR])); // Thumbs down on implicitly invoking the shell to invoke the child. From @@ -527,24 +546,32 @@ LLProcess::LLProcess(const LLSDOrParams& params): #endif } - // Have to instantiate named std::strings for string params items so their - // c_str() values persist. - std::string cwd(params.cwd); - if (! cwd.empty()) + // In preparation for calling apr_proc_create(), we collect a number of + // const char* pointers obtained from std::string::c_str(). Turns out + // LLInitParam::Block's helpers Optional, Mandatory, Multiple et al. + // guarantee that converting to the wrapped type (std::string in our + // case), e.g. by calling operator(), returns a reference to *the same + // instance* of the wrapped type that's stored in our Block subclass. + // That's important! We know 'params' persists throughout this method + // call; but without that guarantee, we'd have to assume that converting + // one of its members to std::string might return a different (temp) + // instance. Capturing the c_str() from a temporary std::string is Bad Bad + // Bad. But armed with this knowledge, when you see params.cwd().c_str(), + // grit your teeth and smile and carry on. + + if (params.cwd.isProvided()) { - chkapr(apr_procattr_dir_set(procattr, cwd.c_str())); + chkapr(apr_procattr_dir_set(procattr, params.cwd().c_str())); } // create an argv vector for the child process std::vector argv; - // add the executable path - std::string executable(params.executable); - argv.push_back(executable.c_str()); + // Add the executable path. See above remarks about c_str(). + argv.push_back(params.executable().c_str()); - // and any arguments - std::vector args(params.args.begin(), params.args.end()); - BOOST_FOREACH(const std::string& arg, args) + // Add arguments. See above remarks about c_str(). + BOOST_FOREACH(const std::string& arg, params.args) { argv.push_back(arg.c_str()); } @@ -590,7 +617,7 @@ LLProcess::LLProcess(const LLSDOrParams& params): { if (select[i] != APR_CHILD_BLOCK) continue; - std::string desc(STRINGIZE(mDesc << ' ' << whichfile[i])); + std::string desc(STRINGIZE(mDesc << ' ' << whichfile(FILESLOT(i)))); apr_file_t* pipe(mProcess.*(members[i])); if (i == STDIN) { @@ -598,7 +625,7 @@ LLProcess::LLProcess(const LLSDOrParams& params): } else { - mPipes.replace(i, new ReadPipeImpl(desc, pipe)); + mPipes.replace(i, new ReadPipeImpl(desc, pipe, FILESLOT(i))); } LL_DEBUGS("LLProcess") << "Instantiating " << typeid(mPipes[i]).name() << "('" << desc << "')" << LL_ENDL; @@ -609,9 +636,8 @@ LLProcess::LLProcess(const LLSDOrParams& params): static std::string getDesc(const LLProcess::Params& params) { // If caller specified a description string, by all means use it. - std::string desc(params.desc); - if (! desc.empty()) - return desc; + if (params.desc.isProvided()) + return params.desc; // Caller didn't say. Use the executable name -- but use just the filename // part. On Mac, for instance, full pathnames get cumbersome. @@ -670,22 +696,22 @@ bool LLProcess::kill(const std::string& who) return ! isRunning(); } -bool LLProcess::isRunning(void) +bool LLProcess::isRunning() const { return getStatus().mState == RUNNING; } -LLProcess::Status LLProcess::getStatus() +LLProcess::Status LLProcess::getStatus() const { return mStatus; } -std::string LLProcess::getStatusString() +std::string LLProcess::getStatusString() const { return getStatusString(getStatus()); } -std::string LLProcess::getStatusString(const Status& status) +std::string LLProcess::getStatusString(const Status& status) const { return getStatusString(mDesc, status); } @@ -816,7 +842,7 @@ LLProcess::handle LLProcess::getProcessHandle() const #endif } -std::string LLProcess::getPipeName(FILESLOT) +std::string LLProcess::getPipeName(FILESLOT) const { // LLProcess::FileParam::type "npipe" is not yet implemented return ""; @@ -832,7 +858,7 @@ PIPETYPE* LLProcess::getPipePtr(std::string& error, FILESLOT slot) } if (mPipes.is_null(slot)) { - error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a monitored pipe"); + 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 @@ -840,7 +866,7 @@ PIPETYPE* LLProcess::getPipePtr(std::string& error, FILESLOT slot) PIPETYPE* ppipe = dynamic_cast(&mPipes[slot]); if (! ppipe) { - error = STRINGIZE(mDesc << ' ' << whichfile[slot] << " not a " << typeid(PIPETYPE).name()); + error = STRINGIZE(mDesc << ' ' << whichfile(slot) << " not a " << typeid(PIPETYPE).name()); return NULL; } @@ -895,10 +921,9 @@ boost::optional LLProcess::getOptReadPipe(FILESLOT slot) std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params) { - std::string cwd(params.cwd); - if (! cwd.empty()) + if (params.cwd.isProvided()) { - out << "cd " << LLStringUtil::quote(cwd) << ": "; + out << "cd " << LLStringUtil::quote(params.cwd) << ": "; } out << LLStringUtil::quote(params.executable); BOOST_FOREACH(const std::string& arg, params.args) diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index d005847e18..637b7e2f9c 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -58,12 +58,16 @@ typedef boost::shared_ptr LLProcessPtr; * arguments. It also keeps track of whether the process is still running, and * can kill it if required. * + * In discussing LLProcess, we use the term "parent" to refer to this process + * (the process invoking LLProcess), versus "child" to refer to the process + * spawned by LLProcess. + * * 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. + * LLProcess object's Status won't update until the next "mainloop" tick. For + * instance, the Second Life viewer's main loop already posts to an + * LLEventPump by that name once per iteration. 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 { @@ -110,10 +114,10 @@ public: * pumping nonblocking I/O every frame. The expectation (at least * for stdout or stderr) is that the caller will listen for * incoming data and consume it as it arrives. It's important not - * to neglect such a pipe, because it's buffered in viewer memory. - * If you suspect the child may produce a great volume of output - * between viewer frames, consider directing the child to write to - * a filesystem file instead, then read the file later. + * to neglect such a pipe, because it's buffered in memory. If you + * suspect the child may produce a great volume of output between + * frames, consider directing the child to write to a filesystem + * file instead, then read the file later. * * - "tpipe": do not engage LLProcess machinery to monitor the * parent end of the pipe. A "tpipe" is used only to connect @@ -145,9 +149,14 @@ public: Optional name; FileParam(const std::string& tp="", const std::string& nm=""): - type("type", tp), - name("name", nm) - {} + type("type"), + name("name") + { + // If caller wants to specify values, use explicit assignment to + // set them rather than initialization. + if (! tp.empty()) type = tp; + if (! nm.empty()) name = nm; + } }; /// Param block definition @@ -175,17 +184,28 @@ public: /// current working directory, if need it changed Optional cwd; /// implicitly kill process on destruction of LLProcess object + /// (default true) Optional autokill; /** * Up to three FileParam items: for child stdin, stdout, stderr. * Passing two FileParam entries means default treatment for stderr, * and so forth. * + * @note LLInitParam::Block permits usage like this: + * @code + * LLProcess::Params params; + * ... + * params.files + * .add(LLProcess::FileParam()) // stdin + * .add(LLProcess::FileParam().type("pipe") // stdout + * .add(LLProcess::FileParam().type("file").name("error.log")); + * @endcode + * * @note While it's theoretically plausible to pass additional open * file handles to a child specifically written to expect them, our - * underlying implementation library doesn't support that. + * underlying implementation doesn't yet support that. */ - Multiple files; + Multiple > files; /** * On child-process termination, if this LLProcess object still * exists, post LLSD event to LLEventPump with specified name (default @@ -217,9 +237,8 @@ public: static LLProcessPtr create(const LLSDOrParams& params); virtual ~LLProcess(); - // isRunning() isn't const because, when child terminates, it sets stored - // Status - bool isRunning(void); + /// Is child process still running? + bool isRunning() const; /** * State of child process @@ -252,11 +271,11 @@ public: }; /// Status query - Status getStatus(); + Status getStatus() const; /// English Status string query, for logging etc. - std::string getStatusString(); + std::string getStatusString() const; /// English Status string query for previously-captured Status - std::string getStatusString(const Status& status); + std::string getStatusString(const Status& status) const; /// static English Status string query static std::string getStatusString(const std::string& desc, const Status& status); @@ -311,7 +330,7 @@ public: * filesystem name for the specified pipe. Otherwise returns the empty * string. @see LLProcess::FileParam::type */ - std::string getPipeName(FILESLOT); + std::string getPipeName(FILESLOT) const; /// base of ReadPipe, WritePipe class LL_COMMON_API BasePipe @@ -419,7 +438,7 @@ public: * contain "data"="abcde". However, you may still read the entire * "abcdef" from get_istream(): this limit affects only the size of * the data posted with the LLSD event. If you don't call this method, - * all pending data will be posted. + * @em no data will be posted: the default is 0 bytes. */ virtual void setLimit(size_type limit) = 0; diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index fe599e7892..d7feddd26b 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -696,7 +696,7 @@ namespace tut "syntax_error:\n"); py.mParams.files.add(LLProcess::FileParam()); // inherit stdin py.mParams.files.add(LLProcess::FileParam()); // inherit stdout - py.mParams.files.add(LLProcess::FileParam("pipe")); // pipe for stderr + py.mParams.files.add(LLProcess::FileParam().type("pipe")); // pipe for stderr py.run(); ensure_equals("Status.mState", py.mPy->getStatus().mState, LLProcess::EXITED); ensure_equals("Status.mData", py.mPy->getStatus().mData, 1); @@ -1088,6 +1088,8 @@ namespace tut py.launch(); std::ostream& childin(py.mPy->getWritePipe(LLProcess::STDIN).get_ostream()); LLProcess::ReadPipe& childout(py.mPy->getReadPipe(LLProcess::STDOUT)); + // lift the default limit; allow event to carry (some of) the actual data + childout.setLimit(20); // listen for incoming data on childout EventListener listener(childout.getPump()); // also listen with a function that prompts the child to continue -- cgit v1.3 From 7fd281ac9971caa1dfffd42e6ff16dd44da20179 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 27 Feb 2012 15:24:14 -0500 Subject: Reduce redundancy in llprocess_test.cpp using get_test_name(). --- indra/llcommon/tests/llprocess_test.cpp | 46 ++++++++++++++++----------------- 1 file changed, 23 insertions(+), 23 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index d7feddd26b..b02a5c0631 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -620,7 +620,7 @@ namespace tut // guaranteed to exist on every machine, under every OS? Have to // create one. Naturally, ensure we clean it up when done. NamedTempDir tempdir; - PythonProcessLauncher py("getcwd()", + PythonProcessLauncher py(get_test_name(), "from __future__ import with_statement\n" "import os, sys\n" "with open(sys.argv[1], 'w') as f:\n" @@ -634,7 +634,7 @@ namespace tut void object::test<3>() { set_test_name("arguments"); - PythonProcessLauncher py("args", + PythonProcessLauncher py(get_test_name(), "from __future__ import with_statement\n" "import sys\n" // note nonstandard output-file arg! @@ -668,7 +668,7 @@ namespace tut void object::test<4>() { set_test_name("exit(0)"); - PythonProcessLauncher py("exit(0)", + PythonProcessLauncher py(get_test_name(), "import sys\n" "sys.exit(0)\n"); py.run(); @@ -680,7 +680,7 @@ namespace tut void object::test<5>() { set_test_name("exit(2)"); - PythonProcessLauncher py("exit(2)", + PythonProcessLauncher py(get_test_name(), "import sys\n" "sys.exit(2)\n"); py.run(); @@ -692,7 +692,7 @@ namespace tut void object::test<6>() { set_test_name("syntax_error:"); - PythonProcessLauncher py("syntax_error:", + PythonProcessLauncher py(get_test_name(), "syntax_error:\n"); py.mParams.files.add(LLProcess::FileParam()); // inherit stdin py.mParams.files.add(LLProcess::FileParam()); // inherit stdout @@ -713,7 +713,7 @@ namespace tut void object::test<7>() { set_test_name("explicit kill()"); - PythonProcessLauncher py("kill()", + PythonProcessLauncher py(get_test_name(), "from __future__ import with_statement\n" "import sys, time\n" "with open(sys.argv[1], 'w') as f:\n" @@ -750,7 +750,7 @@ namespace tut // 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. - ensure_equals("kill() script output", readfile(out.getName()), "ok"); + ensure_equals(get_test_name() + " script output", readfile(out.getName()), "ok"); } template<> template<> @@ -760,7 +760,7 @@ namespace tut NamedTempFile out("out", "not started"); LLProcess::handle phandle(0); { - PythonProcessLauncher py("kill()", + PythonProcessLauncher py(get_test_name(), "from __future__ import with_statement\n" "import sys, time\n" "with open(sys.argv[1], 'w') as f:\n" @@ -792,7 +792,7 @@ namespace tut // 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. - ensure_equals("kill() script output", readfile(out.getName()), "ok"); + ensure_equals(get_test_name() + " script output", readfile(out.getName()), "ok"); } template<> template<> @@ -803,7 +803,7 @@ namespace tut NamedTempFile to("to", ""); LLProcess::handle phandle(0); { - PythonProcessLauncher py("autokill", + PythonProcessLauncher py(get_test_name(), "from __future__ import with_statement\n" "import sys, time\n" "with open(sys.argv[1], 'w') as f:\n" @@ -852,7 +852,7 @@ namespace tut 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"); + ensure_equals(get_test_name() + " script output", readfile(from.getName()), "ack"); } template<> template<> @@ -860,7 +860,7 @@ namespace tut { set_test_name("'bogus' test"); TestRecorder recorder; - PythonProcessLauncher py("'bogus' test", + PythonProcessLauncher py(get_test_name(), "print 'Hello world'\n"); py.mParams.files.add(LLProcess::FileParam("bogus")); py.mPy = LLProcess::create(py.mParams); @@ -876,7 +876,7 @@ namespace tut set_test_name("'file' test"); // Replace this test with one or more real 'file' tests when we // implement 'file' support - PythonProcessLauncher py("'file' test", + PythonProcessLauncher py(get_test_name(), "print 'Hello world'\n"); py.mParams.files.add(LLProcess::FileParam()); py.mParams.files.add(LLProcess::FileParam("file")); @@ -891,7 +891,7 @@ namespace tut // Replace this test with one or more real 'tpipe' tests when we // implement 'tpipe' support TestRecorder recorder; - PythonProcessLauncher py("'tpipe' test", + PythonProcessLauncher py(get_test_name(), "print 'Hello world'\n"); py.mParams.files.add(LLProcess::FileParam()); py.mParams.files.add(LLProcess::FileParam("tpipe")); @@ -909,7 +909,7 @@ namespace tut // Replace this test with one or more real 'npipe' tests when we // implement 'npipe' support TestRecorder recorder; - PythonProcessLauncher py("'npipe' test", + PythonProcessLauncher py(get_test_name(), "print 'Hello world'\n"); py.mParams.files.add(LLProcess::FileParam()); py.mParams.files.add(LLProcess::FileParam()); @@ -926,7 +926,7 @@ namespace tut { set_test_name("internal pipe name warning"); TestRecorder recorder; - PythonProcessLauncher py("pipe warning", + PythonProcessLauncher py(get_test_name(), "import sys\n" "sys.exit(7)\n"); py.mParams.files.add(LLProcess::FileParam("pipe", "somename")); @@ -990,7 +990,7 @@ namespace tut void object::test<15>() { set_test_name("get*Pipe() validation"); - PythonProcessLauncher py("just stderr", + PythonProcessLauncher py(get_test_name(), "print 'this output is expected'\n"); py.mParams.files.add(LLProcess::FileParam("pipe")); // pipe for stdin py.mParams.files.add(LLProcess::FileParam()); // inherit stdout @@ -1010,7 +1010,7 @@ namespace tut void object::test<16>() { set_test_name("talk to stdin/stdout"); - PythonProcessLauncher py("stdin/stdout", + PythonProcessLauncher py(get_test_name(), "import sys, time\n" "print 'ok'\n" "sys.stdout.flush()\n" @@ -1071,7 +1071,7 @@ namespace tut void object::test<17>() { set_test_name("listen for ReadPipe events"); - PythonProcessLauncher py("ReadPipe listener", + PythonProcessLauncher py(get_test_name(), "import sys\n" "sys.stdout.write('abc')\n" "sys.stdout.flush()\n" @@ -1131,7 +1131,7 @@ namespace tut void object::test<18>() { set_test_name("setLimit()"); - PythonProcessLauncher py("setLimit()", + PythonProcessLauncher py(get_test_name(), "import sys\n" "sys.stdout.write(sys.argv[1])\n"); std::string abc("abcdefghijklmnopqrstuvwxyz"); @@ -1160,7 +1160,7 @@ namespace tut void object::test<19>() { set_test_name("peek() ReadPipe data"); - PythonProcessLauncher py("peek() ReadPipe", + PythonProcessLauncher py(get_test_name(), "import sys\n" "sys.stdout.write(sys.argv[1])\n"); std::string abc("abcdefghijklmnopqrstuvwxyz"); @@ -1213,7 +1213,7 @@ namespace tut void object::test<20>() { set_test_name("good postend"); - PythonProcessLauncher py("postend", + PythonProcessLauncher py(get_test_name(), "import sys\n" "sys.exit(35)\n"); std::string pumpname("postend"); @@ -1247,7 +1247,7 @@ namespace tut std::string pumpname("postend"); EventListener listener(LLEventPumps::instance().obtain(pumpname)); LLProcess::Params params; - params.desc = "bad postend"; + params.desc = get_test_name(); params.postend = pumpname; LLProcessPtr child = LLProcess::create(params); ensure("shouldn't have launched", ! child); -- cgit v1.3 From 3649eda62ad3a04203e6c562e78815a95896bbd4 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 29 Feb 2012 17:10:19 -0500 Subject: Guarantee LLProcess::Params::postend listener any ReadPipe data. Previously one might get process-terminated notification but still have to wait for the child process's final data to arrive on one or more ReadPipes. That required complex consumer timing logic to handle incomplete pending ReadPipe data, e.g. a partial last line with no terminating newline. New code guarantees that by the time LLProcess sends process-terminated notification, all pending pipe data will have been buffered in ReadPipes. Document LLProcess::ReadPipe::getPump() notification event; add "eof" key. Add LLProcess::ReadPipe::getline() and read() convenience methods. Add static LLProcess::getline() and basename() convenience methods, publishing logic already present elsewhere. Use ReadPipe::getline() and read() in unit tests. Add unit test for "eof" event on ReadPipe::getPump(). Add unit test verifying that final data have been buffered by termination notification event. --- indra/llcommon/llprocess.cpp | 107 +++++++++++++++++++----- indra/llcommon/llprocess.h | 31 ++++++- indra/llcommon/tests/llprocess_test.cpp | 139 ++++++++++++++++++++++---------- 3 files changed, 210 insertions(+), 67 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 3b17b819bd..edfdebfe87 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -220,7 +220,8 @@ public: // Essential to initialize our std::istream with our special streambuf! mStream(&mStreambuf), mPump("ReadPipe", true), // tweak name as needed to avoid collisions - mLimit(0) + mLimit(0), + mEOF(false) { mConnection = LLEventPumps::instance().obtain("mainloop") .listen(LLEventPump::inventName("ReadPipe"), @@ -230,11 +231,25 @@ public: // Much of the implementation is simply connecting the abstract virtual // methods with implementation data concealed from the base class. virtual std::istream& get_istream() { return mStream; } + virtual std::string getline() { return LLProcess::getline(mStream); } virtual LLEventPump& getPump() { return mPump; } virtual void setLimit(size_type limit) { mLimit = limit; } virtual size_type getLimit() const { return mLimit; } virtual size_type size() const { return mStreambuf.size(); } + virtual std::string read(size_type len) + { + // Read specified number of bytes into a buffer. Make a buffer big + // enough. + size_type readlen((std::min)(size(), len)); + std::vector buffer(readlen); + mStream.read(&buffer[0], readlen); + // Since we've already clamped 'readlen', we can think of no reason + // why mStream.read() should read fewer than 'readlen' bytes. + // Nonetheless, use the actual retrieved length. + return std::string(&buffer[0], mStream.gcount()); + } + virtual std::string peek(size_type offset=0, size_type len=npos) const { // Constrain caller's offset and len to overlap actual buffer content. @@ -287,14 +302,18 @@ public: return (found == end)? npos : (found - begin); } -private: bool tick(const LLSD&) { + // Once we've hit EOF, skip all the rest of this. + if (mEOF) + return false; + typedef boost::asio::streambuf::mutable_buffers_type mutable_buffer_sequence; // Try, every time, to read into our streambuf. In fact, we have no // idea how much data the child might be trying to send: keep trying // until we're convinced we've temporarily exhausted the pipe. - bool exhausted = false; + enum PipeState { RETRY, EXHAUSTED, CLOSED }; + PipeState state = RETRY; std::size_t committed(0); do { @@ -329,7 +348,9 @@ private: } // Either way, though, we won't need any more tick() calls. mConnection.disconnect(); - exhausted = true; // also break outer retry loop + // Ignore any subsequent calls we might get anyway. + mEOF = true; + state = CLOSED; // also break outer retry loop break; } @@ -347,7 +368,7 @@ private: if (gotten < toread) { // break outer retry loop too - exhausted = true; + state = EXHAUSTED; break; } } @@ -356,15 +377,20 @@ private: mStreambuf.commit(tocommit); committed += tocommit; - // 'exhausted' is set when we can't fill any one buffer of the - // mutable_buffer_sequence established by the current prepare() - // call -- whether due to error or not enough bytes. That is, - // 'exhausted' is still false when we've filled every physical + // state is changed from RETRY when we can't fill any one buffer + // of the mutable_buffer_sequence established by the current + // prepare() call -- whether due to error or not enough bytes. + // That is, if state is still RETRY, we've filled every physical // buffer in the mutable_buffer_sequence. In that case, for all we // know, the child might have still more data pending -- go for it! - } while (! exhausted); - - if (committed) + } while (state == RETRY); + + // Once we recognize that the pipe is closed, make one more call to + // listener. The listener might be waiting for a particular substring + // to arrive, or a particular length of data or something. The event + // with "eof" == true announces that nothing further will arrive, so + // use it or lose it. + if (committed || state == CLOSED) { // If we actually received new data, publish it on our LLEventPump // as advertised. Constrain it by mLimit. But show listener the @@ -373,14 +399,16 @@ private: mPump.post(LLSDMap ("data", peek(0, datasize)) ("len", LLSD::Integer(mStreambuf.size())) - ("index", LLSD::Integer(mIndex)) + ("slot", LLSD::Integer(mIndex)) ("name", whichfile(mIndex)) - ("desc", mDesc)); + ("desc", mDesc) + ("eof", state == CLOSED)); } return false; } +private: std::string mDesc; apr_file_t* mPipe; LLProcess::FILESLOT mIndex; @@ -389,6 +417,7 @@ private: std::istream mStream; LLEventStream mPump; size_type mLimit; + bool mEOF; }; /// Need an exception to avoid constructing an invalid LLProcess object, but @@ -641,17 +670,22 @@ static std::string getDesc(const LLProcess::Params& params) // Caller didn't say. Use the executable name -- but use just the filename // part. On Mac, for instance, full pathnames get cumbersome. + return LLProcess::basename(params.executable); +} + +//static +std::string LLProcess::basename(const std::string& path) +{ // If there are Linden utility functions to manipulate pathnames, I // haven't found them -- and for this usage, Boost.Filesystem seems kind // of heavyweight. - std::string executable(params.executable); - std::string::size_type delim = executable.find_last_of("\\/"); - // If executable contains no pathname delimiters, return the whole thing. + std::string::size_type delim = path.find_last_of("\\/"); + // If path contains no pathname delimiters, return the whole thing. if (delim == std::string::npos) - return executable; + return path; // Return just the part beyond the last delimiter. - return executable.substr(delim + 1); + return path.substr(delim + 1); } LLProcess::~LLProcess() @@ -804,6 +838,24 @@ void LLProcess::handle_status(int reason, int status) // KILLED; refine below. mStatus.mState = EXITED; + // Make last-gasp calls for each of the ReadPipes we have on hand. Since + // they're listening on "mainloop", we can be sure they'll eventually + // collect all pending data from the child. But we want to be able to + // guarantee to our consumer that by the time we post on the "postend" + // LLEventPump, our ReadPipes are already buffering all the data there + // will ever be from the child. That lets the "postend" listener decide + // what to do with that final data. + for (size_t i = 0; i < mPipes.size(); ++i) + { + std::string error; + ReadPipeImpl* ppipe = getPipePtr(error, FILESLOT(i)); + if (ppipe) + { + static LLSD trivial; + ppipe->tick(trivial); + } + } + // wi->rv = apr_proc_wait(wi->child, &wi->rc, &wi->why, APR_NOWAIT); // It's just wrong to call apr_proc_wait() here. The only way APR knows to // call us with APR_OC_REASON_DEATH is that it's already reaped this child @@ -919,6 +971,23 @@ boost::optional LLProcess::getOptReadPipe(FILESLOT slot) return getOptPipe(slot); } +//static +std::string LLProcess::getline(std::istream& in) +{ + std::string line; + std::getline(in, line); + // Blur the distinction between "\r\n" and plain "\n". std::getline() will + // have eaten the "\n", but we could still end up with a trailing "\r". + std::string::size_type lastpos = line.find_last_not_of("\r"); + if (lastpos != std::string::npos) + { + // Found at least one character that's not a trailing '\r'. SKIP OVER + // IT and erase the rest of the line. + line.erase(lastpos+1); + } + return line; +} + std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params) { if (params.cwd.isProvided()) diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 637b7e2f9c..06ada83698 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -156,7 +156,7 @@ public: // set them rather than initialization. if (! tp.empty()) type = tp; if (! nm.empty()) name = nm; - } + } }; /// Param block definition @@ -375,6 +375,18 @@ public: */ virtual std::istream& get_istream() = 0; + /** + * Like std::getline(get_istream(), line), but trims off trailing '\r' + * to make calling code less platform-sensitive. + */ + virtual std::string getline() = 0; + + /** + * Like get_istream().read(buffer, n), but returns std::string rather + * than requiring caller to construct a buffer, etc. + */ + virtual std::string read(size_type len) = 0; + /** * Get accumulated buffer length. * Often we need to refrain from actually reading the std::istream @@ -420,9 +432,15 @@ public: /** * Get LLEventPump& on which to listen for incoming data. The posted - * LLSD::Map event will contain a key "data" whose value is an - * LLSD::String containing (part of) the data accumulated in the - * buffer. + * LLSD::Map event will contain: + * + * - "data" part of pending data; see setLimit() + * - "len" entire length of pending data, regardless of setLimit() + * - "slot" this ReadPipe's FILESLOT, e.g. LLProcess::STDOUT + * - "name" e.g. "stdout" + * - "desc" e.g. "SLPlugin (pid) stdout" + * - "eof" @c true means there no more data will arrive on this pipe, + * therefore no more events on this pump * * If the child sends "abc", and this ReadPipe posts "data"="abc", but * you don't consume it by reading the std::istream returned by @@ -487,6 +505,11 @@ public: */ boost::optional getOptReadPipe(FILESLOT slot); + /// little utilities that really should already be somewhere else in the + /// code base + static std::string basename(const std::string& path); + static std::string getline(std::istream&); + private: /// constructor is private: use create() instead LLProcess(const LLSDOrParams& params); diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index b02a5c0631..3537133a47 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -295,22 +295,6 @@ public: LLError::Settings* mOldSettings; }; -std::string getline(std::istream& in) -{ - std::string line; - std::getline(in, line); - // Blur the distinction between "\r\n" and plain "\n". std::getline() will - // have eaten the "\n", but we could still end up with a trailing "\r". - std::string::size_type lastpos = line.find_last_not_of("\r"); - if (lastpos != std::string::npos) - { - // Found at least one character that's not a trailing '\r'. SKIP OVER - // IT and then erase the rest of the line. - line.erase(lastpos+1); - } - return line; -} - /***************************************************************************** * TUT *****************************************************************************/ @@ -1030,7 +1014,7 @@ namespace tut } ensure("script never started", i < timeout); ensure_equals("bad wakeup from stdin/stdout script", - getline(childout.get_istream()), "ok"); + childout.getline(), "ok"); // important to get the implicit flush from std::endl py.mPy->getWritePipe().get_ostream() << "go" << std::endl; for (i = 0; i < timeout && py.mPy->isRunning() && ! childout.contains("\n"); ++i) @@ -1038,7 +1022,7 @@ namespace tut yield(); } ensure("script never replied", childout.contains("\n")); - ensure_equals("child didn't ack", getline(childout.get_istream()), "ack"); + ensure_equals("child didn't ack", childout.getline(), "ack"); ensure_equals("bad child termination", py.mPy->getStatus().mState, LLProcess::EXITED); ensure_equals("bad child exit code", py.mPy->getStatus().mData, 0); } @@ -1129,6 +1113,32 @@ namespace tut template<> template<> void object::test<18>() + { + set_test_name("ReadPipe \"eof\" event"); + PythonProcessLauncher py(get_test_name(), + "print 'Hello from Python!'\n"); + py.mParams.files.add(LLProcess::FileParam()); // stdin + py.mParams.files.add(LLProcess::FileParam("pipe")); // stdout + py.launch(); + LLProcess::ReadPipe& childout(py.mPy->getReadPipe(LLProcess::STDOUT)); + EventListener listener(childout.getPump()); + waitfor(*py.mPy); + // We can't be positive there will only be a single event, if the OS + // (or any other intervening layer) does crazy buffering. What we want + // to ensure is that there was exactly ONE event with "eof" true, and + // that it was the LAST event. + std::list::const_reverse_iterator rli(listener.mHistory.rbegin()), + rlend(listener.mHistory.rend()); + ensure("no events", rli != rlend); + ensure("last event not \"eof\"", (*rli)["eof"].asBoolean()); + while (++rli != rlend) + { + ensure("\"eof\" event not last", ! (*rli)["eof"].asBoolean()); + } + } + + template<> template<> + void object::test<19>() { set_test_name("setLimit()"); PythonProcessLauncher py(get_test_name(), @@ -1157,7 +1167,7 @@ namespace tut } template<> template<> - void object::test<19>() + void object::test<20>() { set_test_name("peek() ReadPipe data"); PythonProcessLauncher py(get_test_name(), @@ -1210,7 +1220,32 @@ namespace tut } template<> template<> - void object::test<20>() + void object::test<21>() + { + set_test_name("bad postend"); + std::string pumpname("postend"); + EventListener listener(LLEventPumps::instance().obtain(pumpname)); + LLProcess::Params params; + params.desc = get_test_name(); + 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")); + ensure_equals("desc", postend["desc"].asString(), std::string(params.desc)); + 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()); + } + + template<> template<> + void object::test<22>() { set_test_name("good postend"); PythonProcessLauncher py(get_test_name(), @@ -1240,32 +1275,48 @@ namespace tut ensure_contains("string", str, "35"); } + struct PostendListener + { + PostendListener(LLProcess::ReadPipe& rpipe, + const std::string& pumpname, + const std::string& expect): + mReadPipe(rpipe), + mExpect(expect), + mTriggered(false) + { + LLEventPumps::instance().obtain(pumpname) + .listen("PostendListener", boost::bind(&PostendListener::postend, this, _1)); + } + + bool postend(const LLSD&) + { + mTriggered = true; + ensure_equals("postend listener", mReadPipe.read(mReadPipe.size()), mExpect); + return false; + } + + LLProcess::ReadPipe& mReadPipe; + std::string mExpect; + bool mTriggered; + }; + template<> template<> - void object::test<21>() + void object::test<23>() { - set_test_name("bad postend"); + set_test_name("all data visible at postend"); + PythonProcessLauncher py(get_test_name(), + "import sys\n" + // note, no '\n' in written data + "sys.stdout.write('partial line')\n"); std::string pumpname("postend"); - EventListener listener(LLEventPumps::instance().obtain(pumpname)); - LLProcess::Params params; - params.desc = get_test_name(); - 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")); - ensure_equals("desc", postend["desc"].asString(), std::string(params.desc)); - 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()); + py.mParams.files.add(LLProcess::FileParam()); // stdin + py.mParams.files.add(LLProcess::FileParam("pipe")); // stdout + py.mParams.postend = pumpname; + py.launch(); + PostendListener listener(py.mPy->getReadPipe(LLProcess::STDOUT), + pumpname, + "partial line"); + waitfor(*py.mPy); + ensure("postend never triggered", listener.mTriggered); } - - // TODO: - // test EOF -- check logging - } // namespace tut -- cgit v1.3 From 2596816f316e13b717bcdacddad0da48c90c3b6d Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 1 Mar 2012 13:43:52 -0500 Subject: Break out TestRecorder class as CaptureLog into wrapllerrs.h. Giving more unit tests the ability to capture and examine log output is generally useful. Renaming the class just makes it less ambiguous: what's a TestRecorder? Something that records tests? --- indra/llcommon/tests/llprocess_test.cpp | 74 +++------------------------------ indra/llcommon/tests/wrapllerrs.h | 65 +++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 68 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 3537133a47..c07a9d3925 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -35,7 +35,7 @@ #include "stringize.h" #include "llsdutil.h" #include "llevents.h" -#include "llerrorcontrol.h" +#include "wrapllerrs.h" #if defined(LL_WINDOWS) #define sleep(secs) _sleep((secs) * 1000) @@ -233,68 +233,6 @@ private: std::string mPath; }; -// statically reference the function in test.cpp... it's short, we could -// replicate, but better to reuse -extern void wouldHaveCrashed(const std::string& message); - -/** - * Capture log messages. This is adapted (simplified) from the one in - * llerror_test.cpp. Sigh, should've broken that out into a separate header - * file, but time for this project is short... - */ -class TestRecorder : public LLError::Recorder -{ -public: - TestRecorder(): - // Mostly what we're trying to accomplish by saving and resetting - // LLError::Settings is to bypass the default RecordToStderr and - // RecordToWinDebug Recorders. As these are visible only inside - // llerror.cpp, we can't just call LLError::removeRecorder() with - // each. For certain tests we need to produce, capture and examine - // DEBUG log messages -- but we don't want to spam the user's console - // with that output. If it turns out that saveAndResetSettings() has - // some bad effect, give up and just let the DEBUG level log messages - // display. - mOldSettings(LLError::saveAndResetSettings()) - { - LLError::setFatalFunction(wouldHaveCrashed); - LLError::setDefaultLevel(LLError::LEVEL_DEBUG); - LLError::addRecorder(this); - } - - ~TestRecorder() - { - LLError::removeRecorder(this); - LLError::restoreSettings(mOldSettings); - } - - void recordMessage(LLError::ELevel level, - const std::string& message) - { - mMessages.push_back(message); - } - - /// Don't assume the message we want is necessarily the LAST log message - /// emitted by the underlying code; search backwards through all messages - /// for the sought string. - std::string messageWith(const std::string& search) - { - for (std::list::const_reverse_iterator rmi(mMessages.rbegin()), - rmend(mMessages.rend()); - rmi != rmend; ++rmi) - { - if (rmi->find(search) != std::string::npos) - return *rmi; - } - // failed to find any such message - return std::string(); - } - - typedef std::list MessageList; - MessageList mMessages; - LLError::Settings* mOldSettings; -}; - /***************************************************************************** * TUT *****************************************************************************/ @@ -843,7 +781,7 @@ namespace tut void object::test<10>() { set_test_name("'bogus' test"); - TestRecorder recorder; + CaptureLog recorder; PythonProcessLauncher py(get_test_name(), "print 'Hello world'\n"); py.mParams.files.add(LLProcess::FileParam("bogus")); @@ -874,7 +812,7 @@ namespace tut set_test_name("'tpipe' test"); // Replace this test with one or more real 'tpipe' tests when we // implement 'tpipe' support - TestRecorder recorder; + CaptureLog recorder; PythonProcessLauncher py(get_test_name(), "print 'Hello world'\n"); py.mParams.files.add(LLProcess::FileParam()); @@ -892,7 +830,7 @@ namespace tut set_test_name("'npipe' test"); // Replace this test with one or more real 'npipe' tests when we // implement 'npipe' support - TestRecorder recorder; + CaptureLog recorder; PythonProcessLauncher py(get_test_name(), "print 'Hello world'\n"); py.mParams.files.add(LLProcess::FileParam()); @@ -909,7 +847,7 @@ namespace tut void object::test<14>() { set_test_name("internal pipe name warning"); - TestRecorder recorder; + CaptureLog recorder; PythonProcessLauncher py(get_test_name(), "import sys\n" "sys.exit(7)\n"); @@ -965,7 +903,7 @@ namespace tut #define EXPECT_FAIL_WITH_LOG(EXPECT, CODE) \ do \ { \ - TestRecorder recorder; \ + CaptureLog recorder; \ ensure(#CODE " succeeded", ! (CODE)); \ ensure("wrong log message", ! recorder.messageWith(EXPECT).empty()); \ } while (0) diff --git a/indra/llcommon/tests/wrapllerrs.h b/indra/llcommon/tests/wrapllerrs.h index ffda84729b..a61f8451b3 100644 --- a/indra/llcommon/tests/wrapllerrs.h +++ b/indra/llcommon/tests/wrapllerrs.h @@ -30,6 +30,14 @@ #define LL_WRAPLLERRS_H #include "llerrorcontrol.h" +#include +#include +#include +#include + +// statically reference the function in test.cpp... it's short, we could +// replicate, but better to reuse +extern void wouldHaveCrashed(const std::string& message); struct WrapLL_ERRS { @@ -70,4 +78,61 @@ struct WrapLL_ERRS LLError::FatalFunction mPriorFatal; }; +/** + * Capture log messages. This is adapted (simplified) from the one in + * llerror_test.cpp. + */ +class CaptureLog : public LLError::Recorder +{ +public: + CaptureLog(): + // Mostly what we're trying to accomplish by saving and resetting + // LLError::Settings is to bypass the default RecordToStderr and + // RecordToWinDebug Recorders. As these are visible only inside + // llerror.cpp, we can't just call LLError::removeRecorder() with + // each. For certain tests we need to produce, capture and examine + // DEBUG log messages -- but we don't want to spam the user's console + // with that output. If it turns out that saveAndResetSettings() has + // some bad effect, give up and just let the DEBUG level log messages + // display. + mOldSettings(LLError::saveAndResetSettings()) + { + LLError::setFatalFunction(wouldHaveCrashed); + LLError::setDefaultLevel(LLError::LEVEL_DEBUG); + LLError::addRecorder(this); + } + + ~CaptureLog() + { + LLError::removeRecorder(this); + LLError::restoreSettings(mOldSettings); + } + + void recordMessage(LLError::ELevel level, + const std::string& message) + { + mMessages.push_back(message); + } + + /// Don't assume the message we want is necessarily the LAST log message + /// emitted by the underlying code; search backwards through all messages + /// for the sought string. + std::string messageWith(const std::string& search) + { + for (std::list::const_reverse_iterator rmi(mMessages.rbegin()), + rmend(mMessages.rend()); + rmi != rmend; ++rmi) + { + if (rmi->find(search) != std::string::npos) + return *rmi; + } + // failed to find any such message + return std::string(); + } + + typedef std::list MessageList; + MessageList mMessages; + LLError::Settings* mOldSettings; +}; + #endif /* ! defined(LL_WRAPLLERRS_H) */ -- cgit v1.3 From f904867720291bc63ea5e140194069d29f70fb40 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 1 Mar 2012 14:33:23 -0500 Subject: Make CaptureLog::withMessage() raise tut::failure if not found. All known callers were using ensure(! withMessage(...).empty()). Centralize that logic. Make failure message report the string being sought and the log messages in which it wasn't found. In case someone does want to permit the search to fail, add an optional 'required' parameter, default true. Leverage new functionality in llprocess_test.cpp. --- indra/llcommon/tests/llprocess_test.cpp | 6 +----- indra/llcommon/tests/wrapllerrs.h | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 9 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index c07a9d3925..6103764f24 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -788,7 +788,6 @@ namespace tut py.mPy = LLProcess::create(py.mParams); ensure("should have rejected 'bogus'", ! py.mPy); std::string message(recorder.messageWith("bogus")); - ensure("did not log 'bogus' type", ! message.empty()); ensure_contains("did not name 'stdin'", message, "stdin"); } @@ -820,7 +819,6 @@ namespace tut py.mPy = LLProcess::create(py.mParams); ensure("should have rejected 'tpipe'", ! py.mPy); std::string message(recorder.messageWith("tpipe")); - ensure("did not log 'tpipe' type", ! message.empty()); ensure_contains("did not name 'stdout'", message, "stdout"); } @@ -839,7 +837,6 @@ namespace tut py.mPy = LLProcess::create(py.mParams); ensure("should have rejected 'npipe'", ! py.mPy); std::string message(recorder.messageWith("npipe")); - ensure("did not log 'npipe' type", ! message.empty()); ensure_contains("did not name 'stderr'", message, "stderr"); } @@ -856,7 +853,6 @@ namespace tut ensure_equals("Status.mState", py.mPy->getStatus().mState, LLProcess::EXITED); ensure_equals("Status.mData", py.mPy->getStatus().mData, 7); std::string message(recorder.messageWith("not yet supported")); - ensure("did not log pipe name warning", ! message.empty()); ensure_contains("log message did not mention internal pipe name", message, "somename"); } @@ -905,7 +901,7 @@ namespace tut { \ CaptureLog recorder; \ ensure(#CODE " succeeded", ! (CODE)); \ - ensure("wrong log message", ! recorder.messageWith(EXPECT).empty()); \ + recorder.messageWith(EXPECT); \ } while (0) template<> template<> diff --git a/indra/llcommon/tests/wrapllerrs.h b/indra/llcommon/tests/wrapllerrs.h index a61f8451b3..28ffbf517f 100644 --- a/indra/llcommon/tests/wrapllerrs.h +++ b/indra/llcommon/tests/wrapllerrs.h @@ -29,11 +29,13 @@ #if ! defined(LL_WRAPLLERRS_H) #define LL_WRAPLLERRS_H +#include #include "llerrorcontrol.h" #include #include #include #include +#include // statically reference the function in test.cpp... it's short, we could // replicate, but better to reuse @@ -117,17 +119,26 @@ public: /// Don't assume the message we want is necessarily the LAST log message /// emitted by the underlying code; search backwards through all messages /// for the sought string. - std::string messageWith(const std::string& search) + std::string messageWith(const std::string& search, bool required=true) { - for (std::list::const_reverse_iterator rmi(mMessages.rbegin()), - rmend(mMessages.rend()); + for (MessageList::const_reverse_iterator rmi(mMessages.rbegin()), rmend(mMessages.rend()); rmi != rmend; ++rmi) { if (rmi->find(search) != std::string::npos) return *rmi; } // failed to find any such message - return std::string(); + if (! required) + return std::string(); + + std::ostringstream out; + out << "failed to find '" << search << "' in captured log messages:"; + for (MessageList::const_iterator mi(mMessages.begin()), mend(mMessages.end()); + mi != mend; ++mi) + { + out << '\n' << *mi; + } + throw tut::failure(out.str()); } typedef std::list MessageList; -- cgit v1.3 From 7d3cf544c7962ae6cc8e0e8b8e8db817e366a9cc Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 13 Mar 2012 14:15:11 -0400 Subject: Add timeout functionality to waitfor() helper functions. Otherwise, a stuck child process could potentially hang the test, and thus the whole viewer build. --- indra/llcommon/tests/llprocess_test.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) (limited to 'indra/llcommon/tests/llprocess_test.cpp') diff --git a/indra/llcommon/tests/llprocess_test.cpp b/indra/llcommon/tests/llprocess_test.cpp index 6103764f24..99186ed434 100644 --- a/indra/llcommon/tests/llprocess_test.cpp +++ b/indra/llcommon/tests/llprocess_test.cpp @@ -101,20 +101,26 @@ void yield(int seconds=1) LLEventPumps::instance().obtain("mainloop").post(LLSD()); } -void waitfor(LLProcess& proc) +void waitfor(LLProcess& proc, int timeout=60) { - while (proc.isRunning()) + int i = 0; + for ( ; i < timeout && proc.isRunning(); ++i) { yield(); } + tut::ensure(STRINGIZE("process took longer than " << timeout << " seconds to terminate"), + i < timeout); } -void waitfor(LLProcess::handle h, const std::string& desc) +void waitfor(LLProcess::handle h, const std::string& desc, int timeout=60) { - while (LLProcess::isRunning(h, desc)) + int i = 0; + for ( ; i < timeout && LLProcess::isRunning(h, desc); ++i) { yield(); } + tut::ensure(STRINGIZE("process took longer than " << timeout << " seconds to terminate"), + i < timeout); } /** -- cgit v1.3