Skip to content

Commit

Permalink
allow SpawnDebugger after user close
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
garfieldnate committed Jul 22, 2024
1 parent 33ea1fd commit 0eb9873
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 24 deletions.
112 changes: 94 additions & 18 deletions Core/ClientSML/src/sml_ClientAgent.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1466,13 +1497,28 @@ 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;
if (jarpath)
{
if (!isfile(jarpath))
{
std::cerr << "SpawnDebugger: jarparth '" << jarpath << "'is not a file" << std::endl;
return false;
}
p = jarpath;
Expand Down Expand Up @@ -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
Expand All @@ -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)
{
Expand All @@ -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();

Expand All @@ -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);
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);
}

Expand All @@ -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;
Expand All @@ -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
Expand Down
9 changes: 7 additions & 2 deletions Core/ClientSML/src/sml_ClientAgent.h
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -891,6 +895,7 @@ namespace sml
protected:
// for {Spawn, Kill}Debugger()
DebuggerProcessInformation* m_pDPI;
void ClearDebuggerProcessInformation();

};

Expand Down
13 changes: 9 additions & 4 deletions SoarCLI/soar_cli.h
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down

0 comments on commit 0eb9873

Please sign in to comment.