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();