From 5c5cf11d036dcde6fa91b426723a22f14cb3fa85 Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev <117672381+akleshchev@users.noreply.github.com> Date: Fri, 10 Jul 2026 22:56:12 +0300 Subject: [PATCH 01/36] Replace apr process with boost process --- indra/cmake/Boost.cmake | 19 +- indra/llcommon/llprocess.cpp | 1556 +++++++++++++++++----------------- indra/llcommon/llprocess.h | 472 ++--------- 3 files changed, 880 insertions(+), 1167 deletions(-) diff --git a/indra/cmake/Boost.cmake b/indra/cmake/Boost.cmake index 3bea85f3ea..f73d2cf296 100644 --- a/indra/cmake/Boost.cmake +++ b/indra/cmake/Boost.cmake @@ -60,6 +60,14 @@ if (WINDOWS) libboost_url-mt${addrsfx} PATHS "${ARCH_PREBUILT_DIRS_RELEASE}" REQUIRED NO_DEFAULT_PATH) + # Add Boost.Process library + find_library(BOOST_PROCESS_LIBRARY + NAMES + libboost_process + libboost_process-mt + libboost_process-mt${addrsfx} + PATHS "${ARCH_PREBUILT_DIRS_RELEASE}" REQUIRED NO_DEFAULT_PATH) + else (WINDOWS) find_library(BOOST_CONTEXT_LIBRARY @@ -104,6 +112,14 @@ else (WINDOWS) boost_url-mt${addrsfx} PATHS "${ARCH_PREBUILT_DIRS_RELEASE}" REQUIRED NO_DEFAULT_PATH) + # Add Boost.Process library + find_library(BOOST_PROCESS_LIBRARY + NAMES + boost_process + boost_process-mt + boost_process-mt${addrsfx} + PATHS "${ARCH_PREBUILT_DIRS_RELEASE}" REQUIRED NO_DEFAULT_PATH) + endif (WINDOWS) target_link_libraries(ll::boost INTERFACE @@ -112,7 +128,8 @@ target_link_libraries(ll::boost INTERFACE ${BOOST_FILESYSTEM_LIBRARY} ${BOOST_PROGRAMOPTIONS_LIBRARY} ${BOOST_THREAD_LIBRARY} - ${BOOST_URL_LIBRARY}) + ${BOOST_URL_LIBRARY} + ${BOOST_PROCESS_LIBRARY}) if (LINUX) target_link_libraries(ll::boost INTERFACE rt) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 4a01ec567e..105bc315bf 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -35,7 +35,17 @@ #include "apr_signal.h" #include "llevents.h" #include "llexception.h" +#include "stringize.h" +#include +#include +#include +#include +#include +#include +#include +#include +#include #include #include #include @@ -47,11 +57,16 @@ #include #include + +namespace bp = boost::process::v1; +namespace asio = boost::asio; + /***************************************************************************** * Helpers *****************************************************************************/ +/* 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); @@ -60,7 +75,7 @@ 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 @@ -130,731 +145,725 @@ class LLProcessListener }; static LLProcessListener sProcessListener; +std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params) +{ + if (params.cwd.isProvided()) + { + out << "cd " << LLStringUtil::quote(params.cwd) << ": "; + } + out << LLStringUtil::quote(params.executable); + for (const std::string& arg : params.args) + { + out << ' ' << LLStringUtil::quote(arg); + } + return out; +} /***************************************************************************** -* WritePipe and ReadPipe +* Helper classes for pipe I/O *****************************************************************************/ -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 +class WritePipeImpl : public LLProcess::WritePipe { LOG_CLASS(WritePipeImpl); public: - WritePipeImpl(const std::string& desc, apr_file_t* pipe): + WritePipeImpl(const std::string& desc, + std::shared_ptr 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)); - -#if ! LL_WINDOWS - // We can't count on every child process reading everything we try to - // write to it. And if the child terminates with WritePipe data still - // pending, unless we explicitly suppress it, Posix will hit us with - // SIGPIPE. That would terminate the viewer, boom. "Ignoring" it means - // APR gets the correct errno, passes it back to us, we log it, etc. - signal(SIGPIPE, SIG_IGN); -#endif + mStream(&mStreambuf), + mWritePending(false) + { + // Start async write monitoring + startAsyncWrite(); } - virtual std::ostream& get_ostream() { return mStream; } - virtual size_type size() const { return mStreambuf.size(); } + virtual ~WritePipeImpl() = default; - bool tick(const LLSD&) + virtual std::ostream& get_ostream() override { return mStream; } + + virtual size_type size() const override { - typedef boost::asio::streambuf::const_buffers_type const_buffer_sequence; - // If there's anything to send, try to send it. - std::size_t total(mStreambuf.size()), consumed(0); - if (total) + return mStreambuf.size(); + } + + void tick() + { + // Don't start a new write if one is already pending + if (mWritePending || !mPipe || !mPipe->is_open() || mStreambuf.size() == 0) + return; + + mWritePending = true; + + // Write buffered data asynchronously + asio::async_write(*mPipe, mStreambuf.data(), + [this](const boost::system::error_code& ec, std::size_t bytes_transferred) { - const_buffer_sequence bufs = mStreambuf.data(); - // In general, our streambuf might contain a number of different - // physical buffers; iterate over those. - bool keepwriting = true; - for (auto bufi(boost::asio::buffer_sequence_begin(bufs)), bufend(boost::asio::buffer_sequence_end(bufs)); - bufi != bufend && keepwriting; ++bufi) + mWritePending = false; + + if (!ec) { - // http://www.boost.org/doc/libs/1_49_0_beta1/doc/html/boost_asio/reference/buffer.html#boost_asio.reference.buffer.accessing_buffer_contents - // Although apr_file_write() accepts const void*, we - // manipulate const char* so we can increment the pointer. - const char* remainptr = static_cast(bufi->data()); - std::size_t remainlen = boost::asio::buffer_size(*bufi); - while (remainlen) - { - // Tackle the current buffer in discrete chunks. On - // Windows, we've observed strange failures when trying to - // write big lengths (~1 MB) in a single operation. Even a - // 32K chunk seems too large. At some point along the way - // apr_file_write() returns 11 (Resource temporarily - // unavailable, i.e. EAGAIN) and says it wrote 0 bytes -- - // even though it did write the chunk! Our next write - // attempt retries with the same chunk, resulting in the - // chunk being duplicated at the child end. Using smaller - // chunks is empirically more reliable. - std::size_t towrite((std::min)(remainlen, std::size_t(4*1024))); - apr_size_t written(towrite); - apr_status_t err = apr_file_write(mPipe, remainptr, &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); - } + mStreambuf.consume(bytes_transferred); + LL_DEBUGS("LLProcess") << "Wrote " << bytes_transferred + << " bytes to " << mDesc << LL_ENDL; + } + else if (ec != asio::error::operation_aborted) + { + LL_WARNS("LLProcess") << "Write error on " << mDesc + << ": " << ec.message() << LL_ENDL; + } + }); + } - // '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; - // don't forget to advance to next chunk of current buffer - remainptr += written; - remainlen -= written; - - char msgbuf[512]; - LL_DEBUGS("LLProcess") << "wrote " << written << " of " << towrite - << " bytes to " << mDesc - << " (original " << total << ")," - << " code " << err << ": " - << apr_strerror(err, msgbuf, sizeof(msgbuf)) - << 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) - { - keepwriting = false; // break outer loop over buffers too - break; - } - } // next chunk of current buffer - } // next buffer - // 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 later. - mStreambuf.consume(consumed); - } +private: + void startAsyncWrite() + { + if (!mPipe || !mPipe->is_open() || mStreambuf.size() == 0) + return; - return false; + // Write buffered data asynchronously + asio::async_write(*mPipe, mStreambuf.data(), + [this](const boost::system::error_code& ec, std::size_t bytes_transferred) + { + if (!ec) + { + mStreambuf.consume(bytes_transferred); + LL_DEBUGS("LLProcess") << "Wrote " << bytes_transferred + << " bytes to " << mDesc << LL_ENDL; + // Continue writing if there's more data + startAsyncWrite(); + } + else if (ec != asio::error::operation_aborted) + { + LL_WARNS("LLProcess") << "Write error on " << mDesc + << ": " << ec.message() << LL_ENDL; + } + }); } -private: std::string mDesc; - apr_file_t* mPipe; - LLTempBoundListener mConnection; - boost::asio::streambuf mStreambuf; + std::shared_ptr mPipe; + asio::streambuf mStreambuf; std::ostream mStream; + bool mWritePending; }; -class ReadPipeImpl: public LLProcess::ReadPipe +class ReadPipeImpl : public LLProcess::ReadPipe { LOG_CLASS(ReadPipeImpl); public: - ReadPipeImpl(const std::string& desc, apr_file_t* pipe, LLProcess::FILESLOT index): + ReadPipeImpl(const std::string& desc, + std::shared_ptr pipe, + LLProcess::FILESLOT slot) : mDesc(desc), mPipe(pipe), - mIndex(index), - // Essential to initialize our std::istream with our special streambuf! + mSlot(slot), mStream(&mStreambuf), - mPump("ReadPipe", true), // tweak name as needed to avoid collisions + mPump("ReadPipe", true), // tweak name as needed to avoid collisions, use LLEventPump::inventName? mLimit(0), mEOF(false) { - mConnection = LLEventPumps::instance().obtain("mainloop") - .listen(LLEventPump::inventName("ReadPipe"), - boost::bind(&ReadPipeImpl::tick, this, _1)); + // Start async read + startAsyncRead(); } - ~ReadPipeImpl() + virtual ~ReadPipeImpl() { - if (mConnection.connected()) + if (mPipe && mPipe->is_open()) { - mConnection.disconnect(); + boost::system::error_code ec; + mPipe->close(ec); } } - // 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::istream& get_istream() override { return mStream; } + + virtual std::string getline() override + { + return LLProcess::getline(mStream); + } + + virtual LLEventPump& getPump() override { return mPump; } + + virtual void setLimit(size_type limit) override { mLimit = limit; } + + virtual size_type getLimit() const override { return mLimit; } - virtual std::string read(size_type len) + virtual size_type size() const override { return mStreambuf.size(); } + + virtual std::string read(size_type len) override { - // Read specified number of bytes into a buffer. - size_type readlen((std::min)(size(), len)); - // Formally, &buffer[0] is invalid for a vector of size() 0. Exit - // early in that situation. - if (! readlen) + size_type readlen = (std::min)(size(), len); + if (!readlen) return ""; - // Make a buffer big enough. + 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 + virtual std::string peek(size_type offset = 0, size_type len = npos) const override { - // Constrain caller's offset and len to overlap actual buffer content. std::size_t real_offset = (std::min)(mStreambuf.size(), std::size_t(offset)); - size_type want_end = (len == npos)? npos : (real_offset + len); - std::size_t real_end = (std::min)(mStreambuf.size(), std::size_t(want_end)); - boost::asio::streambuf::const_buffers_type cbufs = mStreambuf.data(); - return std::string(boost::asio::buffers_begin(cbufs) + real_offset, - boost::asio::buffers_begin(cbufs) + real_end); + size_type want_end = (len == npos) ? npos : (real_offset + len); + std::size_t real_end = (std::min)(mStreambuf.size(), std::size_t(want_end)); + + auto cbufs = mStreambuf.data(); + return std::string(asio::buffers_begin(cbufs) + real_offset, + asio::buffers_begin(cbufs) + real_end); } - virtual size_type find(const std::string& seek, size_type offset=0) const + virtual size_type find(const std::string& seek, size_type offset = 0) const override { - // 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); + auto cbufs = mStreambuf.data(); + auto begin = asio::buffers_begin(cbufs); + auto end = asio::buffers_end(cbufs); + auto found = std::search(begin + offset, end, seek.begin(), seek.end()); + return (found == end) ? npos : (found - begin); } - virtual size_type find(char seek, size_type offset=0) const + virtual size_type find(char seek, size_type offset = 0) const override { - // 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()) - { 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::find(begin + offset, end, seek)); - return (found == end)? npos : (found - begin); + auto cbufs = mStreambuf.data(); + auto begin = asio::buffers_begin(cbufs); + auto end = asio::buffers_end(cbufs); + auto found = std::find(begin + offset, end, seek); + return (found == end) ? npos : (found - begin); } - bool tick(const LLSD&) +private: + void startAsyncRead() { - // 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. - enum PipeState { RETRY, EXHAUSTED, CLOSED }; - PipeState state = RETRY; - std::size_t committed(0); - do + if (!mPipe || !mPipe->is_open() || mEOF) + return; + + size_type to_read = (mLimit > 0 && mStreambuf.size() >= mLimit) ? 0 : 4096; + if (to_read == 0) + return; + + auto bufs = mStreambuf.prepare(to_read); + + mPipe->async_read_some(bufs, + [this](const boost::system::error_code& ec, std::size_t bytes_transferred) { - // 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 (auto bufi(boost::asio::buffer_sequence_begin(bufs)), bufend(boost::asio::buffer_sequence_end(bufs)); - bufi != bufend; ++bufi) + if (!ec) { - // 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, - bufi->data(), - &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))) + mStreambuf.commit(bytes_transferred); + LL_DEBUGS("LLProcess") << "Read " << bytes_transferred + << " bytes from " << mDesc << LL_ENDL; + + LLSD event; + event["len"] = LLSD::Integer(mStreambuf.size()); + event["slot"] = LLSD::Integer(mSlot); + event["desc"] = mDesc; + + if (mLimit > 0) { - // 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(); - // Ignore any subsequent calls we might get anyway. - mEOF = true; - state = CLOSED; // also break outer retry loop - break; + size_type data_len = (std::min)(mStreambuf.size(), mLimit); + event["data"] = peek(0, data_len); } - // '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") << "filled " << 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) + mPump.post(event); + startAsyncRead(); + } + else if (ec == asio::error::eof) + { + if (bytes_transferred > 0) { - // break outer retry loop too - state = EXHAUSTED; - break; + mStreambuf.commit(bytes_transferred); + + LLSD event; + event["len"] = LLSD::Integer(mStreambuf.size()); + event["slot"] = LLSD::Integer(mSlot); + event["desc"] = mDesc; + + if (mLimit > 0) + { + size_type data_len = (std::min)(mStreambuf.size(), mLimit); + event["data"] = peek(0, data_len); + } + + mPump.post(event); } - } - // Don't forget to "commit" the data! - mStreambuf.commit(tocommit); - committed += tocommit; - - // 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 (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 - // 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())) - ("slot", LLSD::Integer(mIndex)) - ("name", whichfile(mIndex)) - ("desc", mDesc) - ("eof", state == CLOSED) - ("exhst", state == EXHAUSTED)); - } + mEOF = true; + LL_DEBUGS("LLProcess") << "EOF on " << mDesc << LL_ENDL; - return false; + LLSD eof_event; + eof_event["eof"] = true; + eof_event["slot"] = LLSD::Integer(mSlot); + eof_event["desc"] = mDesc; + mPump.post(eof_event); + } + else if (ec != asio::error::operation_aborted) + { + LL_WARNS("LLProcess") << "Read error on " << mDesc + << ": " << ec.message() << LL_ENDL; + } + }); } -private: std::string mDesc; - apr_file_t* mPipe; - LLProcess::FILESLOT mIndex; - LLTempBoundListener mConnection; - boost::asio::streambuf mStreambuf; + std::shared_ptr mPipe; + LLProcess::FILESLOT mSlot; + mutable asio::streambuf mStreambuf; std::istream mStream; - LLEventStream mPump; + LLEventStream mPump; // pump specific to this pipe size_type mLimit; bool mEOF; }; /***************************************************************************** -* LLProcess itself +* LLProcess implementation *****************************************************************************/ -/// Need an exception to avoid constructing an invalid LLProcess object, but -/// internal use only -struct LLProcessError: public LLException + +const LLProcess::BasePipe::size_type LLProcess::BasePipe::npos = +static_cast(-1); + +LLProcess::LLProcess(const Params& params) : + mStatus(), + mDesc(params.desc.isProvided() ? params.desc() : basename(params.executable())), + mPostend(params.postend.isProvided() ? params.postend() : ""), + mAutokill(params.autokill), + mAttached(params.attached) { - LLProcessError(const std::string& msg): LLException(msg) {} -}; + launch(params); +} -LLProcessPtr LLProcess::create(const LLSDOrParams& params) +LLProcess::~LLProcess() { - try + if (mChild && mChild->running()) { - return LLProcessPtr(new LLProcess(params)); - } - catch (const LLProcessError& e) - { - LL_WARNS("LLProcess") << e.what() << LL_ENDL; + if (mAttached && mAutokill) + { + LL_INFOS("LLProcess") << "Terminating child process " << mDesc << LL_ENDL; + std::error_code ec; + mChild->terminate(ec); + +#if !LL_WINDOWS + // On POSIX, terminate() sends SIGTERM which allows graceful shutdown + // Give the process some time to terminate gracefully + for (int i = 0; i < 30 && mChild->running(); ++i) + { + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + } - // 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 (params.postend.isProvided()) + // Force kill if still running + if (mChild->running()) + { + LL_WARNS("LLProcess") << "Force killing " << mDesc << LL_ENDL; + (void)::kill(mChild->id(), SIGKILL); + } +#else + // On Windows, terminate() already does an immediate hard kill via TerminateProcess() + // Wait briefly to ensure termination completes + WaitForSingleObject(mChild->native_handle(), 100); +#endif + } + else { - LLEventPumps::instance().obtain(params.postend) - .post(LLSDMap - // no "id" - ("desc", getDesc(params)) - ("state", LLProcess::UNSTARTED) - // no "data" - ("string", e.what()) - ); + LL_INFOS("LLProcess") << "Not terminating " << mDesc + << " (attached=" << mAttached + << ", autokill=" << mAutokill << ")" << LL_ENDL; } + } - return LLProcessPtr(); + if (mMainloopConnection.connected()) + { + mMainloopConnection.disconnect(); } } -/// 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") +//static +LLProcessPtr LLProcess::create(const LLSDOrParams& params) +{ + try + { + return std::make_shared(params); + } + catch (const std::exception& e) + { + LL_WARNS("LLProcess") << "Failed to create process: " << e.what() << LL_ENDL; + return LLProcessPtr(); + } +} -LLProcess::LLProcess(const LLSDOrParams& params): - mAutokill(params.autokill), - // Because 'autokill' originally meant both 'autokill' and 'attached', to - // preserve existing semantics, we promise that mAttached defaults to the - // same setting as mAutokill. - mAttached(params.attached.isProvided()? params.attached : params.autokill), - mPool(NULL) +void LLProcess::launch(const Params& params) { - mPipes.resize(NSLOTS); + // Validate FileParam types before attempting to launch + int file_idx = 0; + for (const auto& fparam : params.files) + { + if (fparam.type.isProvided()) + { + const std::string& type = fparam.type(); + // Only "" (inherit) and "pipe" are supported + if (!type.empty() && type != "pipe") + { + std::string slotname; + switch (file_idx) + { + case STDIN: slotname = "stdin"; break; + case STDOUT: slotname = "stdout"; break; + case STDERR: slotname = "stderr"; break; + default: slotname = STRINGIZE("file slot " << file_idx); break; + } - if (! params.validateBlock(true)) + LL_WARNS("LLProcess") << "For " << params.executable() + << ": unsupported FileParam for " << slotname + << ": type='" << type << "'"; + + if (fparam.name.isProvided()) + { + LL_CONT << ", name='" << fparam.name() << "'"; + } + + LL_CONT << LL_ENDL; + + throw std::runtime_error( + STRINGIZE("unsupported FileParam type '" << type + << "' for " << slotname)); + } + + // Warn about internal pipe names (not yet supported) + if (type == "pipe" && fparam.name.isProvided() && !fparam.name().empty()) + { + LL_WARNS("LLProcess") << "Internal pipe name '" << fparam.name() + << "' not yet supported; ignoring" << LL_ENDL; + } + } + file_idx++; + } + + // Build arguments vector + std::vector args; + for (const auto& arg : params.args) { - LLTHROW(LLProcessError(STRINGIZE("not launched: failed parameter validation\n" - << LLSDNotationStreamer(params)))); + args.push_back(arg); } - mPostend = params.postend; + // Determine pipe configuration + bool use_stdin_pipe = false; + bool use_stdout_pipe = false; + bool use_stderr_pipe = false; - apr_pool_create(&mPool, gAPRPoolp); - if (!mPool) + file_idx = 0; + for (const auto& fparam : params.files) { - LLTHROW(LLProcessError(STRINGIZE("failed to create apr pool"))); + if (fparam.type.isProvided() && fparam.type() == "pipe") + { + switch (file_idx) + { + case STDIN: use_stdin_pipe = true; break; + case STDOUT: use_stdout_pipe = true; break; + case STDERR: use_stderr_pipe = true; break; + } + } + file_idx++; } - apr_procattr_t *procattr = NULL; - chkapr(apr_procattr_create(&procattr, mPool)); + // Create pipes if needed + if (use_stdin_pipe) + { + mStdinPipe = std::make_unique(mIOContext); + LL_DEBUGS("LLProcess") << "Created stdin pipe for " << mDesc << LL_ENDL; + } + if (use_stdout_pipe) + { + mStdoutPipe = std::make_unique(mIOContext); + LL_DEBUGS("LLProcess") << "Created stdout pipe for " << mDesc << LL_ENDL; + } + if (use_stderr_pipe) + { + mStderrPipe = std::make_unique(mIOContext); + LL_DEBUGS("LLProcess") << "Created stderr pipe for " << mDesc << LL_ENDL; + } - // IQA-490, CHOP-900: On Windows, ask APR to jump through hoops to - // constrain the set of handles passed to the child process. Before we - // changed to APR, the Windows implementation of LLProcessLauncher called - // CreateProcess(bInheritHandles=false), meaning to pass NO open handles - // to the child process. Now that we support pipes, though, we must allow - // apr_proc_create() to pass bInheritHandles=true. But without taking - // special pains, that causes trouble in a number of ways, due to the fact - // that the viewer is constantly opening and closing files -- most of - // which CreateProcess() passes to every child process! -#if ! defined(APR_HAS_PROCATTR_CONSTRAIN_HANDLE_SET) - // Our special preprocessor symbol isn't even defined -- wrong APR - LL_WARNS("LLProcess") << "This version of APR lacks Linden " - << "apr_procattr_constrain_handle_set() extension" << LL_ENDL; -#else - chkapr(apr_procattr_constrain_handle_set(procattr, 1)); -#endif + // Build the process + try + { + std::error_code ec; - // 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). - // 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). - std::vector select; - for (const FileParam& fparam : params.files) - { - // 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 + // Create child process with appropriate redirections + if (use_stdin_pipe && use_stdout_pipe && use_stderr_pipe) { - select.push_back(APR_NO_PIPE); + if (params.cwd.isProvided()) + { + mChild = std::make_unique( + params.executable(), + bp::args(args), + bp::std_in < *mStdinPipe, + bp::std_out > *mStdoutPipe, + bp::std_err > *mStderrPipe, + bp::start_dir(params.cwd()), + mIOContext, + ec + ); + } + else + { + mChild = std::make_unique( + params.executable(), + bp::args(args), + bp::std_in < *mStdinPipe, + bp::std_out > *mStdoutPipe, + bp::std_err > *mStderrPipe, + mIOContext, + ec + ); + } } - else if (fparam.type() == "pipe") // anonymous pipe + else if (use_stdout_pipe && use_stderr_pipe) { - if (! fparam.name().empty()) + if (params.cwd.isProvided()) { - LL_WARNS("LLProcess") << "For " << params.executable() - << ": internal names for reusing pipes ('" - << fparam.name() << "' for " << which - << ") are not yet supported -- creating distinct pipe" - << LL_ENDL; + mChild = std::make_unique( + params.executable(), + bp::args(args), + bp::std_out > *mStdoutPipe, + bp::std_err > *mStderrPipe, + bp::start_dir(params.cwd()), + mIOContext, + ec + ); + } + else + { + mChild = std::make_unique( + params.executable(), + bp::args(args), + bp::std_out > *mStdoutPipe, + bp::std_err > *mStderrPipe, + mIOContext, + ec + ); + } + } + else if (use_stdout_pipe) + { + if (params.cwd.isProvided()) + { + mChild = std::make_unique( + params.executable(), + bp::args(args), + bp::std_out > *mStdoutPipe, + bp::start_dir(params.cwd()), + mIOContext, + ec + ); + } + else + { + mChild = std::make_unique( + params.executable(), + bp::args(args), + bp::std_out > *mStdoutPipe, + mIOContext, + ec + ); } - // 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.push_back(APR_CHILD_BLOCK); } else { - LLTHROW(LLProcessError(STRINGIZE("For " << params.executable() - << ": unsupported FileParam for " << which - << ": type='" << fparam.type() - << "', name='" << fparam.name() << "'"))); + if (params.cwd.isProvided()) + { + mChild = std::make_unique( + params.executable(), + bp::args(args), + bp::start_dir(params.cwd()), + mIOContext, + ec + ); + } + else + { + mChild = std::make_unique( + params.executable(), + bp::args(args), + mIOContext, + ec + ); + } } - } - // 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 - // 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, mAutokill? 0 : 1)); - - if (mAutokill) - { -#if ! defined(APR_HAS_PROCATTR_AUTOKILL_SET) - // Our special preprocessor symbol isn't even defined -- wrong APR - LL_WARNS("LLProcess") << "This version of APR lacks Linden apr_procattr_autokill_set() extension" << LL_ENDL; -#elif ! APR_HAS_PROCATTR_AUTOKILL_SET - // Symbol is defined, but to 0: expect apr_procattr_autokill_set() to - // return APR_ENOTIMPL. -#else // APR_HAS_PROCATTR_AUTOKILL_SET nonzero - ll_apr_warn_status(apr_procattr_autokill_set(procattr, 1)); -#endif - } - - // 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, 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, params.cwd().c_str())); - } - - // create an argv vector for the child process - std::vector argv; + if (ec) + { + throw std::runtime_error(STRINGIZE("failed to launch " << params.executable() + << ": " << ec.message())); + } - // Add the executable path. See above remarks about c_str(). - argv.push_back(params.executable().c_str()); + mStatus.mState = RUNNING; - // Add arguments. See above remarks about c_str(). - for (const std::string& arg : params.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. Hand-expand chkapr() macro so we can fill in the actual command - // string instead of the variable names. - if (ll_apr_warn_status(apr_proc_create(&mProcess, argv[0], &argv[0], NULL, procattr, - mPool))) - { - LLTHROW(LLProcessError(STRINGIZE(params << " failed"))); - } - - // arrange to call status_callback() - apr_proc_other_child_register(&mProcess, &LLProcess::status_callback, this, mProcess.in, - mPool); - // and make sure we poll it once per "mainloop" tick - sProcessListener.addPoll(*this); - mStatus.mState = RUNNING; - - 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 - // theory our destructor should kill an autokill child, but in practice - // that doesn't always work (e.g. VWR-21538). - if (mAutokill) - { -/*==========================================================================*| - // NO: There may be an APR bug, not sure -- but at least on Mac, when - // gAPRPoolp is destroyed, OUR process receives SIGTERM! Apparently - // either our own PID is getting into the list of processes to kill() - // (unlikely), or somehow one of those PIDs is getting zeroed first, - // so that kill() sends SIGTERM to the whole process group -- this - // process included. I'd have to build and link with a debug version - // of APR to know for sure. It's too bad: this mechanism would be just - // right for dealing with static autokill LLProcessPtr variables, - // which aren't destroyed until after APR is no longer available. - - // 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 suspend 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(); - } - - // 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; - std::string desc(STRINGIZE(mDesc << ' ' << whichfile(FILESLOT(i)))); - apr_file_t* pipe(mProcess.*(members[i])); - if (i == STDIN) + // Create pipe wrappers + if (mStdinPipe) { - mPipes[i] = std::make_unique(desc, pipe); + mWritePipe = std::make_unique( + STRINGIZE(mDesc << " stdin"), + std::shared_ptr(mStdinPipe.get(), [](auto*) {}) + ); } - else + if (mStdoutPipe) { - mPipes[i] = std::make_unique(desc, pipe, FILESLOT(i)); + mStdoutReadPipe = std::make_unique( + STRINGIZE(mDesc << " stdout"), + std::shared_ptr(mStdoutPipe.get(), [](auto*) {}), + STDOUT + ); + } + if (mStderrPipe) + { + mStderrReadPipe = std::make_unique( + STRINGIZE(mDesc << " stderr"), + std::shared_ptr(mStderrPipe.get(), [](auto*) {}), + STDERR + ); } - // Removed temporaily for Xcode 7 build tests: error was: - // "error: expression with side effects will be evaluated despite - // being used as an operand to 'typeid' [-Werror,-Wpotentially-evaluated-expression]"" - //LL_DEBUGS("LLProcess") << "Instantiating " << typeid(mPipes[i]).name() - // << "('" << desc << "')" << LL_ENDL; - } -} - -// 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. - 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. - 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::size_type delim = path.find_last_of("\\/"); - // If path contains no pathname delimiters, return the whole thing. - if (delim == std::string::npos) - return path; + // Hook into mainloop for I/O processing + mMainloopConnection = LLEventPumps::instance().obtain("mainloop") + .listen(LLEventPump::inventName("LLProcess"), + [this](const LLSD&) { tick(); return false; }); - // Return just the part beyond the last delimiter. - return path.substr(delim + 1); + LL_INFOS("LLProcess") << "Launched " << mDesc + << " (PID: " << mChild->id() << ")" << LL_ENDL; + } + catch (const std::exception& e) + { + throw std::runtime_error(STRINGIZE("failed to create process: " << e.what())); + } } -LLProcess::~LLProcess() +void LLProcess::tick() { - // In the Linden viewer, there's at least one static LLProcessPtr. Its - // destructor will be called *after* ll_cleanup_apr(). In such a case, - // unregistering is pointless (and fatal!) -- and kill(), which also - // relies on APR, is impossible. - if (! gAPRPoolp) - return; - - // 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) + // Poll I/O context to process async operations + while (mIOContext.poll_one() > 0) { - // 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); - // One less LLProcess to poll for - sProcessListener.dropPoll(*this); + // Keep polling until no more handlers are ready } - if (mAttached) + if (auto* wp = dynamic_cast(mWritePipe.get())) + wp->tick(); + +#if LL_WINDOWS + // Check process status + if (mChild && !mChild->running()) { - kill("destructor"); + WaitForSingleObject(mChild->native_handle(), 100); + int exit_code = mChild->exit_code(); + handleExit(exit_code); } - - if (mPool) +#else + // Check process status using WNOHANG to avoid blocking or generating + // signals that interfere with other waitpid() callers. + if (mChild && mStatus.mState == RUNNING) { - apr_pool_destroy(mPool); - mPool = NULL; + int status = 0; + pid_t result; + do { + result = ::waitpid(mChild->id(), &status, WNOHANG); + } while (result == -1 && errno == EINTR); + + if (result == mChild->id()) + { + // Child has exited; decode status manually + int exit_code = 0; + if (WIFEXITED(status)) + exit_code = WEXITSTATUS(status); + handleExit(exit_code); + } + // result == 0 means still running; result == -1/ECHILD means already reaped } +#endif } -bool LLProcess::kill(const std::string& who) +void LLProcess::handleExit(int exit_code) { - if (isRunning()) + if (mStatus.mState != RUNNING) + return; // Already handled + + // Before handling exit, drain any remaining data from pipes + // This is important because the child might have written data just before exiting + for (int i = 0; i < 10; ++i) { - LL_INFOS("LLProcess") << who << " killing " << mDesc << LL_ENDL; + if (mIOContext.poll_one() == 0) + break; // No more work to do + } #if LL_WINDOWS - int sig = -1; -#else // Posix - int sig = SIGTERM; -#endif + // On Windows, all terminations result in EXITED state + mStatus.mState = EXITED; + mStatus.mData = exit_code; +#else + // On POSIX, need to check if process was signaled + int status; + pid_t result = waitpid(mChild->id(), &status, WNOHANG); - ll_apr_warn_status(apr_proc_kill(&mProcess, sig)); + if (result == mChild->id()) + { + if (WIFEXITED(status)) + { + mStatus.mState = EXITED; + mStatus.mData = WEXITSTATUS(status); + } + else if (WIFSIGNALED(status)) + { + mStatus.mState = KILLED; + mStatus.mData = WTERMSIG(status); + } + else + { + // Shouldn't happen, but treat as exit + mStatus.mState = EXITED; + mStatus.mData = exit_code; + } } + else + { + // Couldn't get detailed status, use what boost::process gives us + mStatus.mState = EXITED; + mStatus.mData = exit_code; + } +#endif - return ! isRunning(); -} + LL_INFOS("LLProcess") << mDesc << " exited with code " << exit_code << LL_ENDL; -//static -bool LLProcess::kill(const LLProcessPtr& p, const std::string& who) -{ - if (! p) - return true; // process dead! (was never running) - return p->kill(who); + // Post to event pump if configured + if (!mPostend.empty()) + { + LLSD event; + event["id"] = static_cast(getProcessID()); + event["desc"] = mDesc; + event["state"] = mStatus.mState; + event["data"] = exit_code; + event["string"] = getStatusString(mStatus); + + LLEventPumps::instance().obtain(mPostend).post(event); + } + + // Disconnect from mainloop + if (mMainloopConnection.connected()) + { + mMainloopConnection.disconnect(); + } } bool LLProcess::isRunning() const { - return getStatus().mState == RUNNING; + return mChild && mChild->running(); } //static -bool LLProcess::isRunning(const LLProcessPtr& p) +bool LLProcess::isRunning(const LLProcessPtr& ptr) { - if (! p) - return false; - return p->isRunning(); + return ptr && ptr->isRunning(); } LLProcess::Status LLProcess::getStatus() const @@ -863,19 +872,26 @@ LLProcess::Status LLProcess::getStatus() const } //static -LLProcess::Status LLProcess::getStatus(const LLProcessPtr& p) +LLProcess::Status LLProcess::getStatus(const LLProcessPtr& ptr) { - if (! p) + if (!ptr) { - // default-constructed Status has mState == UNSTARTED - return Status(); + Status status; + status.mState = UNSTARTED; + return status; } - return p->getStatus(); + return ptr->getStatus(); } std::string LLProcess::getStatusString() const { - return getStatusString(getStatus()); + return getStatusString(mDesc, mStatus); +} + +//static +std::string LLProcess::getStatusString(const std::string& desc, const LLProcessPtr& ptr) +{ + return getStatusString(desc, getStatus(ptr)); } std::string LLProcess::getStatusString(const Status& status) const @@ -884,235 +900,240 @@ std::string LLProcess::getStatusString(const Status& status) const } //static -std::string LLProcess::getStatusString(const std::string& desc, const LLProcessPtr& p) +std::string LLProcess::getStatusString(const std::string& desc, const Status& status) { - if (! p) + std::string result = desc + ": "; + switch (status.mState) { - // default-constructed Status has mState == UNSTARTED - return getStatusString(desc, Status()); + case UNSTARTED: return result + "not started"; + case RUNNING: return result + "running"; + case EXITED: return result + STRINGIZE("exited with code " << status.mData); + case KILLED: return result + STRINGIZE("killed by signal " << status.mData); + default: return result + "unknown state"; } - return desc + " " + p->getStatusString(); } -//static -std::string LLProcess::getStatusString(const std::string& desc, const Status& status) +bool LLProcess::kill(const std::string& who) { - if (status.mState == UNSTARTED) - return desc + " was never launched"; + if (!mChild || !mChild->running()) + return true; - if (status.mState == RUNNING) - return desc + " running"; + LL_INFOS("LLProcess") << who << " killing " << mDesc << LL_ENDL; - if (status.mState == EXITED) - return STRINGIZE(desc << " exited with code " << status.mData); + std::error_code ec; + mChild->terminate(ec); - 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 - << " (" << apr_signal_description_get(status.mData) << ")"); -#endif + if (ec) + { + LL_WARNS("LLProcess") << "Failed to terminate " << mDesc + << ": " << ec.message() << LL_ENDL; + return false; + } - return STRINGIZE(desc << " in unknown state " << status.mState << " (" << status.mData << ")"); + // Don't set status here - let handleExit() do it when the process actually terminates + // The state will be determined by how the process exited: + // - Windows: EXITED with exit code -1 + // - POSIX: KILLED with signal SIGTERM (or SIGKILL) + + return true; } -// Classic-C-style APR callback -void LLProcess::status_callback(int reason, void* data, int status) +//static +bool LLProcess::kill(const LLProcessPtr& ptr, const std::string& who) { - // Our only role is to bounce this static method call back into object - // space. - static_cast(data)->handle_status(reason, status); + return ptr && ptr->kill(who); } -#define tabent(symbol) { symbol, #symbol } -static struct ReasonCode +LLProcess::id LLProcess::getProcessID() const { - int code; - const char* name; -} reasons[] = + if (!mChild) + return 0; + +#if LL_WINDOWS + return static_cast(mChild->id()); +#else + return mChild->id(); +#endif +} + +LLProcess::handle LLProcess::getProcessHandle() const { - 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 + if (!mChild) + return 0; -// Object-oriented callback -void LLProcess::handle_status(int reason, int status) +#if LL_WINDOWS + return mChild->native_handle(); +#else + return mChild->id(); +#endif +} + +//static +LLProcess::handle LLProcess::isRunning(handle h, const std::string& desc) { - { - // 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; - for (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 LL_WINDOWS + if (h == 0 || h == INVALID_HANDLE_VALUE) + return 0; - if (! (reason == APR_OC_REASON_DEATH || reason == APR_OC_REASON_LOST)) + DWORD exit_code; + if (GetExitCodeProcess(h, &exit_code)) { - // We're only interested in the call when the child terminates. - return; + return (exit_code == STILL_ACTIVE) ? h : 0; } + return 0; +#else + if (h == 0) + return 0; - // 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); - // 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 - // KILLED; refine below. - mStatus.mState = EXITED; + // Use waitpid with WNOHANG to check if process is still running + // This is more reliable than kill(pid, 0) and properly reaps zombies + int status; + pid_t result; + + // Retry on EINTR (interrupted system call) + do + { + result = waitpid(h, &status, WNOHANG); + } while (result == -1 && errno == EINTR); - // 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) + if (result == 0) + { + // Process still running + return h; + } + else if (result == h) + { + // Process has terminated (and we've reaped it) + return 0; + } + else if (result == -1) + { + // Error occurred + if (errno == ECHILD) { - static LLSD trivial; - ppipe->tick(trivial); + // Process doesn't exist or was already reaped + return 0; } + // For other errors, assume process is gone + LL_WARNS("LLProcess") << "waitpid(" << h << ") failed: " + << strerror(errno) << LL_ENDL; + return 0; } -// 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; - - // 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()) - ); - } + // Shouldn't get here, but if we do, assume process is gone + return 0; +#endif } -LLProcess::id LLProcess::getProcessID() const +std::string LLProcess::getPipeName(FILESLOT slot) const { - return mProcess.pid; + // Named pipes not yet implemented in this PoC + return ""; } -LLProcess::handle LLProcess::getProcessHandle() const +LLProcess::WritePipe& LLProcess::getWritePipe(FILESLOT slot) { -#if LL_WINDOWS - return mProcess.hproc; -#else - return mProcess.pid; -#endif -} + if (slot != STDIN) + throw NoPipe(STRINGIZE(mDesc << ": no slot " << slot)); -std::string LLProcess::getPipeName(FILESLOT) const -{ - // LLProcess::FileParam::type "npipe" is not yet implemented - return ""; + if (!mWritePipe) + throw NoPipe(STRINGIZE(mDesc << ": stdin is not a monitored pipe")); + + return *mWritePipe; } -template -PIPETYPE* LLProcess::getPipePtr(std::string& error, FILESLOT slot) +LLProcess::ReadPipe& LLProcess::getReadPipe(FILESLOT slot) { if (slot >= NSLOTS) + throw NoPipe(STRINGIZE(mDesc << ": no slot " << slot)); + + if (slot == STDIN) + throw NoPipe(STRINGIZE(mDesc << ": ReadPipe is invalid for stdin")); + + // At this point, slot must be STDOUT or STDERR + // Check if a pipe was configured for this slot + if (slot == STDOUT) { - error = STRINGIZE(mDesc << " has no slot " << slot); - return NULL; + if (!mStdoutReadPipe) + throw NoPipe(STRINGIZE(mDesc << ": stdout is not a monitored pipe")); + return *mStdoutReadPipe; } - if (!mPipes[slot]) + else if (slot == STDERR) { - error = STRINGIZE(mDesc << ' ' << whichfile(slot) << " not a monitored pipe"); - return NULL; + if (!mStderrReadPipe) + throw NoPipe(STRINGIZE(mDesc << ": stderr is not a monitored pipe")); + return *mStderrReadPipe; } - // Make sure we dynamic_cast in pointer domain so we can test, rather than - // accepting runtime's exception. - PIPETYPE* ppipe = dynamic_cast(mPipes[slot].get()); - if (! ppipe) + else { - error = STRINGIZE(mDesc << ' ' << whichfile(slot) << " not a " << typeid(PIPETYPE).name()); - return NULL; + // This should never happen given the checks above, but handle it anyway + throw NoPipe(STRINGIZE(mDesc << ": no slot " << slot)); } - - error.clear(); - return ppipe; } -template -PIPETYPE& LLProcess::getPipe(FILESLOT slot) +LLProcess::WritePipe* LLProcess::getOptWritePipe(FILESLOT slot) { - std::string error; - PIPETYPE* wp = getPipePtr(error, slot); - if (! wp) + if (slot != STDIN) { - LLTHROW(NoPipe(error)); + LL_WARNS("LLProcess") << mDesc << ": no slot " << slot << LL_ENDL; + return nullptr; } - return *wp; -} -template -boost::optional LLProcess::getOptPipe(FILESLOT slot) -{ - std::string error; - PIPETYPE* wp = getPipePtr(error, slot); - if (! wp) + if (!mWritePipe) { - LL_DEBUGS("LLProcess") << error << LL_ENDL; - return boost::optional(); + LL_WARNS("LLProcess") << mDesc << ": stdin is not a monitored pipe" << LL_ENDL; + return nullptr; } - return *wp; -} -LLProcess::WritePipe& LLProcess::getWritePipe(FILESLOT slot) -{ - return getPipe(slot); + return mWritePipe.get(); } -boost::optional LLProcess::getOptWritePipe(FILESLOT slot) +LLProcess::ReadPipe* LLProcess::getOptReadPipe(FILESLOT slot) { - return getOptPipe(slot); -} + if (slot >= NSLOTS) + { + LL_WARNS("LLProcess") << mDesc << ": no slot " << slot << LL_ENDL; + return nullptr; + } -LLProcess::ReadPipe& LLProcess::getReadPipe(FILESLOT slot) -{ - return getPipe(slot); + if (slot == STDIN) + { + LL_WARNS("LLProcess") << mDesc << ": ReadPipe is invalid for stdin" << LL_ENDL; + return nullptr; + } + + if (slot == STDOUT) + { + if (!mStdoutReadPipe) + { + LL_WARNS("LLProcess") << mDesc << ": stdout is not a monitored pipe" << LL_ENDL; + return nullptr; + } + return mStdoutReadPipe.get(); + } + else if (slot == STDERR) + { + if (!mStderrReadPipe) + { + LL_WARNS("LLProcess") << mDesc << ": stderr is not a monitored pipe" << LL_ENDL; + return nullptr; + } + return mStderrReadPipe.get(); + } + else + { + LL_WARNS("LLProcess") << mDesc << ": no slot " << slot << LL_ENDL; + return nullptr; + } } -boost::optional LLProcess::getOptReadPipe(FILESLOT slot) +//static +std::string LLProcess::basename(const std::string& path) { - return getOptPipe(slot); + std::string::size_type delim = path.find_last_of("\\/"); + if (delim == std::string::npos) + return path; + return path.substr(delim + 1); } //static @@ -1120,44 +1141,19 @@ 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); - } + // Trim trailing \r for cross-platform compatibility + if (!line.empty() && line.back() == '\r') + line.pop_back(); return line; } -std::ostream& operator<<(std::ostream& out, const LLProcess::Params& params) -{ - if (params.cwd.isProvided()) - { - out << "cd " << LLStringUtil::quote(params.cwd) << ": "; - } - out << LLStringUtil::quote(params.executable); - for (const std::string& arg : params.args) - { - out << ' ' << LLStringUtil::quote(arg); - } - return out; -} - /***************************************************************************** * Windows specific *****************************************************************************/ #if LL_WINDOWS - +/* static std::string WindowsErrorString(const std::string& operation); -void LLProcess::autokill() -{ - // hopefully now handled by apr_procattr_autokill_set() -} - LLProcess::handle LLProcess::isRunning(handle h, const std::string& desc) { // This direct Windows implementation is because we have no access to the @@ -1197,7 +1193,7 @@ static LLProcess::Status interpret_status(int status) // 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 = LLProcess::KILLED; @@ -1218,12 +1214,12 @@ static std::string WindowsErrorString(const std::string& operation) return STRINGIZE(operation << " failed (" << result << "): " << windows_message(result)); } - +*/ /***************************************************************************** * Posix specific *****************************************************************************/ #else // Mac and linux - +/* #include #include #include @@ -1342,6 +1338,6 @@ static LLProcess::Status interpret_status(int status) } return result; -} +}*/ #endif // Posix diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 0c71cfc415..bcd1475f1c 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -31,6 +31,15 @@ #include "llsdparam.h" #include "llexception.h" #include "apr_thread_proc.h" +#include +#include +#include +#include +#include +#include +#include +#include +#include #include #include // std::ostream @@ -65,100 +74,38 @@ typedef std::shared_ptr LLProcessPtr; * 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 { LOG_CLASS(LLProcess); public: /** * Specify what to pass for each of child stdin, stdout, stderr. - * @see LLProcess::Params::files. */ - struct FileParam: public LLInitParam::Block + 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 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 - * 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. + * - "" (default): inherit from parent + * - "pipe": create a pipe for I/O + * - "file": open a filesystem file (future enhancement) */ Optional type; Optional name; - FileParam(const std::string& tp="", const std::string& nm=""): + FileParam(const std::string& tp = "", const std::string& 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; + if (!tp.empty()) type = tp; + if (!nm.empty()) name = nm; } }; /// Param block definition - struct Params: public LLInitParam::Block + struct Params : public LLInitParam::Block { - Params(): + Params() : executable("executable"), args("args"), cwd("cwd"), @@ -167,100 +114,26 @@ class LL_COMMON_API LLProcess files("files"), postend("postend"), desc("desc") - {} + { + } - /// pathname of executable Mandatory executable; - /** - * 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. - */ Multiple args; - /// current working directory, if need it changed Optional cwd; - /// implicitly kill child process on termination of parent, whether - /// voluntary or crash (default true) Optional autokill; - /// implicitly kill process on destruction of LLProcess object - /// (default same as autokill) - /// - /// Originally, 'autokill' conflated two concepts: kill child process on - /// - destruction of its LLProcess object, and - /// - termination of parent process, voluntary or otherwise. - /// - /// It's useful to tease these apart. Some child processes are sent a - /// "clean up and terminate" message before the associated LLProcess - /// object is destroyed. A child process launched with attached=false - /// has an extra time window from the destruction of its LLProcess - /// until parent-process termination in which to perform its own - /// orderly shutdown, yet autokill=true still guarantees that we won't - /// accumulate orphan instances of such processes indefinitely. With - /// attached=true, if a child process cannot clean up between the - /// shutdown message and LLProcess destruction (presumably very soon - /// thereafter), it's forcibly killed anyway -- which can lead to - /// distressing user-visible crash indications. - /// - /// (The usefulness of attached=true with autokill=false is less - /// clear, but we don't prohibit that combination.) Optional attached; - /** - * 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 doesn't yet 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") - */ + Multiple> files; 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; - /** - * Factory accepting either plain LLSD::Map or Params block. - * MAY RETURN DEFAULT-CONSTRUCTED LLProcessPtr if params invalid! - */ static LLProcessPtr create(const LLSDOrParams& params); virtual ~LLProcess(); /// 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&); /** @@ -268,10 +141,10 @@ class LL_COMMON_API LLProcess */ enum state { - UNSTARTED, ///< initial value, invisible to consumer - RUNNING, ///< child process launched - EXITED, ///< child process terminated voluntarily - KILLED ///< child process terminated involuntarily + UNSTARTED, + RUNNING, + EXITED, + KILLED }; /** @@ -279,296 +152,123 @@ class LL_COMMON_API LLProcess */ 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() : mState(UNSTARTED), mData(0) {} + state mState; + int mData; // exit code or signal number }; - /// 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 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(const std::string& who=""); - static bool kill(const LLProcessPtr& p, const std::string& who=""); + 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() - typedef HANDLE handle; ///< as returned by getProcessHandle() + typedef int id; + typedef HANDLE handle; #else typedef pid_t id; typedef pid_t handle; #endif - /** - * 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; + static handle isRunning(handle, const std::string& desc = ""); - /** - * 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 - * 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 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 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. This static isRunning() is only intended for after the - * launching LLProcess object has been destroyed. - */ - static handle isRunning(handle, const std::string& desc=""); + enum FILESLOT { STDIN = 0, STDOUT = 1, STDERR = 2, NSLOTS = 3 }; - /// Provide symbolic access to child's file slots - enum FILESLOT { STDIN=0, STDOUT=1, STDERR=2, NSLOTS=3 }; + /// Exception thrown by getWritePipe(), getReadPipe() if you didn't ask to + /// create a pipe at the corresponding FILESLOT. + struct NoPipe : public LLException + { + NoPipe(const std::string& what) : LLException(what) {} + }; - /** - * 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) const; - /// base of ReadPipe, WritePipe + /// Base class for pipes class LL_COMMON_API BasePipe { public: - virtual ~BasePipe() = 0; - + virtual ~BasePipe() = default; 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() - class WritePipe: public BasePipe + /// Write pipe for stdin + 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 + /// Read pipe for stdout/stderr + 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; - - /** - * 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; + virtual std::string peek(size_type offset = 0, size_type len = npos) const = 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(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; + bool contains(SEEK seek, size_type offset = 0) const + { + return find(seek, offset) != npos; + } - /** - * 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 size_type find(char seek, size_type offset=0) const = 0; + virtual size_type find(const std::string& seek, size_type offset = 0) const = 0; + virtual size_type find(char seek, size_type offset = 0) const = 0; - /** - * Get LLEventPump& on which to listen for incoming data. The posted - * 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 - * 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, - * @em no data will be posted: the default is 0 bytes. - */ virtual void setLimit(size_type limit) = 0; - - /** - * Query the current setLimit() limit. - */ virtual size_type getLimit() const = 0; }; - /// Exception thrown by getWritePipe(), getReadPipe() if you didn't ask to - /// create a pipe at the corresponding FILESLOT. - struct NoPipe: public LLException - { - NoPipe(const std::string& what): LLException(what) {} - }; + WritePipe& getWritePipe(FILESLOT slot = STDIN); + ReadPipe& getReadPipe(FILESLOT index); + WritePipe* getOptWritePipe(FILESLOT slot = STDIN); + ReadPipe* getOptReadPipe(FILESLOT index); - /** - * 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); + static std::string basename(const std::string& path); + static std::string getline(std::istream& in); - /** - * 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); + // Constructor is public for the sake of make_shared + // but create() should be used instead for proper initialization. + LLProcess(const Params& params); - /** - * 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: + void launch(const Params& params); + void tick(); + void handleExit(int exit_code); - /// 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&); + // Boost.Process components + boost::asio::io_context mIOContext; + std::unique_ptr mChild; - // Non-copyable - LLProcess(const LLProcess&) = delete; - LLProcess& operator=(const LLProcess&) = delete; + // Pipes - using Boost.Process async pipes + std::unique_ptr mStdinPipe; + std::unique_ptr mStdoutPipe; + std::unique_ptr mStderrPipe; -private: - /// constructor is private: use create() instead - LLProcess(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); - // 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); + // Our pipe wrapper implementations + std::unique_ptr mWritePipe; + std::unique_ptr mStdoutReadPipe; + std::unique_ptr mStderrReadPipe; + Status mStatus; std::string mDesc; std::string mPostend; - apr_proc_t mProcess; - bool mAutokill, mAttached; - Status mStatus; - // explicitly want this ptr_vector to be able to store NULLs - typedef std::vector> PipeVector; - PipeVector mPipes; - apr_pool_t* mPool; + bool mAutokill; + bool mAttached; + + // For integrating with LLEventPump mainloop + boost::signals2::scoped_connection mMainloopConnection; }; /// for logging From 202b4b97e83c339e26cb5079e54df404e1d5c96f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 06:53:22 +0000 Subject: [PATCH 02/36] Fix macOS CI failures: SIGPIPE and EINTR in LLProcess Two root causes identified from the failed job (build macos-15-xlarge, Release): 1. INTEGRATION_TEST_llleap: "Error -13: terminated by signal SIGPIPE" The original APR-based code called signal(SIGPIPE, SIG_IGN) to prevent the viewer from being killed when a child closes its stdin while we write. This call was lost during the rewrite. Restore it in launch(). 2. INTEGRATION_TEST_llsdserialize/llprocess: "waitpid() errno 4 (EINTR)" Passing mIOContext to bp::child caused boost::process v1 to register a SIGCHLD handler via boost::asio without SA_RESTART. That handler interrupted blocking waitpid() calls in the test harness. Fix: do not pass mIOContext to bp::child constructors; child exit is detected by our own WNOHANG waitpid() in tick() instead. Additional cleanup: - Remove the duplicate waitpid() in handleExit(); tick() already reaps the child and decodes the full status (EXITED vs KILLED) before calling it. - Use mStatus.mState instead of mChild->running() in isRunning(), kill(), destructor, and the Windows tick() path to avoid competing waitpid calls. --- indra/llcommon/llprocess.cpp | 105 +++++++++++++++-------------------- indra/llcommon/llprocess.h | 2 +- 2 files changed, 45 insertions(+), 62 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 105bc315bf..0fd5deea4f 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -440,7 +440,7 @@ LLProcess::LLProcess(const Params& params) : LLProcess::~LLProcess() { - if (mChild && mChild->running()) + if (mChild && mStatus.mState == RUNNING) { if (mAttached && mAutokill) { @@ -592,7 +592,17 @@ void LLProcess::launch(const Params& params) { std::error_code ec; - // Create child process with appropriate redirections +#if !LL_WINDOWS + // Ignore SIGPIPE so that writing to a child's closed stdin doesn't + // terminate the viewer process. The write will fail with EPIPE instead. + signal(SIGPIPE, SIG_IGN); +#endif + + // Create child process with appropriate redirections. + // Do NOT pass mIOContext to bp::child: boost::process v1 would then + // install a SIGCHLD handler via boost::asio without SA_RESTART, which + // causes blocking waitpid() calls elsewhere to return EINTR. We detect + // child exit ourselves via waitpid(WNOHANG) in tick() instead. if (use_stdin_pipe && use_stdout_pipe && use_stderr_pipe) { if (params.cwd.isProvided()) @@ -604,7 +614,6 @@ void LLProcess::launch(const Params& params) bp::std_out > *mStdoutPipe, bp::std_err > *mStderrPipe, bp::start_dir(params.cwd()), - mIOContext, ec ); } @@ -616,7 +625,6 @@ void LLProcess::launch(const Params& params) bp::std_in < *mStdinPipe, bp::std_out > *mStdoutPipe, bp::std_err > *mStderrPipe, - mIOContext, ec ); } @@ -631,7 +639,6 @@ void LLProcess::launch(const Params& params) bp::std_out > *mStdoutPipe, bp::std_err > *mStderrPipe, bp::start_dir(params.cwd()), - mIOContext, ec ); } @@ -642,7 +649,6 @@ void LLProcess::launch(const Params& params) bp::args(args), bp::std_out > *mStdoutPipe, bp::std_err > *mStderrPipe, - mIOContext, ec ); } @@ -656,7 +662,6 @@ void LLProcess::launch(const Params& params) bp::args(args), bp::std_out > *mStdoutPipe, bp::start_dir(params.cwd()), - mIOContext, ec ); } @@ -666,7 +671,6 @@ void LLProcess::launch(const Params& params) params.executable(), bp::args(args), bp::std_out > *mStdoutPipe, - mIOContext, ec ); } @@ -679,7 +683,6 @@ void LLProcess::launch(const Params& params) params.executable(), bp::args(args), bp::start_dir(params.cwd()), - mIOContext, ec ); } @@ -688,7 +691,6 @@ void LLProcess::launch(const Params& params) mChild = std::make_unique( params.executable(), bp::args(args), - mIOContext, ec ); } @@ -754,11 +756,13 @@ void LLProcess::tick() #if LL_WINDOWS // Check process status - if (mChild && !mChild->running()) + if (mChild && mStatus.mState == RUNNING && !mChild->running()) { WaitForSingleObject(mChild->native_handle(), 100); - int exit_code = mChild->exit_code(); - handleExit(exit_code); + Status exitStatus; + exitStatus.mState = EXITED; + exitStatus.mData = mChild->exit_code(); + handleExit(exitStatus); } #else // Check process status using WNOHANG to avoid blocking or generating @@ -773,67 +777,46 @@ void LLProcess::tick() if (result == mChild->id()) { - // Child has exited; decode status manually - int exit_code = 0; + // Child has exited; decode exit status now (before handleExit, + // since the child has already been reaped by this waitpid call). + Status exitStatus; if (WIFEXITED(status)) - exit_code = WEXITSTATUS(status); - handleExit(exit_code); + { + exitStatus.mState = EXITED; + exitStatus.mData = WEXITSTATUS(status); + } + else if (WIFSIGNALED(status)) + { + exitStatus.mState = KILLED; + exitStatus.mData = WTERMSIG(status); + } + else + { + exitStatus.mState = EXITED; + exitStatus.mData = 0; + } + handleExit(exitStatus); } // result == 0 means still running; result == -1/ECHILD means already reaped } #endif } -void LLProcess::handleExit(int exit_code) +void LLProcess::handleExit(Status exitStatus) { if (mStatus.mState != RUNNING) return; // Already handled - // Before handling exit, drain any remaining data from pipes - // This is important because the child might have written data just before exiting + mStatus = exitStatus; + + // Drain any remaining data from pipes before notifying callers for (int i = 0; i < 10; ++i) { if (mIOContext.poll_one() == 0) - break; // No more work to do - } - -#if LL_WINDOWS - // On Windows, all terminations result in EXITED state - mStatus.mState = EXITED; - mStatus.mData = exit_code; -#else - // On POSIX, need to check if process was signaled - int status; - pid_t result = waitpid(mChild->id(), &status, WNOHANG); - - if (result == mChild->id()) - { - if (WIFEXITED(status)) - { - mStatus.mState = EXITED; - mStatus.mData = WEXITSTATUS(status); - } - else if (WIFSIGNALED(status)) - { - mStatus.mState = KILLED; - mStatus.mData = WTERMSIG(status); - } - else - { - // Shouldn't happen, but treat as exit - mStatus.mState = EXITED; - mStatus.mData = exit_code; - } + break; } - else - { - // Couldn't get detailed status, use what boost::process gives us - mStatus.mState = EXITED; - mStatus.mData = exit_code; - } -#endif - LL_INFOS("LLProcess") << mDesc << " exited with code " << exit_code << LL_ENDL; + LL_INFOS("LLProcess") << mDesc << " " << getStatusString(mStatus) << LL_ENDL; // Post to event pump if configured if (!mPostend.empty()) @@ -842,7 +825,7 @@ void LLProcess::handleExit(int exit_code) event["id"] = static_cast(getProcessID()); event["desc"] = mDesc; event["state"] = mStatus.mState; - event["data"] = exit_code; + event["data"] = mStatus.mData; event["string"] = getStatusString(mStatus); LLEventPumps::instance().obtain(mPostend).post(event); @@ -857,7 +840,7 @@ void LLProcess::handleExit(int exit_code) bool LLProcess::isRunning() const { - return mChild && mChild->running(); + return mStatus.mState == RUNNING; } //static @@ -915,7 +898,7 @@ std::string LLProcess::getStatusString(const std::string& desc, const Status& st bool LLProcess::kill(const std::string& who) { - if (!mChild || !mChild->running()) + if (!mChild || mStatus.mState != RUNNING) return true; LL_INFOS("LLProcess") << who << " killing " << mDesc << LL_ENDL; diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index bcd1475f1c..e75ed954eb 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -245,7 +245,7 @@ class LL_COMMON_API LLProcess private: void launch(const Params& params); void tick(); - void handleExit(int exit_code); + void handleExit(Status exitStatus); // Boost.Process components boost::asio::io_context mIOContext; From e5398dcd97151d68f7d80f7da7662d6bc2ef9527 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 06:54:47 +0000 Subject: [PATCH 03/36] Fix destructor: use waitpid(WNOHANG) instead of mChild->running() Replace the mChild->running() calls in the POSIX destructor path with direct waitpid(WNOHANG) calls. This avoids competing with tick()'s own waitpid polling and is consistent with how the rest of the class tracks process exit status. --- indra/llcommon/llprocess.cpp | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 0fd5deea4f..f8b37470df 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -449,18 +449,26 @@ LLProcess::~LLProcess() mChild->terminate(ec); #if !LL_WINDOWS - // On POSIX, terminate() sends SIGTERM which allows graceful shutdown - // Give the process some time to terminate gracefully - for (int i = 0; i < 30 && mChild->running(); ++i) + // On POSIX, terminate() sends SIGTERM which allows graceful shutdown. + // Poll with waitpid(WNOHANG) rather than mChild->running() to avoid + // competing with tick()'s own waitpid call. + pid_t pid = mChild->id(); + for (int i = 0; i < 30; ++i) { + int child_status; + if (::waitpid(pid, &child_status, WNOHANG) == pid) + break; // child exited std::this_thread::sleep_for(std::chrono::milliseconds(100)); } // Force kill if still running - if (mChild->running()) { - LL_WARNS("LLProcess") << "Force killing " << mDesc << LL_ENDL; - (void)::kill(mChild->id(), SIGKILL); + int child_status; + if (::waitpid(pid, &child_status, WNOHANG) == 0) + { + LL_WARNS("LLProcess") << "Force killing " << mDesc << LL_ENDL; + (void)::kill(pid, SIGKILL); + } } #else // On Windows, terminate() already does an immediate hard kill via TerminateProcess() From f08be9baf650f8481fa01d5c2c0dbd157854f29b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 08:07:20 +0000 Subject: [PATCH 04/36] Fix 8 failing INTEGRATION_TEST_llprocess test cases - launch(): replace 4-branch if/else with generic forwarding lambda covering all 8 pipe combinations (stdin, stdout, stderr) - fixes tests 6 (stderr-only pipe), 17 and 18 (stdin+stdout pipe) - ~LLProcess(): call mChild->detach() in non-autokill branch so that bp::child::~child() does not send SIGKILL to surviving process - fixes tests 9 and 10 (autokill=false / attached=false) - create(): fire postend event with state=UNSTARTED on launch failure - fixes test 22 (bad postend) - kill(): on POSIX use ::kill(pid, SIGTERM) instead of mChild->terminate() which sends SIGKILL and may corrupt the stored pid - fixes test 7 (explicit kill expects SIGTERM) - tick(): handle ECHILD from waitpid (zombie reaped externally) by synthesising EXITED/0, preventing 60-second waitfor() timeouts - getWritePipe() / getOptWritePipe(): distinguish "not piped" from "piped but wrong direction" for STDOUT/STDERR slots - fixes test 16 (get*Pipe() validation error message checks) --- indra/llcommon/llprocess.cpp | 193 +++++++++++++++++++---------------- 1 file changed, 103 insertions(+), 90 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index f8b37470df..8692cd0363 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -481,6 +481,10 @@ LLProcess::~LLProcess() LL_INFOS("LLProcess") << "Not terminating " << mDesc << " (attached=" << mAttached << ", autokill=" << mAutokill << ")" << LL_ENDL; + // Detach the bp::child so its destructor does not send SIGKILL + // to the still-running process (boost::process v1 terminates the + // child in bp::child::~child() if the handle is still valid). + mChild->detach(); } } @@ -500,6 +504,20 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params) catch (const std::exception& e) { LL_WARNS("LLProcess") << "Failed to create process: " << e.what() << LL_ENDL; + + // Even on failure, fire the postend event if requested, so callers + // that listen for it can detect the launch failure. + if (params.postend.isProvided() && !params.postend().empty()) + { + std::string desc = params.desc.isProvided() ? params.desc() : + (params.executable.isProvided() ? LLProcess::basename(params.executable()) : ""); + LLSD event; + event["desc"] = desc; + event["state"] = LLProcess::UNSTARTED; + event["string"] = e.what(); + LLEventPumps::instance().obtain(params.postend()).post(event); + } + return LLProcessPtr(); } } @@ -611,98 +629,48 @@ void LLProcess::launch(const Params& params) // install a SIGCHLD handler via boost::asio without SA_RESTART, which // causes blocking waitpid() calls elsewhere to return EINTR. We detect // child exit ourselves via waitpid(WNOHANG) in tick() instead. - if (use_stdin_pipe && use_stdout_pipe && use_stderr_pipe) - { + // + // Use a generic lambda to avoid repeating the executable/args/cwd for + // each of the 8 pipe-combination branches. + auto make_child = [&](auto&&... redirects) { if (params.cwd.isProvided()) - { mChild = std::make_unique( params.executable(), bp::args(args), - bp::std_in < *mStdinPipe, - bp::std_out > *mStdoutPipe, - bp::std_err > *mStderrPipe, bp::start_dir(params.cwd()), + std::forward(redirects)..., ec ); - } else - { mChild = std::make_unique( params.executable(), bp::args(args), - bp::std_in < *mStdinPipe, - bp::std_out > *mStdoutPipe, - bp::std_err > *mStderrPipe, + std::forward(redirects)..., ec ); - } - } + }; + + if (use_stdin_pipe && use_stdout_pipe && use_stderr_pipe) + make_child(bp::std_in < *mStdinPipe, + bp::std_out > *mStdoutPipe, + bp::std_err > *mStderrPipe); + else if (use_stdin_pipe && use_stdout_pipe) + make_child(bp::std_in < *mStdinPipe, + bp::std_out > *mStdoutPipe); + else if (use_stdin_pipe && use_stderr_pipe) + make_child(bp::std_in < *mStdinPipe, + bp::std_err > *mStderrPipe); else if (use_stdout_pipe && use_stderr_pipe) - { - if (params.cwd.isProvided()) - { - mChild = std::make_unique( - params.executable(), - bp::args(args), - bp::std_out > *mStdoutPipe, - bp::std_err > *mStderrPipe, - bp::start_dir(params.cwd()), - ec - ); - } - else - { - mChild = std::make_unique( - params.executable(), - bp::args(args), - bp::std_out > *mStdoutPipe, - bp::std_err > *mStderrPipe, - ec - ); - } - } + make_child(bp::std_out > *mStdoutPipe, + bp::std_err > *mStderrPipe); + else if (use_stdin_pipe) + make_child(bp::std_in < *mStdinPipe); else if (use_stdout_pipe) - { - if (params.cwd.isProvided()) - { - mChild = std::make_unique( - params.executable(), - bp::args(args), - bp::std_out > *mStdoutPipe, - bp::start_dir(params.cwd()), - ec - ); - } - else - { - mChild = std::make_unique( - params.executable(), - bp::args(args), - bp::std_out > *mStdoutPipe, - ec - ); - } - } + make_child(bp::std_out > *mStdoutPipe); + else if (use_stderr_pipe) + make_child(bp::std_err > *mStderrPipe); else - { - if (params.cwd.isProvided()) - { - mChild = std::make_unique( - params.executable(), - bp::args(args), - bp::start_dir(params.cwd()), - ec - ); - } - else - { - mChild = std::make_unique( - params.executable(), - bp::args(args), - ec - ); - } - } + make_child(); if (ec) { @@ -805,7 +773,18 @@ void LLProcess::tick() } handleExit(exitStatus); } - // result == 0 means still running; result == -1/ECHILD means already reaped + else if (result == -1 && errno == ECHILD) + { + // The zombie was already reaped by someone else (e.g. a SIGCHLD + // handler from APR or another library). We can't determine the + // real exit code; synthesize EXITED/0 so the process is no longer + // considered "running" and waitfor() doesn't spin for 60 seconds. + Status exitStatus; + exitStatus.mState = EXITED; + exitStatus.mData = 0; + handleExit(exitStatus); + } + // result == 0 means still running } #endif } @@ -911,6 +890,7 @@ bool LLProcess::kill(const std::string& who) LL_INFOS("LLProcess") << who << " killing " << mDesc << LL_ENDL; +#if LL_WINDOWS std::error_code ec; mChild->terminate(ec); @@ -920,12 +900,23 @@ bool LLProcess::kill(const std::string& who) << ": " << ec.message() << LL_ENDL; return false; } +#else + // Send SIGTERM so the child can clean up gracefully, and so tick()'s + // waitpid() reports WIFSIGNALED/WTERMSIG == SIGTERM rather than SIGKILL. + // Do NOT call mChild->terminate(): in boost::process v1, that function + // sends SIGKILL and may set the internal pid to -1, which would cause + // tick()'s waitpid(mChild->id(), ...) to wait for any child (-1) and + // never match the result against the stored pid. + pid_t pid = mChild->id(); + if (::kill(pid, SIGTERM) != 0 && errno != ESRCH) + { + LL_WARNS("LLProcess") << "Failed to send SIGTERM to " << mDesc + << " (pid " << pid << "): " << strerror(errno) << LL_ENDL; + return false; + } +#endif // Don't set status here - let handleExit() do it when the process actually terminates - // The state will be determined by how the process exited: - // - Windows: EXITED with exit code -1 - // - POSIX: KILLED with signal SIGTERM (or SIGKILL) - return true; } @@ -1024,13 +1015,24 @@ std::string LLProcess::getPipeName(FILESLOT slot) const LLProcess::WritePipe& LLProcess::getWritePipe(FILESLOT slot) { - if (slot != STDIN) + if (slot >= NSLOTS) throw NoPipe(STRINGIZE(mDesc << ": no slot " << slot)); - if (!mWritePipe) - throw NoPipe(STRINGIZE(mDesc << ": stdin is not a monitored pipe")); + if (slot == STDIN) + { + if (!mWritePipe) + throw NoPipe(STRINGIZE(mDesc << ": stdin is not a monitored pipe")); + return *mWritePipe; + } - return *mWritePipe; + // STDOUT or STDERR slots: neither has a WritePipe. + // Distinguish "not piped at all" from "piped but wrong direction". + const char* slotname = (slot == STDOUT) ? "stdout" : "stderr"; + bool is_piped = (slot == STDOUT) ? bool(mStdoutReadPipe) : bool(mStderrReadPipe); + if (!is_piped) + throw NoPipe(STRINGIZE(mDesc << ": " << slotname << " is not a monitored pipe")); + else + throw NoPipe(STRINGIZE(mDesc << ": " << slotname << " is a ReadPipe, not a WritePipe")); } LLProcess::ReadPipe& LLProcess::getReadPipe(FILESLOT slot) @@ -1064,19 +1066,30 @@ LLProcess::ReadPipe& LLProcess::getReadPipe(FILESLOT slot) LLProcess::WritePipe* LLProcess::getOptWritePipe(FILESLOT slot) { - if (slot != STDIN) + if (slot >= NSLOTS) { LL_WARNS("LLProcess") << mDesc << ": no slot " << slot << LL_ENDL; return nullptr; } - if (!mWritePipe) + if (slot == STDIN) { - LL_WARNS("LLProcess") << mDesc << ": stdin is not a monitored pipe" << LL_ENDL; - return nullptr; + if (!mWritePipe) + { + LL_WARNS("LLProcess") << mDesc << ": stdin is not a monitored pipe" << LL_ENDL; + return nullptr; + } + return mWritePipe.get(); } - return mWritePipe.get(); + // STDOUT or STDERR slots: neither has a WritePipe. + const char* slotname = (slot == STDOUT) ? "stdout" : "stderr"; + bool is_piped = (slot == STDOUT) ? bool(mStdoutReadPipe) : bool(mStderrReadPipe); + if (!is_piped) + LL_WARNS("LLProcess") << mDesc << ": " << slotname << " is not a monitored pipe" << LL_ENDL; + else + LL_WARNS("LLProcess") << mDesc << ": " << slotname << " is a ReadPipe, not a WritePipe" << LL_ENDL; + return nullptr; } LLProcess::ReadPipe* LLProcess::getOptReadPipe(FILESLOT slot) From b50b88a90296fc07a3d385f67f4e05685fadca3f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 12:52:21 +0000 Subject: [PATCH 05/36] Drain all ready LLProcess pipe events on exit --- indra/llcommon/llprocess.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 8692cd0363..6eb6b39a22 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -796,11 +796,13 @@ void LLProcess::handleExit(Status exitStatus) mStatus = exitStatus; - // Drain any remaining data from pipes before notifying callers - for (int i = 0; i < 10; ++i) + // Drain any remaining pipe handlers before notifying callers. LLLeap + // consumes child stdout/stderr from these callbacks, and some tests write + // enough data to require far more than a handful of async-read completions + // after the child has already exited. + while (mIOContext.poll_one() > 0) { - if (mIOContext.poll_one() == 0) - break; + // Keep polling until no more handlers are immediately ready. } LL_INFOS("LLProcess") << mDesc << " " << getStatusString(mStatus) << LL_ENDL; From 4f39c6928b36208e71f798f51045aef967fc7631 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 15:42:22 +0000 Subject: [PATCH 06/36] Fix WritePipeImpl to use self-chaining startAsyncWrite --- indra/llcommon/llprocess.cpp | 45 +++++++++++------------------------- 1 file changed, 14 insertions(+), 31 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 6eb6b39a22..78fa85ac29 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -172,10 +172,7 @@ class WritePipeImpl : public LLProcess::WritePipe mPipe(pipe), mStream(&mStreambuf), mWritePending(false) - { - // Start async write monitoring - startAsyncWrite(); - } + {} virtual ~WritePipeImpl() = default; @@ -186,50 +183,36 @@ class WritePipeImpl : public LLProcess::WritePipe return mStreambuf.size(); } + // Called from LLProcess::tick() to initiate writing buffered data. + // Self-chains: each completed write immediately starts the next one if + // more data is waiting, so large transfers don't stall between ticks. void tick() { - // Don't start a new write if one is already pending - if (mWritePending || !mPipe || !mPipe->is_open() || mStreambuf.size() == 0) - return; - - mWritePending = true; - - // Write buffered data asynchronously - asio::async_write(*mPipe, mStreambuf.data(), - [this](const boost::system::error_code& ec, std::size_t bytes_transferred) - { - mWritePending = false; - - if (!ec) - { - mStreambuf.consume(bytes_transferred); - LL_DEBUGS("LLProcess") << "Wrote " << bytes_transferred - << " bytes to " << mDesc << LL_ENDL; - } - else if (ec != asio::error::operation_aborted) - { - LL_WARNS("LLProcess") << "Write error on " << mDesc - << ": " << ec.message() << LL_ENDL; - } - }); + startAsyncWrite(); } private: void startAsyncWrite() { - if (!mPipe || !mPipe->is_open() || mStreambuf.size() == 0) + if (mWritePending || !mPipe || !mPipe->is_open() || mStreambuf.size() == 0) return; - // Write buffered data asynchronously + mWritePending = true; + + // Write all buffered data asynchronously, then chain to the next write + // if more data was queued while this one was in flight. asio::async_write(*mPipe, mStreambuf.data(), [this](const boost::system::error_code& ec, std::size_t bytes_transferred) { + mWritePending = false; + if (!ec) { mStreambuf.consume(bytes_transferred); LL_DEBUGS("LLProcess") << "Wrote " << bytes_transferred << " bytes to " << mDesc << LL_ENDL; - // Continue writing if there's more data + // If wstdin() queued more data while we were writing, send it + // immediately instead of waiting for the next mainloop tick. startAsyncWrite(); } else if (ec != asio::error::operation_aborted) From 62e78ea7ca7f1228d001bdb75686b1136337c982 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 19:23:02 +0000 Subject: [PATCH 07/36] Pump LLProcess writes before poll loop --- indra/llcommon/llprocess.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 78fa85ac29..9057612aca 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -704,15 +704,18 @@ void LLProcess::launch(const Params& params) void LLProcess::tick() { - // Poll I/O context to process async operations + if (auto* wp = dynamic_cast(mWritePipe.get())) + wp->tick(); + + // Poll I/O context to process async operations. Kick off any pending stdin + // write before entering this loop so composed async_write operations can + // advance through as many immediately-ready pipe events as possible during + // the current mainloop tick instead of waiting for the next one. while (mIOContext.poll_one() > 0) { // Keep polling until no more handlers are ready } - if (auto* wp = dynamic_cast(mWritePipe.get())) - wp->tick(); - #if LL_WINDOWS // Check process status if (mChild && mStatus.mState == RUNNING && !mChild->running()) From 65c6893a4823c613a6cfcbaeb7a526c9a7bdef0a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:33:23 +0000 Subject: [PATCH 08/36] Fix LLProcess tick ordering to prevent LLLeap timeout --- indra/llcommon/llprocess.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 9057612aca..63f168f8ab 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -704,18 +704,15 @@ void LLProcess::launch(const Params& params) void LLProcess::tick() { - if (auto* wp = dynamic_cast(mWritePipe.get())) - wp->tick(); - - // Poll I/O context to process async operations. Kick off any pending stdin - // write before entering this loop so composed async_write operations can - // advance through as many immediately-ready pipe events as possible during - // the current mainloop tick instead of waiting for the next one. + // Poll I/O context to process async operations. while (mIOContext.poll_one() > 0) { // Keep polling until no more handlers are ready } + if (auto* wp = dynamic_cast(mWritePipe.get())) + wp->tick(); + #if LL_WINDOWS // Check process status if (mChild && mStatus.mState == RUNNING && !mChild->running()) From 9068cd507259e2395518466ff1eac425cb0bffe4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:34:09 +0000 Subject: [PATCH 09/36] Clarify LLProcess tick ordering rationale --- indra/llcommon/llprocess.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 63f168f8ab..a62b8cc6f4 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -704,7 +704,9 @@ void LLProcess::launch(const Params& params) void LLProcess::tick() { - // Poll I/O context to process async operations. + // Poll I/O context before scheduling the next stdin write so pending read + // callbacks can drain first; this avoids LLLeap large-message stalls where + // a child process waits on output consumption before accepting more input. while (mIOContext.poll_one() > 0) { // Keep polling until no more handlers are ready From e66e107c26c48c0654d30425d2f04724f526b470 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Jul 2026 23:34:50 +0000 Subject: [PATCH 10/36] Tighten LLProcess tick comment wording --- indra/llcommon/llprocess.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index a62b8cc6f4..007b26ce72 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -704,7 +704,7 @@ void LLProcess::launch(const Params& params) void LLProcess::tick() { - // Poll I/O context before scheduling the next stdin write so pending read + // Poll I/O context before initiating stdin writes so pending read // callbacks can drain first; this avoids LLLeap large-message stalls where // a child process waits on output consumption before accepting more input. while (mIOContext.poll_one() > 0) From dfb763c6b8329809a8e70c0b30170d74f22c4d64 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Jul 2026 12:49:03 +0000 Subject: [PATCH 11/36] Fix ReadPipeImpl mLimit backpressure causing INTEGRATION_TEST_llleap pipe deadlock startAsyncRead() was stopping async reads when mStreambuf.size() >= mLimit (20 bytes for LLLeap). This filled the OS pipe buffer and blocked the child process, preventing large (~1 MB) messages from being fully received and causing the llleap test to time out. - Remove mLimit check from startAsyncRead(): always keep reads in flight. mLimit now only controls how many bytes appear in event notifications. - Fix tick() ordering: call wp->tick() before poll_one() so self-chained async writes advance within the same mainloop frame. --- indra/llcommon/llprocess.cpp | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 007b26ce72..4fc7b62750 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -328,11 +328,11 @@ class ReadPipeImpl : public LLProcess::ReadPipe if (!mPipe || !mPipe->is_open() || mEOF) return; - size_type to_read = (mLimit > 0 && mStreambuf.size() >= mLimit) ? 0 : 4096; - if (to_read == 0) - return; - - auto bufs = mStreambuf.prepare(to_read); + // Always read data regardless of mLimit: stopping reads would fill the + // OS pipe buffer and block the child process, causing a deadlock with + // large messages. mLimit only controls how many bytes are included in + // the event notification, not whether we keep consuming pipe data. + auto bufs = mStreambuf.prepare(4096); mPipe->async_read_some(bufs, [this](const boost::system::error_code& ec, std::size_t bytes_transferred) @@ -704,17 +704,17 @@ void LLProcess::launch(const Params& params) void LLProcess::tick() { - // Poll I/O context before initiating stdin writes so pending read - // callbacks can drain first; this avoids LLLeap large-message stalls where - // a child process waits on output consumption before accepting more input. + // Initiate pending stdin writes before draining the I/O context so that + // self-chained async writes can keep advancing within the same tick + // instead of waiting for the next mainloop frame. + if (auto* wp = dynamic_cast(mWritePipe.get())) + wp->tick(); + while (mIOContext.poll_one() > 0) { // Keep polling until no more handlers are ready } - if (auto* wp = dynamic_cast(mWritePipe.get())) - wp->tick(); - #if LL_WINDOWS // Check process status if (mChild && mStatus.mState == RUNNING && !mChild->running()) From 4026ddb7907fd404cc7635a55b7834f23730a854 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Jul 2026 12:50:42 +0000 Subject: [PATCH 12/36] Address code review: remove misleading inventName comment, fix double-space in comment --- indra/llcommon/llprocess.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 4fc7b62750..108f53584e 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -241,7 +241,7 @@ class ReadPipeImpl : public LLProcess::ReadPipe mPipe(pipe), mSlot(slot), mStream(&mStreambuf), - mPump("ReadPipe", true), // tweak name as needed to avoid collisions, use LLEventPump::inventName? + mPump("ReadPipe", true), mLimit(0), mEOF(false) { @@ -399,7 +399,7 @@ class ReadPipeImpl : public LLProcess::ReadPipe LLProcess::FILESLOT mSlot; mutable asio::streambuf mStreambuf; std::istream mStream; - LLEventStream mPump; // pump specific to this pipe + LLEventStream mPump; // pump specific to this pipe size_type mLimit; bool mEOF; }; From 2cf5a7d1ea1bfe58b4006517f9fe8fee3af245fe Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Jul 2026 12:51:36 +0000 Subject: [PATCH 13/36] Fix stale wstdin() comment reference and malformed C comment closer --- indra/llcommon/llprocess.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 108f53584e..2c577b97a2 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -211,7 +211,7 @@ class WritePipeImpl : public LLProcess::WritePipe mStreambuf.consume(bytes_transferred); LL_DEBUGS("LLProcess") << "Wrote " << bytes_transferred << " bytes to " << mDesc << LL_ENDL; - // If wstdin() queued more data while we were writing, send it + // If callers wrote more data to the ostream while we were writing, send it // immediately instead of waiting for the next mainloop tick. startAsyncWrite(); } @@ -1184,7 +1184,7 @@ static LLProcess::Status interpret_status(int status) // 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 = LLProcess::KILLED; From be1230308c31e768de07deabc4ab5a9522f2cc96 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Jul 2026 12:54:27 +0000 Subject: [PATCH 14/36] Add virtual WritePipe::tick(), promote pipe ownership to shared_ptr - Add virtual tick() to WritePipe base class to avoid dynamic_cast in LLProcess::tick(), and mark WritePipeImpl::tick() as override. - Promote mStdinPipe/mStdoutPipe/mStderrPipe from unique_ptr to shared_ptr so WritePipeImpl/ReadPipeImpl co-own the pipes; prevents dangling pointer if an async operation completes after LLProcess destruction. --- indra/llcommon/llprocess.cpp | 18 +++++++++--------- indra/llcommon/llprocess.h | 11 +++++++---- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 2c577b97a2..40e675b530 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -186,7 +186,7 @@ class WritePipeImpl : public LLProcess::WritePipe // Called from LLProcess::tick() to initiate writing buffered data. // Self-chains: each completed write immediately starts the next one if // more data is waiting, so large transfers don't stall between ticks. - void tick() + void tick() override { startAsyncWrite(); } @@ -582,17 +582,17 @@ void LLProcess::launch(const Params& params) // Create pipes if needed if (use_stdin_pipe) { - mStdinPipe = std::make_unique(mIOContext); + mStdinPipe = std::make_shared(mIOContext); LL_DEBUGS("LLProcess") << "Created stdin pipe for " << mDesc << LL_ENDL; } if (use_stdout_pipe) { - mStdoutPipe = std::make_unique(mIOContext); + mStdoutPipe = std::make_shared(mIOContext); LL_DEBUGS("LLProcess") << "Created stdout pipe for " << mDesc << LL_ENDL; } if (use_stderr_pipe) { - mStderrPipe = std::make_unique(mIOContext); + mStderrPipe = std::make_shared(mIOContext); LL_DEBUGS("LLProcess") << "Created stderr pipe for " << mDesc << LL_ENDL; } @@ -668,14 +668,14 @@ void LLProcess::launch(const Params& params) { mWritePipe = std::make_unique( STRINGIZE(mDesc << " stdin"), - std::shared_ptr(mStdinPipe.get(), [](auto*) {}) + mStdinPipe ); } if (mStdoutPipe) { mStdoutReadPipe = std::make_unique( STRINGIZE(mDesc << " stdout"), - std::shared_ptr(mStdoutPipe.get(), [](auto*) {}), + mStdoutPipe, STDOUT ); } @@ -683,7 +683,7 @@ void LLProcess::launch(const Params& params) { mStderrReadPipe = std::make_unique( STRINGIZE(mDesc << " stderr"), - std::shared_ptr(mStderrPipe.get(), [](auto*) {}), + mStderrPipe, STDERR ); } @@ -707,8 +707,8 @@ void LLProcess::tick() // Initiate pending stdin writes before draining the I/O context so that // self-chained async writes can keep advancing within the same tick // instead of waiting for the next mainloop frame. - if (auto* wp = dynamic_cast(mWritePipe.get())) - wp->tick(); + if (mWritePipe) + mWritePipe->tick(); while (mIOContext.poll_one() > 0) { diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index e75ed954eb..7544e83efe 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -205,6 +205,8 @@ class LL_COMMON_API LLProcess { public: virtual std::ostream& get_ostream() = 0; + // Called each mainloop tick to initiate any pending async writes. + virtual void tick() {} }; /// Read pipe for stdout/stderr @@ -251,10 +253,11 @@ class LL_COMMON_API LLProcess boost::asio::io_context mIOContext; std::unique_ptr mChild; - // Pipes - using Boost.Process async pipes - std::unique_ptr mStdinPipe; - std::unique_ptr mStdoutPipe; - std::unique_ptr mStderrPipe; + // Pipes - using Boost.Process async pipes (shared_ptr so WritePipeImpl/ + // ReadPipeImpl keep the pipe alive as long as async operations are in flight) + std::shared_ptr mStdinPipe; + std::shared_ptr mStdoutPipe; + std::shared_ptr mStderrPipe; // Our pipe wrapper implementations std::unique_ptr mWritePipe; From 2b680a5972006793c5f81f953a1f335027f0d63c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Jul 2026 12:55:27 +0000 Subject: [PATCH 15/36] Improve comments: name deadlock explicitly, clarify std::shared_ptr --- indra/llcommon/llprocess.cpp | 8 ++++---- indra/llcommon/llprocess.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 40e675b530..f0304b0f29 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -328,10 +328,10 @@ class ReadPipeImpl : public LLProcess::ReadPipe if (!mPipe || !mPipe->is_open() || mEOF) return; - // Always read data regardless of mLimit: stopping reads would fill the - // OS pipe buffer and block the child process, causing a deadlock with - // large messages. mLimit only controls how many bytes are included in - // the event notification, not whether we keep consuming pipe data. + // Always read data regardless of mLimit to prevent INTEGRATION_TEST_llleap + // deadlock: stopping reads fills the OS pipe buffer and blocks the child, + // which prevents large (~1 MB) messages from being fully received. + // mLimit only controls how many bytes appear in event notifications. auto bufs = mStreambuf.prepare(4096); mPipe->async_read_some(bufs, diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 7544e83efe..cdb6c8e7d1 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -253,7 +253,7 @@ class LL_COMMON_API LLProcess boost::asio::io_context mIOContext; std::unique_ptr mChild; - // Pipes - using Boost.Process async pipes (shared_ptr so WritePipeImpl/ + // Pipes - using Boost.Process async pipes (std::shared_ptr so WritePipeImpl/ // ReadPipeImpl keep the pipe alive as long as async operations are in flight) std::shared_ptr mStdinPipe; std::shared_ptr mStdoutPipe; From fbdf22b0796084497ceb1a2a3c405cd33e9e7148 Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev <117672381+akleshchev@users.noreply.github.com> Date: Sun, 12 Jul 2026 18:33:24 +0300 Subject: [PATCH 16/36] Cleanup obsolete llprocess code --- indra/llcommon/llprocess.cpp | 195 ----------------------------------- 1 file changed, 195 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index f0304b0f29..bcf5d27643 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -1137,198 +1137,3 @@ std::string LLProcess::getline(std::istream& in) line.pop_back(); return line; } - -/***************************************************************************** -* Windows specific -*****************************************************************************/ -#if LL_WINDOWS -/* -static std::string WindowsErrorString(const std::string& operation); - -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; - - DWORD waitresult = WaitForSingleObject(h, 0); - if(waitresult == WAIT_OBJECT_0) - { - // the process has completed. - if (! desc.empty()) - { - 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; -} - -static LLProcess::Status interpret_status(int status) -{ - 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 = LLProcess::KILLED; - } - else - { - result.mState = LLProcess::EXITED; - } - result.mData = status; - - return result; -} - -/// GetLastError()/FormatMessage() boilerplate -static std::string WindowsErrorString(const std::string& operation) -{ - auto result = GetLastError(); - return STRINGIZE(operation << " failed (" << result << "): " - << windows_message(result)); -} -*/ -/***************************************************************************** -* Posix specific -*****************************************************************************/ -#else // Mac and linux -/* -#include -#include -#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, LLProcess::Status* pstatus=NULL) -{ - 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 == 0) - { - pstatus->mState = LLProcess::RUNNING; - pstatus->mData = 0; - return false; - } - - // Clear caller's Status block; caller must interpret UNSTARTED to mean - // "if this PID was ever valid, it no longer is." - *pstatus = LLProcess::Status(); - - // 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) - { - // No such process -- this may mean we're ignoring SIGCHILD. - return true; - } - - // 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. - LLProcess::Status status; - if(reap_pid(pid, &status)) - { - // the process has exited. - if (! desc.empty()) - { - 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; - } - - return pid; -} - -static LLProcess::Status interpret_status(int status) -{ - LLProcess::Status result; - - 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 - } - - return result; -}*/ - -#endif // Posix From e470379f3b90618b189f0b44cd9f80c0bdc296e5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Jul 2026 17:05:35 +0000 Subject: [PATCH 17/36] Restore original eof event contract in ReadPipe --- indra/llcommon/llprocess.cpp | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index bcf5d27643..257a53b80d 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -362,28 +362,26 @@ class ReadPipeImpl : public LLProcess::ReadPipe if (bytes_transferred > 0) { mStreambuf.commit(bytes_transferred); - - LLSD event; - event["len"] = LLSD::Integer(mStreambuf.size()); - event["slot"] = LLSD::Integer(mSlot); - event["desc"] = mDesc; - - if (mLimit > 0) - { - size_type data_len = (std::min)(mStreambuf.size(), mLimit); - event["data"] = peek(0, data_len); - } - - mPump.post(event); } mEOF = true; LL_DEBUGS("LLProcess") << "EOF on " << mDesc << LL_ENDL; + // Match the original behavior: pack eof, len, and data into a + // single event so consumers can "use it or lose it" -- the + // eof event is the last chance to see any remaining buffered data. LLSD eof_event; eof_event["eof"] = true; + eof_event["len"] = LLSD::Integer(mStreambuf.size()); eof_event["slot"] = LLSD::Integer(mSlot); eof_event["desc"] = mDesc; + + if (mLimit > 0) + { + size_type data_len = (std::min)(mStreambuf.size(), mLimit); + eof_event["data"] = peek(0, data_len); + } + mPump.post(eof_event); } else if (ec != asio::error::operation_aborted) From 51b0c2e8c6bd1e011bc44d03dd51cb7fd2cbbfc3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Jul 2026 17:19:53 +0000 Subject: [PATCH 18/36] Restore all 7 original ReadPipe event fields --- indra/llcommon/llprocess.cpp | 46 +++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 257a53b80d..7354f614fe 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -343,16 +343,19 @@ class ReadPipeImpl : public LLProcess::ReadPipe LL_DEBUGS("LLProcess") << "Read " << bytes_transferred << " bytes from " << mDesc << LL_ENDL; + // Restore original 7-field event contract: + // data, len, slot, name, desc, eof, exhst + // A successful async read corresponds to the original EXHAUSTED + // state: we got data, pipe is not yet closed. + size_type data_len = (std::min)(mStreambuf.size(), mLimit); LLSD event; - event["len"] = LLSD::Integer(mStreambuf.size()); - event["slot"] = LLSD::Integer(mSlot); - event["desc"] = mDesc; - - if (mLimit > 0) - { - size_type data_len = (std::min)(mStreambuf.size(), mLimit); - event["data"] = peek(0, data_len); - } + event["data"] = peek(0, data_len); + event["len"] = LLSD::Integer(mStreambuf.size()); + event["slot"] = LLSD::Integer(mSlot); + event["name"] = whichfile(mSlot); + event["desc"] = mDesc; + event["eof"] = false; + event["exhst"] = true; mPump.post(event); startAsyncRead(); @@ -367,20 +370,19 @@ class ReadPipeImpl : public LLProcess::ReadPipe mEOF = true; LL_DEBUGS("LLProcess") << "EOF on " << mDesc << LL_ENDL; - // Match the original behavior: pack eof, len, and data into a - // single event so consumers can "use it or lose it" -- the - // eof event is the last chance to see any remaining buffered data. + // Match the original behavior: pack all 7 fields into the + // single eof event so consumers can "use it or lose it" -- + // this is the last chance to see any remaining buffered data. + // EOF corresponds to the original CLOSED state. + size_type data_len = (std::min)(mStreambuf.size(), mLimit); LLSD eof_event; - eof_event["eof"] = true; - eof_event["len"] = LLSD::Integer(mStreambuf.size()); - eof_event["slot"] = LLSD::Integer(mSlot); - eof_event["desc"] = mDesc; - - if (mLimit > 0) - { - size_type data_len = (std::min)(mStreambuf.size(), mLimit); - eof_event["data"] = peek(0, data_len); - } + eof_event["data"] = peek(0, data_len); + eof_event["len"] = LLSD::Integer(mStreambuf.size()); + eof_event["slot"] = LLSD::Integer(mSlot); + eof_event["name"] = whichfile(mSlot); + eof_event["desc"] = mDesc; + eof_event["eof"] = true; + eof_event["exhst"] = false; mPump.post(eof_event); } From 01bba65560060e7f26378041b87788248b50fd2f Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev <117672381+akleshchev@users.noreply.github.com> Date: Sun, 12 Jul 2026 20:27:12 +0300 Subject: [PATCH 19/36] Restore whichfile --- indra/llcommon/llprocess.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 7354f614fe..a4656df67d 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -64,18 +64,18 @@ namespace asio = boost::asio; /***************************************************************************** * Helpers *****************************************************************************/ -/* + static const char* whichfile_[] = { "stdin", "stdout", "stderr" }; -static LLProcess::Status interpret_status(int status); -static std::string getDesc(const LLProcess::Params& params); +//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 From f19647928b9fd1634170221455c2bcb8a522e60a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Jul 2026 17:49:38 +0000 Subject: [PATCH 20/36] Fix Windows kill() exit code and stdout/stderr EOF --- indra/llcommon/llprocess.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index a4656df67d..fefca7280c 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -878,13 +878,16 @@ bool LLProcess::kill(const std::string& who) LL_INFOS("LLProcess") << who << " killing " << mDesc << LL_ENDL; #if LL_WINDOWS - std::error_code ec; - mChild->terminate(ec); - - if (ec) + // Call TerminateProcess directly with exit code (UINT)-1 so that + // tick()'s GetExitCodeProcess reads back -1 as a signed int, matching + // the original APR-based behavior expected by tests and callers. + // Do NOT use mChild->terminate(): boost::process v1 may use a different + // exit code (e.g. EXIT_FAILURE=1) or invalidate the handle internally, + // which would break the exit-code check in tick(). + if (!::TerminateProcess(mChild->native_handle(), (UINT)-1)) { LL_WARNS("LLProcess") << "Failed to terminate " << mDesc - << ": " << ec.message() << LL_ENDL; + << ": error " << ::GetLastError() << LL_ENDL; return false; } #else From e468513224a382c5a2ce033804852bdeeaeabe36 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Jul 2026 21:45:59 +0000 Subject: [PATCH 21/36] Fix Windows EOF event race after process exit --- indra/llcommon/llprocess.cpp | 20 +++++++++++++++----- indra/llcommon/llprocess.h | 2 ++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index fefca7280c..f8062675d0 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -271,6 +271,8 @@ class ReadPipeImpl : public LLProcess::ReadPipe virtual size_type getLimit() const override { return mLimit; } + virtual bool atEOF() const override { return mEOF; } + virtual size_type size() const override { return mStreambuf.size(); } virtual std::string read(size_type len) override @@ -772,6 +774,18 @@ void LLProcess::tick() // result == 0 means still running } #endif + + // Keep pumping after process exit until all ReadPipes report EOF, then + // disconnect from mainloop to avoid losing trailing EOF notifications. + if (mStatus.mState != RUNNING && mMainloopConnection.connected()) + { + bool stdout_eof = (!mStdoutReadPipe || mStdoutReadPipe->atEOF()); + bool stderr_eof = (!mStderrReadPipe || mStderrReadPipe->atEOF()); + if (stdout_eof && stderr_eof) + { + mMainloopConnection.disconnect(); + } + } } void LLProcess::handleExit(Status exitStatus) @@ -805,11 +819,7 @@ void LLProcess::handleExit(Status exitStatus) LLEventPumps::instance().obtain(mPostend).post(event); } - // Disconnect from mainloop - if (mMainloopConnection.connected()) - { - mMainloopConnection.disconnect(); - } + // Leave mainloop connected until tick() observes EOF on all ReadPipes. } bool LLProcess::isRunning() const diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index cdb6c8e7d1..cf5e41576a 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -230,6 +230,8 @@ class LL_COMMON_API LLProcess virtual LLEventPump& getPump() = 0; virtual void setLimit(size_type limit) = 0; virtual size_type getLimit() const = 0; + // True once the pipe has observed child-process EOF. + virtual bool atEOF() const = 0; }; WritePipe& getWritePipe(FILESLOT slot = STDIN); From 4706cbf7de69100962a9590da06b3f780ca1c72e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 13 Jul 2026 07:09:59 +0000 Subject: [PATCH 22/36] Fix 4 Windows CI test failures in llprocess --- indra/llcommon/llprocess.cpp | 49 +++++++++++++++++++++++++++++------- 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index f8062675d0..23e1a9eb7f 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -199,8 +199,13 @@ class WritePipeImpl : public LLProcess::WritePipe mWritePending = true; - // Write all buffered data asynchronously, then chain to the next write - // if more data was queued while this one was in flight. + // Write all buffered data asynchronously. Do NOT self-chain in the + // completion handler: sending the next buffer immediately can cause + // the child to respond within the same tick(), which posts a read + // event before the test listener has a chance to disconnect -- that + // was the root cause of test 18 "more than 3 events" and test 9 + // "many small messages" failures. tick() calls mWritePipe->tick() on + // every mainloop frame, so any newly-queued data will be sent then. asio::async_write(*mPipe, mStreambuf.data(), [this](const boost::system::error_code& ec, std::size_t bytes_transferred) { @@ -211,9 +216,6 @@ class WritePipeImpl : public LLProcess::WritePipe mStreambuf.consume(bytes_transferred); LL_DEBUGS("LLProcess") << "Wrote " << bytes_transferred << " bytes to " << mDesc << LL_ENDL; - // If callers wrote more data to the ostream while we were writing, send it - // immediately instead of waiting for the next mainloop tick. - startAsyncWrite(); } else if (ec != asio::error::operation_aborted) { @@ -362,7 +364,13 @@ class ReadPipeImpl : public LLProcess::ReadPipe mPump.post(event); startAsyncRead(); } - else if (ec == asio::error::eof) + else if (ec == asio::error::eof +#if LL_WINDOWS + // On Windows anonymous pipes, the write end closing + // delivers ERROR_BROKEN_PIPE (109), not asio::error::eof. + || ec.value() == ERROR_BROKEN_PIPE +#endif + ) { if (bytes_transferred > 0) { @@ -418,7 +426,7 @@ LLProcess::LLProcess(const Params& params) : mDesc(params.desc.isProvided() ? params.desc() : basename(params.executable())), mPostend(params.postend.isProvided() ? params.postend() : ""), mAutokill(params.autokill), - mAttached(params.attached) + mAttached(params.attached.isProvided() ? params.attached() : bool(params.autokill)) { launch(params); } @@ -944,7 +952,26 @@ LLProcess::handle LLProcess::getProcessHandle() const return 0; #if LL_WINDOWS - return mChild->native_handle(); + // Duplicate the process handle so the caller owns an independent copy. + // boost::process v1's child_handle destructor always closes proc_info.hProcess + // (even after child::detach()), so the raw native_handle() becomes invalid + // once ~child() runs. DuplicateHandle gives the caller a handle whose + // lifetime is not tied to boost's internal child_handle cleanup. + HANDLE source = mChild->native_handle(); + if (!source || source == INVALID_HANDLE_VALUE) + return 0; + HANDLE dup = nullptr; + if (!::DuplicateHandle( + ::GetCurrentProcess(), source, + ::GetCurrentProcess(), &dup, + PROCESS_QUERY_INFORMATION | SYNCHRONIZE, + FALSE, 0)) + { + LL_WARNS("LLProcess") << "DuplicateHandle failed for " << mDesc + << ": error " << ::GetLastError() << LL_ENDL; + return 0; + } + return dup; #else return mChild->id(); #endif @@ -960,8 +987,12 @@ LLProcess::handle LLProcess::isRunning(handle h, const std::string& desc) DWORD exit_code; if (GetExitCodeProcess(h, &exit_code)) { - return (exit_code == STILL_ACTIVE) ? h : 0; + if (exit_code == STILL_ACTIVE) + return h; // process still running + // Process has exited: close the duplicated handle (see getProcessHandle()). } + // Either process exited or GetExitCodeProcess failed; either way we're done. + CloseHandle(h); return 0; #else if (h == 0) From 0d661ac2ec13765605304261362ac5f9376400aa Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev <117672381+akleshchev@users.noreply.github.com> Date: Mon, 13 Jul 2026 20:34:31 +0300 Subject: [PATCH 23/36] Cleanup old apr headers and helpers --- indra/llcommon/llprocess.cpp | 74 ------------------------------------ indra/llcommon/llprocess.h | 1 - 2 files changed, 75 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 23e1a9eb7f..dd75d69f81 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -30,9 +30,6 @@ #include "llsdserialize.h" #include "llsingleton.h" #include "llstring.h" -#include "stringize.h" -#include "llapr.h" -#include "apr_signal.h" #include "llevents.h" #include "llexception.h" #include "stringize.h" @@ -67,9 +64,6 @@ namespace asio = boost::asio; static const char* whichfile_[] = { "stdin", "stdout", "stderr" }; -//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_)) @@ -77,74 +71,6 @@ static std::string whichfile(LLProcess::FILESLOT 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 - * 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; - 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 cf5e41576a..d5f4d74991 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -30,7 +30,6 @@ #include "llinitparam.h" #include "llsdparam.h" #include "llexception.h" -#include "apr_thread_proc.h" #include #include #include From ba16482ee646f64c22193d17c12bb9ca26b5cdc5 Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev <117672381+akleshchev@users.noreply.github.com> Date: Mon, 13 Jul 2026 22:40:57 +0300 Subject: [PATCH 24/36] Fix param validation --- indra/llcommon/llprocess.cpp | 17 ++++++++++++++++- indra/llcommon/llprocess.h | 2 +- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index dd75d69f81..1822fb61be 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -54,6 +54,15 @@ #include #include +#if !LL_WINDOWS + // not necessarily available on random SDL platforms + // for waitpid() +#include +#include +#include +#include +#endif + namespace bp = boost::process::v1; namespace asio = boost::asio; @@ -441,8 +450,14 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params) } } -void LLProcess::launch(const Params& params) +void LLProcess::launch(const LLSDOrParams& params) { + if (!params.validateBlock(true)) + { + LL_WARNS("LLProcess") << "Failed parameter validation " << LLSDNotationStreamer(params) << LL_ENDL; + throw std::runtime_error("not launched: failed parameter validation\n"); + } + // Validate FileParam types before attempting to launch int file_idx = 0; for (const auto& fparam : params.files) diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index d5f4d74991..847d92fbb3 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -246,7 +246,7 @@ class LL_COMMON_API LLProcess LLProcess(const Params& params); private: - void launch(const Params& params); + void launch(const LLSDOrParams& params); void tick(); void handleExit(Status exitStatus); From c7899c6e621328e4959c9eeb063252d4e7bf2d15 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 13 Jul 2026 19:45:36 +0000 Subject: [PATCH 25/36] Fix LLProcess::kill() to return true for null process ptr --- indra/llcommon/llprocess.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 1822fb61be..87aef10cd6 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -872,7 +872,7 @@ bool LLProcess::kill(const std::string& who) //static bool LLProcess::kill(const LLProcessPtr& ptr, const std::string& who) { - return ptr && ptr->kill(who); + return !ptr || ptr->kill(who); } LLProcess::id LLProcess::getProcessID() const From 11fb3d44b82f8e7a73962b8ea5719c0b2ef0ac4f Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev <117672381+akleshchev@users.noreply.github.com> Date: Mon, 13 Jul 2026 22:51:39 +0300 Subject: [PATCH 26/36] Safeguard handleExit --- indra/llcommon/llprocess.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 87aef10cd6..30e9a427c1 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -748,6 +748,14 @@ void LLProcess::handleExit(Status exitStatus) // consumes child stdout/stderr from these callbacks, and some tests write // enough data to require far more than a handful of async-read completions // after the child has already exited. + if (mIOContext.stopped()) + { + // If the io_context has reached the stopped state, + // following loop will never run and trailing stdout/stderr + // handlers(and EOF notifications) may be missed. + LL_INFOS("LLProcess") << "mIOContext was stopped on exit, restarting" << LL_ENDL; + mIOContext.restart(); + } while (mIOContext.poll_one() > 0) { // Keep polling until no more handlers are immediately ready. From 77ff2f7fc8ff1b51e9d1a0e95f98521d7a3d3a08 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 13 Jul 2026 19:54:17 +0000 Subject: [PATCH 27/36] Add mIOContext.restart() before poll loop in LLProcess::tick() --- indra/llcommon/llprocess.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 30e9a427c1..2f9bc98143 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -661,6 +661,7 @@ void LLProcess::tick() if (mWritePipe) mWritePipe->tick(); + mIOContext.restart(); while (mIOContext.poll_one() > 0) { // Keep polling until no more handlers are ready From c173137e13535ad1641ff06379fd57ce86961ed1 Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev <117672381+akleshchev@users.noreply.github.com> Date: Mon, 13 Jul 2026 23:56:41 +0300 Subject: [PATCH 28/36] Address review feedback --- indra/llcommon/llprocess.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 2f9bc98143..05b039c922 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -50,14 +50,15 @@ #include #include #include +#include #include #include +#include #include #if !LL_WINDOWS // not necessarily available on random SDL platforms // for waitpid() -#include #include #include #include @@ -296,8 +297,10 @@ class ReadPipeImpl : public LLProcess::ReadPipe event["eof"] = false; event["exhst"] = true; - mPump.post(event); + // Arm the next read before posting: LLEventStream::post() is synchronous + // and listeners may destroy this object. startAsyncRead(); + mPump.post(event); } else if (ec == asio::error::eof #if LL_WINDOWS @@ -875,7 +878,7 @@ bool LLProcess::kill(const std::string& who) #endif // Don't set status here - let handleExit() do it when the process actually terminates - return true; + return !isRunning(); } //static From 1d9e2085a6f471243e7d08d141ba43181f9ce9da Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev <117672381+akleshchev@users.noreply.github.com> Date: Tue, 14 Jul 2026 09:03:28 +0300 Subject: [PATCH 29/36] Remove extra include Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- indra/llcommon/llprocess.h | 1 - 1 file changed, 1 deletion(-) diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 847d92fbb3..9d494d55f7 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -39,7 +39,6 @@ #include #include #include -#include #include // std::ostream #if LL_WINDOWS From 6bc49e73db9aaa6aff1d1e8889a2cd7ddf27a690 Mon Sep 17 00:00:00 2001 From: Andrey Kleshchev <117672381+akleshchev@users.noreply.github.com> Date: Tue, 14 Jul 2026 09:06:18 +0300 Subject: [PATCH 30/36] Fix error code source Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- indra/llcommon/llprocess.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 05b039c922..99943434c8 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -376,7 +376,7 @@ LLProcess::~LLProcess() if (mAttached && mAutokill) { LL_INFOS("LLProcess") << "Terminating child process " << mDesc << LL_ENDL; - std::error_code ec; + boost::system::error_code ec; mChild->terminate(ec); #if !LL_WINDOWS @@ -553,8 +553,7 @@ void LLProcess::launch(const LLSDOrParams& params) // Build the process try { - std::error_code ec; - + boost::system::error_code ec; #if !LL_WINDOWS // Ignore SIGPIPE so that writing to a child's closed stdin doesn't // terminate the viewer process. The write will fail with EPIPE instead. From c7d2902615815efcce594e11dc0f8a9f92378562 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Jul 2026 06:12:41 +0000 Subject: [PATCH 31/36] fix: prevent use-after-free in LLProcess mainloop callback Inherit LLProcess from std::enable_shared_from_this and capture a weak_ptr in the mainloop lambda. Locking the weak_ptr before calling tick() keeps *this alive for the full callback duration, so a listener that drops the last LLProcessPtr during a synchronous event post inside tick()/handleExit() cannot destroy the object while it is still on the call stack. --- indra/llcommon/llprocess.cpp | 15 +++++++++++++-- indra/llcommon/llprocess.h | 3 ++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 99943434c8..4905770cb1 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -641,10 +641,21 @@ void LLProcess::launch(const LLSDOrParams& params) ); } - // Hook into mainloop for I/O processing + // Hook into mainloop for I/O processing. + // Capture a weak_ptr to prevent use-after-free: a listener responding + // to a synchronous event post inside tick() may drop the last + // LLProcessPtr, destroying this object while tick() is on the stack. + // Locking the weak_ptr before calling tick() keeps *this alive for the + // duration of the callback. + std::weak_ptr weak = shared_from_this(); mMainloopConnection = LLEventPumps::instance().obtain("mainloop") .listen(LLEventPump::inventName("LLProcess"), - [this](const LLSD&) { tick(); return false; }); + [weak](const LLSD&) + { + auto self = weak.lock(); + if (self) self->tick(); + return false; + }); LL_INFOS("LLProcess") << "Launched " << mDesc << " (PID: " << mChild->id() << ")" << LL_ENDL; diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 9d494d55f7..638a80970f 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -30,6 +30,7 @@ #include "llinitparam.h" #include "llsdparam.h" #include "llexception.h" +#include #include #include #include @@ -73,7 +74,7 @@ typedef std::shared_ptr LLProcessPtr; * child-process termination in a standalone test context. */ -class LL_COMMON_API LLProcess +class LL_COMMON_API LLProcess : public std::enable_shared_from_this { LOG_CLASS(LLProcess); public: From 5ad1b2766199cf3e6ba07ac6b207e31be3b58d6d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Jul 2026 06:14:58 +0000 Subject: [PATCH 32/36] fix: defer connectMainloop() to after construction to avoid bad_weak_ptr shared_from_this() requires the shared_ptr control block to be fully initialized, which is not the case during the constructor. Move the mainloop listener registration into a separate connectMainloop() method and call it from create() after make_shared() returns. --- indra/llcommon/llprocess.cpp | 45 ++++++++++++++++++++++-------------- indra/llcommon/llprocess.h | 1 + 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 4905770cb1..024f8c18a8 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -430,7 +430,13 @@ LLProcessPtr LLProcess::create(const LLSDOrParams& params) { try { - return std::make_shared(params); + // Construct and then register the mainloop connection separately. + // connectMainloop() calls shared_from_this(), which requires the + // shared_ptr control block to be fully initialized — this is only true + // after make_shared returns, not during the constructor. + LLProcessPtr ptr = std::make_shared(params); + ptr->connectMainloop(); + return ptr; } catch (const std::exception& e) { @@ -641,22 +647,6 @@ void LLProcess::launch(const LLSDOrParams& params) ); } - // Hook into mainloop for I/O processing. - // Capture a weak_ptr to prevent use-after-free: a listener responding - // to a synchronous event post inside tick() may drop the last - // LLProcessPtr, destroying this object while tick() is on the stack. - // Locking the weak_ptr before calling tick() keeps *this alive for the - // duration of the callback. - std::weak_ptr weak = shared_from_this(); - mMainloopConnection = LLEventPumps::instance().obtain("mainloop") - .listen(LLEventPump::inventName("LLProcess"), - [weak](const LLSD&) - { - auto self = weak.lock(); - if (self) self->tick(); - return false; - }); - LL_INFOS("LLProcess") << "Launched " << mDesc << " (PID: " << mChild->id() << ")" << LL_ENDL; } @@ -666,6 +656,27 @@ void LLProcess::launch(const LLSDOrParams& params) } } +void LLProcess::connectMainloop() +{ + // Capture a weak_ptr to prevent use-after-free: a listener responding to + // a synchronous event post inside tick() may drop the last LLProcessPtr, + // destroying this object while tick() is on the call stack. Locking the + // weak_ptr before calling tick() keeps *this alive for the duration of + // the callback. + // NOTE: shared_from_this() is only valid after the shared_ptr owning this + // object has been fully constructed, so this must be called from create() + // rather than from the constructor. + std::weak_ptr weak = shared_from_this(); + mMainloopConnection = LLEventPumps::instance().obtain("mainloop") + .listen(LLEventPump::inventName("LLProcess"), + [weak](const LLSD&) + { + auto self = weak.lock(); + if (self) self->tick(); + return false; + }); +} + void LLProcess::tick() { // Initiate pending stdin writes before draining the I/O context so that diff --git a/indra/llcommon/llprocess.h b/indra/llcommon/llprocess.h index 638a80970f..565a645619 100644 --- a/indra/llcommon/llprocess.h +++ b/indra/llcommon/llprocess.h @@ -247,6 +247,7 @@ class LL_COMMON_API LLProcess : public std::enable_shared_from_this private: void launch(const LLSDOrParams& params); + void connectMainloop(); void tick(); void handleExit(Status exitStatus); From 1ec15ce59e2ecb7005cec822e615abcb47310fac Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Jul 2026 06:15:42 +0000 Subject: [PATCH 33/36] Add missing standard headers , , to llprocess.cpp --- indra/llcommon/llprocess.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 024f8c18a8..8fa8f2a00d 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -50,7 +50,10 @@ #include #include #include +#include +#include #include +#include #include #include #include From 2f7f744ca27a1496bdeb984917c730c07cd133aa Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Jul 2026 06:15:52 +0000 Subject: [PATCH 34/36] fix: improve inline comments in connectMainloop and handleExit --- indra/llcommon/llprocess.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 8fa8f2a00d..feeda33f58 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -674,6 +674,8 @@ void LLProcess::connectMainloop() .listen(LLEventPump::inventName("LLProcess"), [weak](const LLSD&) { + // Lock weak_ptr to keep *this alive during tick(), preventing + // destruction mid-callback if a listener drops the last LLProcessPtr. auto self = weak.lock(); if (self) self->tick(); return false; @@ -779,8 +781,8 @@ void LLProcess::handleExit(Status exitStatus) if (mIOContext.stopped()) { // If the io_context has reached the stopped state, - // following loop will never run and trailing stdout/stderr - // handlers(and EOF notifications) may be missed. + // the following loop will never run and trailing stdout/stderr + // handlers (and EOF notifications) may be missed. LL_INFOS("LLProcess") << "mIOContext was stopped on exit, restarting" << LL_ENDL; mIOContext.restart(); } From ef0ae9ab2a2baeb1eb72e5121f5ed9bd9c6f6a3e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Jul 2026 06:23:02 +0000 Subject: [PATCH 35/36] Fix WritePipeImpl buffer invalidation during async write --- indra/llcommon/llprocess.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index feeda33f58..7633fcb8fb 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -138,6 +138,14 @@ class WritePipeImpl : public LLProcess::WritePipe mWritePending = true; + // Snapshot the number of bytes currently buffered so the async_write + // operates on a fixed-size, stable view. Any data written to + // get_ostream() while this write is in flight lands in the streambuf's + // put area and is excluded from the current operation; it will be sent + // on the next tick(). Without this snapshot, a concurrent write to + // get_ostream() could reallocate/invalidate the buffer sequence. + std::size_t writeSize = mStreambuf.size(); + // Write all buffered data asynchronously. Do NOT self-chain in the // completion handler: sending the next buffer immediately can cause // the child to respond within the same tick(), which posts a read @@ -145,7 +153,7 @@ class WritePipeImpl : public LLProcess::WritePipe // was the root cause of test 18 "more than 3 events" and test 9 // "many small messages" failures. tick() calls mWritePipe->tick() on // every mainloop frame, so any newly-queued data will be sent then. - asio::async_write(*mPipe, mStreambuf.data(), + asio::async_write(*mPipe, asio::buffer(mStreambuf.data(), writeSize), [this](const boost::system::error_code& ec, std::size_t bytes_transferred) { mWritePending = false; From 68e8c3293293f4b51d372d853e3f8ba43eeac143 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Jul 2026 06:23:17 +0000 Subject: [PATCH 36/36] Fix macOS build: use std::error_code instead of boost::system::error_code for bp::child boost::process v1 only specializes initializer_tag (in error.hpp), not initializer_tag. On macOS with Boost 1.90, these are distinct types, so passing boost::system::error_code directly to bp::child() constructor caused a template instantiation failure. Revert the two ec declarations in LLProcess::~LLProcess() and LLProcess::launch() back to std::error_code, which is what boost::process v1 expects. --- indra/llcommon/llprocess.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/indra/llcommon/llprocess.cpp b/indra/llcommon/llprocess.cpp index 7633fcb8fb..b047adffcc 100644 --- a/indra/llcommon/llprocess.cpp +++ b/indra/llcommon/llprocess.cpp @@ -387,7 +387,7 @@ LLProcess::~LLProcess() if (mAttached && mAutokill) { LL_INFOS("LLProcess") << "Terminating child process " << mDesc << LL_ENDL; - boost::system::error_code ec; + std::error_code ec; mChild->terminate(ec); #if !LL_WINDOWS @@ -570,7 +570,7 @@ void LLProcess::launch(const LLSDOrParams& params) // Build the process try { - boost::system::error_code ec; + std::error_code ec; #if !LL_WINDOWS // Ignore SIGPIPE so that writing to a child's closed stdin doesn't // terminate the viewer process. The write will fail with EPIPE instead.