Skip to content

Commit

Permalink
Throw for bad param in GetParameterValue
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@garfieldnate
garfieldnate committed May 22, 2024
1 parent 0c644c3 commit 4d7a6e2
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 53 deletions.
110 changes: 57 additions & 53 deletions Core/ClientSML/src/sml_ClientIdentifier.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<WMElement*> m_Children ;

// The list of WMEs that are using this symbol as their identifier
// (Usually just one value in this list)
std::list<Identifier*> 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 ;
Expand All @@ -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<WMElement*>::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.
Expand All @@ -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<WMElement*>::iterator ChildrenIter ;
typedef std::list<WMElement*>::const_iterator ChildrenConstIter ;

ChildrenIter GetChildrenBegin()
{
return m_pSymbol->m_Children.begin() ;
Expand All @@ -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.
Expand All @@ -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).
Expand All @@ -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.
* (<ID> ^attribute value) - returns "value"
Expand All @@ -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".
Expand All @@ -233,30 +237,30 @@ 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 <code>" as a child of this identifier.
*************************************************************/
void AddErrorCode(int errorCode) ;

/*************************************************************
* @brief Returns the number of children
*************************************************************/
int GetNumberChildren()
{
return (int)m_pSymbol->m_Children.size() ;
}

/*************************************************************
* @brief Gets the n-th child.
* Ownership of this WME is retained by the agent.
Expand All @@ -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.
Expand All @@ -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
25 changes: 25 additions & 0 deletions Core/ClientSMLSWIG/Python/TestPythonSML.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down
7 changes: 7 additions & 0 deletions Core/ClientSMLSWIG/Tcl/TestTclSML.tcl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading

0 comments on commit 4d7a6e2

Please sign in to comment.