From b2fae1f26a117e01c8f38d20b64649a6927352de Mon Sep 17 00:00:00 2001 From: Nathan Glenn Date: Mon, 22 Jul 2024 15:28:36 -0500 Subject: [PATCH] allow SpawnDebugger after user close Previously we would require the user to call KillDebugger() in order to be able to call SpawnDebugger() again, even when the user had manually closed the debugger. Fix that by checking whether a previously-spawned debugger process is still running when the user calls SpawnDebugger again. If it is, print out the process information (since it can happen that the debugger becomes a zombie or something and the user needs more information in order to kill it). If it isn't, spawn a new debugger process as requested. Factor out the debugger process info cleanup logic, since it's being used in 5 places. Would probably be better to use a smart pointer, but for now this is clear enough. Ensure that every branch that returns `false` in `SpawnDebugger` and `KillDebugger` prints a message explaining what happened. Fixes #483. --- Core/ClientSML/src/sml_ClientAgent.cpp | 112 +++++++++++++++++++++---- Core/ClientSML/src/sml_ClientAgent.h | 9 +- SoarCLI/soar_cli.h | 13 ++- 3 files changed, 110 insertions(+), 24 deletions(-) diff --git a/Core/ClientSML/src/sml_ClientAgent.cpp b/Core/ClientSML/src/sml_ClientAgent.cpp index ba9c805088..4054082a78 100644 --- a/Core/ClientSML/src/sml_ClientAgent.cpp +++ b/Core/ClientSML/src/sml_ClientAgent.cpp @@ -94,6 +94,37 @@ namespace sml }; } +void print_debugger_process_information(DebuggerProcessInformation* processInformation) +{ +#ifdef _WIN32 + PrintDebugFormat("Debugger process information: hProcess = %p, hThread = %p, dwProcessId = %d, dwThreadId = %d", + processInformation->debuggerProcessInformation.hProcess, + processInformation->debuggerProcessInformation.hThread, + processInformation->debuggerProcessInformation.dwProcessId, + processInformation->debuggerProcessInformation.dwThreadId); +#else // _WIN32 + PrintDebugFormat("Debugger process information: pid = %d", processInformation->debuggerPid); +#endif // not _WIN32 +} + +/** + * Returns true if a debugger process was previously started via SpawnDebugger and is still running + */ +bool debugger_is_running(DebuggerProcessInformation* processInformation) +{ +#ifdef _WIN32 + DWORD exitCode; + if (GetExitCodeProcess(processInformation->debuggerProcessInformation.hProcess, &exitCode)) + { + return exitCode == STILL_ACTIVE; + } + return false; +#else // _WIN32 + int status; + return waitpid(processInformation->debuggerPid, &status, WNOHANG) == 0; +#endif // not _WIN32 +} + std::string get_soarlib_path() { std::string h = libraryPath; @@ -1466,6 +1497,20 @@ bool isfile(const char* path) #endif } +// There's no great way to unit-test this, but testing from Python you should be able to see the debugger +// open and return values given as below: +// ```python +// import Python_sml_ClientInterface as sml +// k = sml.Kernel.CreateKernelInNewThread() +// a = k.CreateAgent("hi") +// a.SpawnDebugger() +// True +// a.SpawnDebugger() +// False +// # now close the debugger manually and then: +// a.SpawnDebugger() +// True +// ``` bool Agent::SpawnDebugger(int port, const char* jarpath) { std::string p; @@ -1473,6 +1518,7 @@ bool Agent::SpawnDebugger(int port, const char* jarpath) { if (!isfile(jarpath)) { + std::cerr << "SpawnDebugger: jarparth '" << jarpath << "'is not a file" << std::endl; return false; } p = jarpath; @@ -1533,11 +1579,14 @@ bool Agent::SpawnDebugger(int port, const char* jarpath) char buffer[4096 + 1]; #ifdef _MSC_VER - GetCurrentDirectory(4096, buffer); + if (!GetCurrentDirectory(4096, buffer)) { + std::cerr << "SpawnDebugger: GetCurrentDirectory failed: " << GetLastError() << std::endl; + return false; + } #else if (getcwd(buffer, 4096) != buffer) { - perror("getcwd"); + perror("SpawnDebugger: getcwd failed"); return false; } #endif @@ -1550,12 +1599,15 @@ bool Agent::SpawnDebugger(int port, const char* jarpath) { p = debuggerPath; } + else { + std::cerr << "SpawnDebugger: Calculated debugger path '" << debuggerPath << "' is not a file" << std::endl; + } } if (p.length() == 0) - { - return false; - } + { + return false; + } if (port == -1) { @@ -1564,7 +1616,14 @@ bool Agent::SpawnDebugger(int port, const char* jarpath) if (m_pDPI) { - return false; + if (!debugger_is_running(m_pDPI)) + { + ClearDebuggerProcessInformation(); + } else { + std::cerr << "SpawnDebugger: Previously-spawned debugger process is still running" << std::endl; + print_debugger_process_information(m_pDPI); + return false; + } } m_pDPI = new DebuggerProcessInformation(); @@ -1578,9 +1637,15 @@ bool Agent::SpawnDebugger(int port, const char* jarpath) HANDLE pipes[NumPipeTypes]; if (!CreatePipe(&pipes[ParentWrite], &pipes[ChildRead], &sa, 0)) - { return 0; } + { + std::cerr << "SpawnDebugger: CreatePipe (parent write, child read) failed: " << GetLastError() << std::endl; + return false; + } if (!CreatePipe(&pipes[ParentRead], &pipes[ChildWrite], &sa, 0)) - { return 0; } + { + std::cerr << "SpawnDebugger: CreatePipe (parent read, child write) failed: " << GetLastError() << std::endl; + return false; + } // make sure the handles the parent will use aren't inherited. SetHandleInformation(pipes[ParentRead], HANDLE_FLAG_INHERIT, 0); @@ -1656,9 +1721,8 @@ bool Agent::SpawnDebugger(int port, const char* jarpath) if (ret == 0) { - std::cout << "Error code: " << GetLastError() << std::endl; - delete m_pDPI; - m_pDPI = 0; + std::cout << "SpawnDebugger: CreateProcess failed: " << GetLastError() << std::endl; + ClearDebuggerProcessInformation(); return false; } @@ -1682,8 +1746,8 @@ bool Agent::SpawnDebugger(int port, const char* jarpath) if (m_pDPI->debuggerPid < 0) { - delete m_pDPI; - m_pDPI = 0; + ClearDebuggerProcessInformation(); + perror("SpawnDebugger: fork failed"); return false; } @@ -1711,7 +1775,7 @@ bool Agent::SpawnDebugger(int port, const char* jarpath) char buffer[4096 + 1]; if (getcwd(buffer, 4096) != buffer) { - perror("getcwd"); + perror("SpawnDebugger: getcwd failed"); return false; } @@ -1766,7 +1830,7 @@ bool Agent::SpawnDebugger(int port, const char* jarpath) // does not return on success - std::cerr << "Debugger spawn failed: " << strerror(errno) << std::endl; + perror("SpawnDebugger: execlp failed"); exit(1); } @@ -1779,6 +1843,7 @@ bool Agent::KillDebugger() { if (!m_pDPI) { + std::cerr << "KillDebugger: No existing debugger process information" << std::endl; return false; } bool successful = false; @@ -1791,20 +1856,31 @@ bool Agent::KillDebugger() if (ret) { successful = true; + } else { + std::cerr << "KillDebugger: TerminateProcess failed: " << GetLastError() << std::endl; } #else // _WIN32 if (!kill(m_pDPI->debuggerPid, SIGTERM)) { successful = true; + } else { + perror("KillDebugger: kill failed"); } #endif // _WIN32 - - delete m_pDPI; - m_pDPI = 0; + ClearDebuggerProcessInformation(); return successful; } +void Agent::ClearDebuggerProcessInformation() +{ + if (m_pDPI) + { + delete m_pDPI; + m_pDPI = 0; + } +} + char const* Agent::ConvertIdentifier(char const* pClientIdentifier) { // need to keep the result around after the function returns diff --git a/Core/ClientSML/src/sml_ClientAgent.h b/Core/ClientSML/src/sml_ClientAgent.h index efcfcddbe9..e8b3f25b59 100644 --- a/Core/ClientSML/src/sml_ClientAgent.h +++ b/Core/ClientSML/src/sml_ClientAgent.h @@ -874,12 +874,16 @@ namespace sml * Java must be in the path. If jarpath is NULL, the * function will search for SoarJavaDebugger.jar first in * the current directory, then in $SOAR_HOME. Returns - * false if the jar is not found or process spawning fails. + * false if the jar is not found or process spawning fails, + * or if a previously spawned debugger process is still + * running. *************************************************************/ bool SpawnDebugger(int port = -1, const char* jarpath = 0); /************************************************************* - * @brief Kills the previously spawned debugger. + * @brief Kills the previously spawned debugger. Returns false + * if the debugger was never spawned or an OS issue occurs + * while killing the process. *************************************************************/ bool KillDebugger(); @@ -891,6 +895,7 @@ namespace sml protected: // for {Spawn, Kill}Debugger() DebuggerProcessInformation* m_pDPI; + void ClearDebuggerProcessInformation(); }; diff --git a/SoarCLI/soar_cli.h b/SoarCLI/soar_cli.h index 2816a36d5d..87c6a18482 100644 --- a/SoarCLI/soar_cli.h +++ b/SoarCLI/soar_cli.h @@ -183,15 +183,20 @@ class SoarCLI { public: - SoarCLI() - : m_kernel(0), m_currentAgent(0), m_quit(false), m_isMultiAgent(false), - m_longestAgentName(0), m_seen_newline(true), + SoarCLI(): + m_kernel(0), + m_currentAgent(0), + m_port(sml::Kernel::kUseAnyPort), + m_listen(false), #if defined(NO_COLORS) m_color(false), #else m_color(stdout_supports_ansi_colors()), #endif - m_listen(false), m_port(sml::Kernel::kUseAnyPort) {} + m_quit(false), + m_seen_newline(true), + m_isMultiAgent(false), + m_longestAgentName(0) {} ~SoarCLI();