From 4d7a6e24c021fa3926285570b04e59bc378742e1 Mon Sep 17 00:00:00 2001 From: Nathan Glenn Date: Wed, 22 May 2024 15:10:32 -0500 Subject: [PATCH] Throw for bad param in GetParameterValue Previous behavior here was to segfault! An exception is much more user-friendly. Add the SWIG machinery for catching thrown exceptions in the SML binding libraries, too. We wrap every SML function with a try/catch. I don't know if this is actually heavy or not, but at least one implementation out there thought it was and went an alternative route where each method has to be explicitly marked to catch exceptions: https://github.com/KiCad/kicad-source-mirror/blob/47e4ebb32a1366d60649879381eac819f7c7131d/common/swig/ki_exception.i#L41 Add tests for TCL, Python and Java that demonstrate the exceptions being translated for each host language. We don't have C# tests yet T_T. Notice we did have to add the `atexit` handler back to prevent a segfault when the exception is not caught correctly; I don't know exactly why. Filed #464. Fixes #451. --- Core/ClientSML/src/sml_ClientIdentifier.h | 110 +++++++++--------- Core/ClientSMLSWIG/Python/TestPythonSML.py | 25 ++++ Core/ClientSMLSWIG/Tcl/TestTclSML.tcl | 7 ++ Core/ClientSMLSWIG/sml_ClientInterface.i | 19 +++ .../main/java/edu/umich/soar/TestJavaSML.java | 13 +++ 5 files changed, 121 insertions(+), 53 deletions(-) diff --git a/Core/ClientSML/src/sml_ClientIdentifier.h b/Core/ClientSML/src/sml_ClientIdentifier.h index e2a2d0d1bd..e1414d292f 100644 --- a/Core/ClientSML/src/sml_ClientIdentifier.h +++ b/Core/ClientSML/src/sml_ClientIdentifier.h @@ -31,40 +31,40 @@ namespace sml class StringElement ; class WorkingMemory ; class Identifier ; - + // Two identifiers (two wmes) can have the same value because this is a graph not a tree // so we need to represent that separately. class EXPORT IdentifierSymbol { friend class Identifier ; // Provide direct access to children. - + protected: // The value for this id, which is a string identifier (e.g. I3) // We'll use upper case for Soar IDs and lower case for client IDs // (sometimes the client has to generate these before they are assigned by the kernel) std::string m_Symbol ; - + // The list of WMEs owned by this identifier. // (When we delete this identifier we'll delete all these automatically) std::list m_Children ; - + // The list of WMEs that are using this symbol as their identifier // (Usually just one value in this list) std::list m_UsedBy ; - + // This is true if the list of children of this identifier was changed. The client chooses when to clear these flags. bool m_AreChildrenModified ; - + public: IdentifierSymbol(Identifier* pIdentifier) ; ~IdentifierSymbol() ; - + char const* GetIdentifierSymbol() { return m_Symbol.c_str() ; } void SetIdentifierSymbol(char const* pID); - + bool AreChildrenModified() { return m_AreChildrenModified ; @@ -73,47 +73,47 @@ namespace sml { m_AreChildrenModified = state ; } - + // Indicates that an identifier is no longer using this as its value void NoLongerUsedBy(Identifier* pIdentifier); void UsedBy(Identifier* pIdentifier); - + bool IsFirstUser(Identifier* pIdentifier) { if (m_UsedBy.size() == 0) { return false ; } - + Identifier* front = m_UsedBy.front() ; return (front == pIdentifier) ; } - + Identifier* GetFirstUser() { return m_UsedBy.front() ; } - + int GetNumberUsing() { return (int)m_UsedBy.size() ; } - + // Have this identifier take ownership of this WME. So when the identifier is deleted // it will delete the WME. void AddChild(WMElement* pWME) ; WMElement* GetChildByTimeTag(long long timeTag); void TransferChildren(IdentifierSymbol* pDestination); void DeleteAllChildren() ; - + void RemoveChild(WMElement* pWME) ; - + void DebugString(std::string& result); - + private: std::list::iterator FindChildByTimeTag(long long timeTag); } ; - + // // Special note about output-link WMEs: The agent is // free to remove WMEs from the output-link at any time. @@ -134,11 +134,11 @@ namespace sml friend class IntElement ; friend class FloatElement ; friend class OutputDeltaList ; // Allow it to clear are children modified - + public: typedef std::list::iterator ChildrenIter ; typedef std::list::const_iterator ChildrenConstIter ; - + ChildrenIter GetChildrenBegin() { return m_pSymbol->m_Children.begin() ; @@ -147,52 +147,52 @@ namespace sml { return m_pSymbol->m_Children.end() ; } - + private: void ReleaseSymbol(); - + protected: // Two identifiers (i.e. two wmes) can share the same identifier value // So each identifier has a pointer to a symbol object, but two could share the same object. IdentifierSymbol* m_pSymbol ; - + IdentifierSymbol* GetSymbol() { return m_pSymbol ; } void UpdateSymbol(IdentifierSymbol* pSymbol); - + void SetSymbol(IdentifierSymbol* p_ID) ; - + void RecordSymbolInMap(); - + public: virtual char const* GetValueType() const ; - + // Returns a string form of the value stored here. virtual char const* GetValueAsString() const { return m_pSymbol->GetIdentifierSymbol() ; } - + // Returns a string form of the value, uses buf to create the string virtual char const* GetValueAsString(std::string& buf) const { buf.assign(m_pSymbol->GetIdentifierSymbol()); return buf.c_str(); } - + // The Identifier class overrides this to return true. (The poor man's RTTI). virtual bool IsIdentifier() const { return true ; } - + virtual Identifier* ConvertToIdentifier() { return this; } - + /************************************************************* * @brief Searches for a child of this identifier that has the given * time tag. @@ -201,7 +201,7 @@ namespace sml * @param timeTag The tag to look for (e.g. +12 for kernel side or -15 for client side) *************************************************************/ WMElement* FindFromTimeTag(long long timeTag) const ; - + /************************************************************* * @brief Returns the n-th WME that has the given attribute * and this identifier as its parent (or NULL). @@ -212,7 +212,7 @@ namespace sml * (> 0 only needed for multi-valued attributes) *************************************************************/ WMElement* FindByAttribute(char const* pAttribute, int index) const ; - + /************************************************************* * @brief Returns the value (as a string) of the first WME with this attribute. * ( ^attribute value) - returns "value" @@ -222,9 +222,13 @@ namespace sml char const* GetParameterValue(char const* pAttribute) const { WMElement* pWME = FindByAttribute(pAttribute, 0) ; - return pWME ? pWME->GetValueAsString() : NULL ; + if (pWME == NULL){ + std::string err_msg = "[Error]: " + this->m_AttributeName + " has no child attribute \"" + std::string(pAttribute) + "\""; + throw std::invalid_argument(err_msg); + } + return pWME->GetValueAsString(); } - + /************************************************************* * @brief Returns the "command name" for a top-level identifier on the output-link. * That is for output-link O1 (O1 ^move M3) returns "move". @@ -233,22 +237,22 @@ namespace sml { return this->GetAttribute() ; } - + /************************************************************* * @brief Adds "^status complete" as a child of this identifier. *************************************************************/ void AddStatusComplete() ; - + /************************************************************* * @brief Adds "^status error" as a child of this identifier. *************************************************************/ void AddStatusError() ; - + /************************************************************* * @brief Adds "^error-code " as a child of this identifier. *************************************************************/ void AddErrorCode(int errorCode) ; - + /************************************************************* * @brief Returns the number of children *************************************************************/ @@ -256,7 +260,7 @@ namespace sml { return (int)m_pSymbol->m_Children.size() ; } - + /************************************************************* * @brief Gets the n-th child. * Ownership of this WME is retained by the agent. @@ -265,7 +269,7 @@ namespace sml * but we want to export this interface to Java/Tcl etc. and this is easier. *************************************************************/ WMElement* GetChild(int index) ; - + /************************************************************* * @brief This is true if the list of children of this identifier has changed. * The client chooses when to clear these flags. @@ -274,51 +278,51 @@ namespace sml { return m_pSymbol->AreChildrenModified() ; } - + StringElement* CreateStringWME(char const* pAttribute, char const* pValue); IntElement* CreateIntWME(char const* pAttribute, long long value) ; FloatElement* CreateFloatWME(char const* pAttribute, double value) ; Identifier* CreateIdWME(char const* pAttribute) ; Identifier* CreateSharedIdWME(char const* pAttribute, Identifier* pSharedValue) ; - + protected: // This version is only needed at the top of the tree (e.g. the input link) Identifier(Agent* pAgent, char const* pAttributeName, char const* pIdentifier, long long timeTag); - + // The normal case (where there is a parent id) Identifier(Agent* pAgent, Identifier* pParent, char const* pID, char const* pAttributeName, char const* pIdentifier, long long timeTag) ; Identifier(Agent* pAgent, IdentifierSymbol* pParentSymbol, char const* pID, char const* pAttributeName, char const* pIdentifier, long long timeTag) ; - + // The shared id case (where there is a parent id and value is an identifier that already exists) Identifier(Agent* pAgent, Identifier* pParent, char const* pID, char const* pAttributeName, Identifier* pLinkedIdentifier, long long timeTag) ; Identifier(Agent* pAgent, IdentifierSymbol* pParentSymbol, char const* pID, char const* pAttributeName, IdentifierSymbol* pLinkedIdentifierSymbol, long long timeTag) ; - + virtual ~Identifier(void); - + void AddChild(WMElement* pWME) { m_pSymbol->AddChild(pWME) ; } - + void RemoveChild(WMElement* pWME) { m_pSymbol->RemoveChild(pWME) ; } - + #ifdef SML_DIRECT virtual void DirectAdd(Direct_AgentSML_Handle pAgentSML, long long timeTag) ; #endif - + // Send over to the kernel again virtual void Refresh() ; - + private: // NOT IMPLEMENTED Identifier(const Identifier& rhs); Identifier& operator=(const Identifier& rhs); - + }; - + } // namespace #endif // SML_IDENTIFIER_H diff --git a/Core/ClientSMLSWIG/Python/TestPythonSML.py b/Core/ClientSMLSWIG/Python/TestPythonSML.py index 239dd1cf71..00481a5984 100755 --- a/Core/ClientSMLSWIG/Python/TestPythonSML.py +++ b/Core/ClientSMLSWIG/Python/TestPythonSML.py @@ -6,6 +6,7 @@ # destruction (and maybe some other things, too). # # This file needs to be compatible with python 3.5, to run on CI jobs testing the lowest supported python version. +import atexit from pathlib import Path import sys import os @@ -90,6 +91,19 @@ def UserMessageCallback(id, tester, agent, clientName, message): else: print('✅ Kernel creation succeeded') +# Neglecting to shut down the kernel causes a segfault, so we use atexit to guarantee proper cleanup +# TODO: This should be handled by the Python SML interface automatically. +def __cleanup(): + global kernel + try: + if kernel: + kernel.Shutdown() + del kernel + except NameError: + pass + +atexit.register(__cleanup) + agent_create_called = CalledSignal() agentCallbackId0 = kernel.RegisterForAgentEvent(smlEVENT_AFTER_AGENT_CREATED, AgentCreatedCallback, agent_create_called) @@ -189,6 +203,17 @@ def test_agent_reinit(agent): test_agent_reinit(agent) +def test_catch_exception(agent): + input_link = agent.GetInputLink() + try: + input_link.GetParameterValue("non-existent") + except ValueError as e: + print("✅ Correctly caught exception:", e) + else: + assert False, "❌ No exception caught" + +test_catch_exception(agent) + def test_agent_destroy(agent): assert not agent_destroy_called.called, "❌ Agent destroy handler called before destroy" kernel.DestroyAgent(agent) diff --git a/Core/ClientSMLSWIG/Tcl/TestTclSML.tcl b/Core/ClientSMLSWIG/Tcl/TestTclSML.tcl index f905c2388e..ff59b3b4c0 100644 --- a/Core/ClientSMLSWIG/Tcl/TestTclSML.tcl +++ b/Core/ClientSMLSWIG/Tcl/TestTclSML.tcl @@ -185,6 +185,13 @@ if { [string first "^rhstest success" [$kernel ExecuteCommandLine "print s1" Soa set result [$kernel ExecuteCommandLine "init-soar" Soar1] +set input_link [$agent GetInputLink] +if {[catch {$input_link GetParameterValue "non-existent"} msg]} { + puts "✅Correctly caught error:\n$errorInfo\n" +} else { + puts "❌ Catching error FAILED" +} + $kernel DestroyAgent $agent # remove all the remaining kernel callback functions (not required, just to test) diff --git a/Core/ClientSMLSWIG/sml_ClientInterface.i b/Core/ClientSMLSWIG/sml_ClientInterface.i index 4a5c5b6888..094f3a59f8 100644 --- a/Core/ClientSMLSWIG/sml_ClientInterface.i +++ b/Core/ClientSMLSWIG/sml_ClientInterface.i @@ -10,6 +10,25 @@ // see https://www.swig.org/Doc4.1/Windows.html#Windows_interface_file %include +// Language-independent exception handler to wrap ALL functions with +// See https://www.swig.org/Doc4.2/SWIGDocumentation.html#Customization_nn7 +// As more exceptions are added to the codebase, we should add translations here +%include exception.i +%exception { + try { + $action + } + catch(const std::invalid_argument& e) { + SWIG_exception(SWIG_ValueError, e.what()); + } + catch (const std::exception& e) { + SWIG_exception(SWIG_RuntimeError, e.what()); + } + catch(...) { + SWIG_exception(SWIG_RuntimeError, "Unknown exception"); + } +} + // // These functions need to be renamed because they only differ by a const type, which isn't enough to distinguish them // diff --git a/Java/SMLJava/src/main/java/edu/umich/soar/TestJavaSML.java b/Java/SMLJava/src/main/java/edu/umich/soar/TestJavaSML.java index cd66715f95..b48eef71d1 100644 --- a/Java/SMLJava/src/main/java/edu/umich/soar/TestJavaSML.java +++ b/Java/SMLJava/src/main/java/edu/umich/soar/TestJavaSML.java @@ -374,6 +374,19 @@ private void Test() throw new IllegalStateException("bug 1028 test fail"); } + // check exception translation + boolean caught = false; + try { + pInputLink.GetParameterValue("non-existent"); + } catch (IllegalArgumentException e) { + caught = true; + System.out.println("Correctly caught exception: " + e); + } + if (!caught) { + throw new IllegalStateException("Failed to catch exception!"); + } + + // Clean up m_Kernel.Shutdown();