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/llprocess.h | 106 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 indra/llcommon/llprocess.h (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h new file mode 100644 index 0000000000..9a74cfe829 --- /dev/null +++ b/indra/llcommon/llprocess.h @@ -0,0 +1,106 @@ +/** + * @file llprocess.h + * @brief Utility class for launching, terminating, and tracking child processes. + * + * $LicenseInfo:firstyear=2008&license=viewerlgpl$ + * Second Life Viewer Source Code + * Copyright (C) 2010, Linden Research, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; + * version 2.1 of the License only. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * + * Linden Research, Inc., 945 Battery Street, San Francisco, CA 94111 USA + * $/LicenseInfo$ + */ + +#ifndef LL_LLPROCESS_H +#define LL_LLPROCESS_H + +#include +#include + +#if LL_WINDOWS +#define WIN32_LEAN_AND_MEAN +#include +#endif + +class LLSD; + +class LLProcess; +/// LLProcess instances are created on the heap by static factory methods and +/// managed by ref-counted pointers. +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. +*/ +class LL_COMMON_API LLProcess: public boost::noncopyable +{ + LOG_CLASS(LLProcess); +public: + + /** + * Factory accepting LLSD::Map. + * MAY RETURN DEFAULT-CONSTRUCTED LLProcessPtr if params invalid! + * + * 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 LLSD& params); + virtual ~LLProcess(); + + // isRunning isn't const because, if child isn't running, it clears stored + // process ID + bool isRunning(void); + + // 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); + +#if LL_WINDOWS + typedef HANDLE id; +#else + typedef pid_t id; +#endif + /// Get platform-specific process ID + id getProcessID() const { return mProcessID; }; + + /** + * Test if a process (id obtained from getProcessID()) is still + * running. Return is same nonzero id 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); + * @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. + */ + static id isRunning(id); + +private: + /// constructor is private: use create() instead + LLProcess(const LLSD& params); + void launch(const LLSD& params); + + id mProcessID; + bool mAutokill; +}; + +#endif // LL_LLPROCESS_H -- cgit v1.3 From 6e214960ce203d1d50d7bd6bd04eedee3afd0fa3 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 20 Jan 2012 20:19:50 -0500 Subject: Define LLProcess::Params; accept create(const LLSDParamAdapter&). This allows callers to pass either LLSD formatted as before -- which all callers still do -- or an actual LLProcess::Params block. --- indra/llcommon/llprocess.cpp | 31 +++++++++++++------------------ indra/llcommon/llprocess.h | 33 +++++++++++++++++++++++++++------ 2 files changed, 40 insertions(+), 24 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 8c0caca680..dfb2ed69e9 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -26,7 +26,6 @@ #include "linden_common.h" #include "llprocess.h" -#include "llsd.h" #include "llsdserialize.h" #include "stringize.h" @@ -41,7 +40,7 @@ struct LLProcessError: public std::runtime_error LLProcessError(const std::string& msg): std::runtime_error(msg) {} }; -LLProcessPtr LLProcess::create(const LLSD& params) +LLProcessPtr LLProcess::create(const LLSDParamAdapter& params) { try { @@ -54,16 +53,13 @@ LLProcessPtr LLProcess::create(const LLSD& params) } } -LLProcess::LLProcess(const LLSD& params): +LLProcess::LLProcess(const LLSDParamAdapter& params): mProcessID(0), - mAutokill(params["autokill"].asBoolean()) + mAutokill(params.autokill) { - // nonstandard default bool value - if (! params.has("autokill")) - mAutokill = true; - if (! params.has("executable")) + if (! params.validateBlock(true)) { - throw LLProcessError(STRINGIZE("not launched: missing 'executable'\n" + throw LLProcessError(STRINGIZE("not launched: failed parameter validation\n" << LLSDNotationStreamer(params))); } @@ -108,14 +104,14 @@ static std::string quote(const std::string& str) return result + "\""; } -void LLProcess::launch(const LLSD& params) +void LLProcess::launch(const LLSDParamAdapter& params) { PROCESS_INFORMATION pinfo; STARTUPINFOA sinfo; memset(&sinfo, 0, sizeof(sinfo)); - std::string args = quote(params["executable"]); - BOOST_FOREACH(const std::string& arg, llsd::inArray(params["args"])) + std::string args = quote(params.executable); + BOOST_FOREACH(const std::string& arg, params.args) { args += " "; args += quote(arg); @@ -128,7 +124,7 @@ void LLProcess::launch(const LLSD& params) // 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"]); + std::string cwd(params.cwd); const char * working_directory = 0; if (! cwd.empty()) working_directory = cwd.c_str(); @@ -212,7 +208,7 @@ static bool reap_pid(pid_t pid) return false; } -void LLProcess::launch(const LLSD& params) +void LLProcess::launch(const LLSDParamAdapter& params) { // flush all buffers before the child inherits them ::fflush(NULL); @@ -222,7 +218,7 @@ void LLProcess::launch(const LLSD& params) { // child process - std::string cwd(params["cwd"]); + std::string cwd(params.cwd); if (! cwd.empty()) { // change to the desired child working directory @@ -239,12 +235,11 @@ void LLProcess::launch(const LLSD& params) std::vector fake_argv; // add the executable path - std::string executable(params["executable"]); + std::string executable(params.executable); fake_argv.push_back(executable.c_str()); // and any arguments - const LLSD& params_args(params["args"]); - std::vector args(params_args.beginArray(), params_args.endArray()); + std::vector args(params.args.begin(), params.args.end()); BOOST_FOREACH(const std::string& arg, args) { fake_argv.push_back(arg.c_str()); diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 9a74cfe829..9ea129baf2 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -27,6 +27,8 @@ #ifndef LL_LLPROCESS_H #define LL_LLPROCESS_H +#include "llinitparam.h" +#include "llsdparam.h" #include #include @@ -35,8 +37,6 @@ #include #endif -class LLSD; - class LLProcess; /// LLProcess instances are created on the heap by static factory methods and /// managed by ref-counted pointers. @@ -50,17 +50,38 @@ class LL_COMMON_API LLProcess: public boost::noncopyable { LOG_CLASS(LLProcess); public: + /// Param block definition + struct Params: public LLInitParam::Block + { + Params(): + executable("executable"), + args("args"), + cwd("cwd"), + autokill("autokill", true) + {} + + /// pathname of executable + Mandatory executable; + /// zero or more additional command-line arguments + Multiple args; + /// current working directory, if need it changed + Optional cwd; + /// implicitly kill process on destruction of LLProcess object + Optional autokill; + }; /** - * Factory accepting LLSD::Map. + * 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 LLSD& params); + static LLProcessPtr create(const LLSDParamAdapter& params); virtual ~LLProcess(); // isRunning isn't const because, if child isn't running, it clears stored @@ -96,8 +117,8 @@ public: private: /// constructor is private: use create() instead - LLProcess(const LLSD& params); - void launch(const LLSD& params); + LLProcess(const LLSDParamAdapter& params); + void launch(const LLSDParamAdapter& params); id mProcessID; bool mAutokill; -- cgit v1.3 From aa1bbe3277842a9a6e7db5227b35f1fbea50b7a6 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sun, 22 Jan 2012 10:58:16 -0500 Subject: Make LLProcess::Params streamable; use that in LLExternalEditor. --- indra/llcommon/llprocess.cpp | 15 +++++++++++++++ indra/llcommon/llprocess.h | 4 ++++ indra/newview/llexternaleditor.cpp | 14 ++------------ 3 files changed, 21 insertions(+), 12 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index d30d87411d..9d6c19f1dd 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -81,6 +81,21 @@ bool LLProcess::isRunning(void) return (mProcessID != 0); } +std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params) +{ + std::string cwd(params.cwd); + if (! cwd.empty()) + { + out << "cd '" << cwd << "': "; + } + out << '"' << std::string(params.executable) << '"'; + BOOST_FOREACH(const std::string& arg, params.args) + { + out << " \"" << arg << '"'; + } + return out; +} + /***************************************************************************** * Windows specific *****************************************************************************/ diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 9ea129baf2..7dbdf23679 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -31,6 +31,7 @@ #include "llsdparam.h" #include #include +#include // std::ostream #if LL_WINDOWS #define WIN32_LEAN_AND_MEAN @@ -124,4 +125,7 @@ private: bool mAutokill; }; +/// for logging +LL_COMMON_API std::ostream& operator<<(std::ostream&, const LLProcess::Params&); + #endif // LL_LLPROCESS_H diff --git a/indra/newview/llexternaleditor.cpp b/indra/newview/llexternaleditor.cpp index 3dfebad958..0d3ed0ba35 100644 --- a/indra/newview/llexternaleditor.cpp +++ b/indra/newview/llexternaleditor.cpp @@ -74,12 +74,7 @@ LLExternalEditor::EErrorCode LLExternalEditor::setCommand(const std::string& env llinfos << "Adding the filename marker (" << sFilenameMarker << ")" << llendl; } - llinfos << "Setting command [" << bin_path; - BOOST_FOREACH(const std::string& arg, mProcessParams.args) - { - llcont << " \"" << arg << "\""; - } - llcont << "]" << llendl; + llinfos << "Setting command [" << mProcessParams << "]" << llendl; return EC_SUCCESS; } @@ -108,12 +103,7 @@ LLExternalEditor::EErrorCode LLExternalEditor::run(const std::string& file_path) } // Run the editor. - llinfos << "Running editor command [" << std::string(params.executable); - BOOST_FOREACH(const std::string& arg, params.args) - { - llcont << " \"" << arg << "\""; - } - llcont << "]" << llendl; + llinfos << "Running editor command [" << params << "]" << llendl; // Prevent killing the process in destructor. params.autokill = false; return LLProcess::create(params) ? EC_SUCCESS : EC_FAILED_TO_RUN; -- cgit v1.3 From 748d1b311fdecf123df40bd7d22dd7e19afaca84 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Sun, 22 Jan 2012 11:56:38 -0500 Subject: Add LLProcess logging on launch(), kill(), isRunning(). Much as I dislike viewer log spam, seems to me starting a child process, killing it and observing its termination are noteworthy events. New logging makes LLExternalEditor launch message redundant; removed. --- indra/llcommon/llprocess.cpp | 34 +++++++++++++++++++++++++--------- indra/llcommon/llprocess.h | 7 ++++--- indra/newview/llexternaleditor.cpp | 4 +--- 3 files changed, 30 insertions(+), 15 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 9d6c19f1dd..6d329a3fa1 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -77,7 +77,7 @@ LLProcess::~LLProcess() bool LLProcess::isRunning(void) { - mProcessID = isRunning(mProcessID); + mProcessID = isRunning(mProcessID, mDesc); return (mProcessID != 0); } @@ -190,20 +190,23 @@ 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; + CloseHandle(pinfo.hThread); // stops leaks - nothing else + + mDesc = STRINGIZE('"' << std::string(params.executable) << "\" (" << pinfo.dwProcessId << ')'); + LL_INFOS("LLProcess") << "Launched " << params << " (" << pinfo.dwProcessId << ")" << 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(params.executable, pinfo.hProcess); + LLJob::instance().assignProcess(mDesc, mProcessID); } - - // 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; - CloseHandle(pinfo.hThread); // stops leaks - nothing else } -LLProcess::id LLProcess::isRunning(id handle) +LLProcess::id LLProcess::isRunning(id handle, const std::string& desc) { if (! handle) return 0; @@ -212,6 +215,10 @@ LLProcess::id LLProcess::isRunning(id handle) if(waitresult == WAIT_OBJECT_0) { // the process has completed. + if (! desc.empty()) + { + LL_INFOS("LLProcess") << desc << " terminated" << LL_ENDL; + } return 0; } @@ -223,6 +230,7 @@ bool LLProcess::kill(void) if (! mProcessID) return false; + LL_INFOS("LLProcess") << "killing " << mDesc << LL_ENDL; TerminateProcess(mProcessID, 0); return ! isRunning(); } @@ -369,9 +377,12 @@ void LLProcess::launch(const LLSDParamAdapter& params) // parent process mProcessID = child; + + mDesc = STRINGIZE('"' << std::string(params.executable) << "\" (" << mProcessID << ')'); + LL_INFOS("LLProcess") << "Launched " << params << " (" << mProcessID << ")" << LL_ENDL; } -LLProcess::id LLProcess::isRunning(id pid) +LLProcess::id LLProcess::isRunning(id pid, const std::string& desc) { if (! pid) return 0; @@ -380,6 +391,10 @@ LLProcess::id LLProcess::isRunning(id pid) if(reap_pid(pid)) { // the process has exited. + if (! desc.empty()) + { + LL_INFOS("LLProcess") << desc << " terminated" << LL_ENDL; + } return 0; } @@ -393,6 +408,7 @@ bool LLProcess::kill(void) // 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); // This will have the side-effect of reaping the zombie if the process has exited. diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 7dbdf23679..019c33592c 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -97,7 +97,7 @@ public: typedef HANDLE id; #else typedef pid_t id; -#endif +#endif /// Get platform-specific process ID id getProcessID() const { return mProcessID; }; @@ -114,13 +114,14 @@ public: * functionality should be added as nonstatic members operating on * mProcessID. */ - static id isRunning(id); - + static id isRunning(id, const std::string& desc=""); + private: /// constructor is private: use create() instead LLProcess(const LLSDParamAdapter& params); void launch(const LLSDParamAdapter& params); + std::string mDesc; id mProcessID; bool mAutokill; }; diff --git a/indra/newview/llexternaleditor.cpp b/indra/newview/llexternaleditor.cpp index 0d3ed0ba35..561b87618c 100644 --- a/indra/newview/llexternaleditor.cpp +++ b/indra/newview/llexternaleditor.cpp @@ -102,9 +102,7 @@ LLExternalEditor::EErrorCode LLExternalEditor::run(const std::string& file_path) params.args.add(fixed); } - // Run the editor. - llinfos << "Running editor command [" << params << "]" << llendl; - // Prevent killing the process in destructor. + // Run the editor. Prevent killing the process in destructor. params.autokill = false; return LLProcess::create(params) ? EC_SUCCESS : EC_FAILED_TO_RUN; } -- cgit v1.3 From 61e98256df80822f9504a2037d5cbb029c39506d Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 23 Jan 2012 17:38:06 -0500 Subject: Clarify that items in LLProcess::Params::args are implicitly quoted. That is, we try to pass through each args entry as a separate child-process arvg[] entry, whitespace and all. --- indra/llcommon/llprocess.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 019c33592c..51c42582ea 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -63,7 +63,16 @@ public: /// pathname of executable Mandatory executable; - /// zero or more additional command-line arguments + /** + * 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. + */ Multiple args; /// current working directory, if need it changed Optional cwd; -- 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/llprocess.h') 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/llprocess.h') 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 5d2bb536324a906078b224bdc9697b368e96a6b6 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 7 Feb 2012 12:06:03 -0500 Subject: On Linux, #undef Status: we use that name for nested LLProcess struct. Apparently something in the Linux system header chain #defines a macro Status as 'int'. That's just Bad in C++ land. It should at the very least be a typedef! #undefining it in llprocess.h permits the viewer to build. --- indra/llcommon/llprocess.h | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 689f8aedab..0de033c15b 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -37,6 +37,10 @@ #if LL_WINDOWS #define WIN32_LEAN_AND_MEAN #include // HANDLE (eye roll) +#elif LL_LINUX +#if defined(Status) +#undef Status +#endif #endif class LLProcess; -- 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/llprocess.h') 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/llprocess.h') 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 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/llprocess.h') 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 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/llprocess.h') 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 d6ed77a598a0009d386ce3cd6c49d0f1c4b422e8 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 16 Feb 2012 17:39:56 -0500 Subject: Attempt to fix Windows link error for LLProcess::BasePipe::npos. --- indra/llcommon/llprocess.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 2c6951b562..06be0954c0 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -291,7 +291,7 @@ public: std::string getPipeName(FILESLOT); /// base of ReadPipe, WritePipe - class BasePipe + class LL_COMMON_API BasePipe { public: virtual ~BasePipe() = 0; -- 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/llprocess.h') 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/llprocess.h') 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/llprocess.h') 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 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/llprocess.h') 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 4edf34ed01611d75bdcd98aa065a2b286845ebd9 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 15 Mar 2012 23:30:36 -0400 Subject: Promote LLProcess::ReadPipe::size() to BasePipe (hence WritePipe). Certain use cases need to know whether the WritePipe buffer has been flushed to the pipe, or is still pending. --- indra/llcommon/llprocess.cpp | 1 + indra/llcommon/llprocess.h | 24 ++++++++++++++---------- 2 files changed, 15 insertions(+), 10 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index bd08c3ab51..d4786035ce 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -163,6 +163,7 @@ public: } virtual std::ostream& get_ostream() { return mStream; } + virtual size_type size() const { return mStreambuf.size(); } bool tick(const LLSD&) { diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 06ada83698..51010966f9 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -340,6 +340,20 @@ public: typedef std::size_t size_type; static const size_type npos; + + /** + * Get accumulated buffer length. + * + * For WritePipe, is there still pending data to send to child? + * + * For ReadPipe, we often 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 size_type size() const = 0; }; /// As returned by getWritePipe() or getOptWritePipe() @@ -387,16 +401,6 @@ public: */ virtual std::string read(size_type len) = 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 size_type size() const = 0; - /** * Peek at accumulated buffer data without consuming it. Optional * parameters give you substr() functionality. -- cgit v1.3 From 166f75d91f180d0ac45f0b8ee7c3a1b6742209fa Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 6 Jun 2012 16:38:16 -0400 Subject: MAINT-1144: Defend against NULL LLPluginProcessParent::mProcess. The change from LLProcessLauncher to LLProcess introduces the possibility of a NULL (default-constructed) LLProcessPtr. Add certain static LLProcess methods accepting LLProcessPtr, forwarding to nonstatic method when non-NULL but doing something reasonable with NULL. Use these methods in LLPLuginProcessParent. --- indra/llcommon/llprocess.cpp | 38 ++++++++++++++++++++++++++++++++ indra/llcommon/llprocess.h | 11 +++++++-- indra/llplugin/llpluginprocessparent.cpp | 13 +++++------ 3 files changed, 52 insertions(+), 10 deletions(-) (limited to 'indra/llcommon/llprocess.h') diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 9667e4e033..715df36f39 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -819,16 +819,43 @@ bool LLProcess::kill(const std::string& who) return ! isRunning(); } +//static +bool LLProcess::kill(const LLProcessPtr& p, const std::string& who) +{ + if (! p) + return true; // process dead! (was never running) + return p->kill(who); +} + bool LLProcess::isRunning() const { return getStatus().mState == RUNNING; } +//static +bool LLProcess::isRunning(const LLProcessPtr& p) +{ + if (! p) + return false; + return p->isRunning(); +} + LLProcess::Status LLProcess::getStatus() const { return mStatus; } +//static +LLProcess::Status LLProcess::getStatus(const LLProcessPtr& p) +{ + if (! p) + { + // default-constructed Status has mState == UNSTARTED + return Status(); + } + return p->getStatus(); +} + std::string LLProcess::getStatusString() const { return getStatusString(getStatus()); @@ -839,6 +866,17 @@ std::string LLProcess::getStatusString(const Status& status) const return getStatusString(mDesc, status); } +//static +std::string LLProcess::getStatusString(const std::string& desc, const LLProcessPtr& p) +{ + if (! p) + { + // default-constructed Status has mState == UNSTARTED + return getStatusString(desc, Status()); + } + return desc + " " + p->getStatusString(); +} + //static std::string LLProcess::getStatusString(const std::string& desc, const Status& status) { diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 51010966f9..d711ce2f74 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -239,6 +239,10 @@ public: /// Is child process still running? bool isRunning() const; + // static isRunning(LLProcessPtr), getStatus(LLProcessPtr), + // getStatusString(LLProcessPtr), kill(LLProcessPtr) handle the case in + // which the passed LLProcessPtr might be NULL (default-constructed). + static bool isRunning(const LLProcessPtr&); /** * State of child process @@ -272,8 +276,10 @@ public: /// Status query Status getStatus() const; + static Status getStatus(const LLProcessPtr&); /// English Status string query, for logging etc. std::string getStatusString() const; + static std::string getStatusString(const std::string& desc, const LLProcessPtr&); /// English Status string query for previously-captured Status std::string getStatusString(const Status& status) const; /// static English Status string query @@ -282,6 +288,7 @@ public: // 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(const std::string& who=""); + static bool kill(const LLProcessPtr& p, const std::string& who=""); #if LL_WINDOWS typedef int id; ///< as returned by getProcessID() @@ -314,10 +321,10 @@ public: * 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() + * In particular, if child termination is detected by this 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 + * to obtain it. This static isRunning() is only intended for after the * launching LLProcess object has been destroyed. */ static handle isRunning(handle, const std::string& desc=""); diff --git a/indra/llplugin/llpluginprocessparent.cpp b/indra/llplugin/llpluginprocessparent.cpp index f10eaee5b4..71a6145b58 100644 --- a/indra/llplugin/llpluginprocessparent.cpp +++ b/indra/llplugin/llpluginprocessparent.cpp @@ -134,11 +134,8 @@ LLPluginProcessParent::~LLPluginProcessParent() // and remove it from our map mSharedMemoryRegions.erase(iter); } - - if (mProcess) - { - mProcess->kill(); - } + + LLProcess::kill(mProcess); killSockets(); } @@ -471,7 +468,7 @@ void LLPluginProcessParent::idle(void) break; case STATE_EXITING: - if (! mProcess->isRunning()) + if (! LLProcess::isRunning(mProcess)) { setState(STATE_CLEANUP); } @@ -499,7 +496,7 @@ void LLPluginProcessParent::idle(void) break; case STATE_CLEANUP: - mProcess->kill(); + LLProcess::kill(mProcess); killSockets(); setState(STATE_DONE); break; @@ -1078,7 +1075,7 @@ bool LLPluginProcessParent::pluginLockedUpOrQuit() { bool result = false; - if (! mProcess->isRunning()) + if (! LLProcess::isRunning(mProcess)) { LL_WARNS("Plugin") << "child exited" << LL_ENDL; result = true; -- cgit v1.3