-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #5018 - C++ CLI: Calling a non existing method in a measure ends up with a crash and stack trace #5023
Conversation
This shows how to turn errors arising from Swig director calls into normal C++ exceptions. ref #5018
#if defined SWIGRUBY | ||
%feature("director:except") { | ||
// Mimic openstudio::evalString for a possible approach to reporting more info or a callstack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/measure/Measure.i
Outdated
// I just **cannot** figure out a way to get the error location in C, without calling $@.to_s. Seems nothing is available here | ||
int locationError; | ||
VALUE locval = rb_eval_string_protect("$@.to_s", &locationError); | ||
std::string loc; | ||
if (locationError == 0) { | ||
loc = StringValuePtr(locval); | ||
} else { | ||
loc = "Failed to get VM location"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit seems redundant with the backtrace block below, you get the same info twice.
src/measure/Measure.i
Outdated
#if defined SWIGPYTHON | ||
%feature("director:except") { | ||
|
||
if ($error != nullptr) { | ||
|
||
PyObject *exc_type = nullptr; | ||
PyObject *exc_value = nullptr; | ||
PyObject *exc_tb = nullptr; | ||
PyErr_Fetch(&exc_type, &exc_value, &exc_tb); | ||
PyErr_NormalizeException(&exc_type, &exc_value, &exc_tb); | ||
PyObject *str_exc_value = PyObject_Repr(exc_value); // Now a unicode object | ||
PyObject *pyStr2 = PyUnicode_AsEncodedString(str_exc_value, "utf-8", "Error ~"); | ||
Py_DECREF(str_exc_value); | ||
char *strExcValue = PyBytes_AsString(pyStr2); // NOLINT(hicpp-signed-bitwise) | ||
Py_DECREF(pyStr2); | ||
|
||
std::string err_msg = "In method '$symname': `" + std::string{strExcValue} + "`"; | ||
|
||
// See if we can get a full traceback. | ||
// Calls into python, and does the same as capturing the exception in `e` | ||
// then `print(traceback.format_exception(e.type, e.value, e.tb))` | ||
PyObject *pModuleName = PyUnicode_DecodeFSDefault("traceback"); | ||
PyObject *pyth_module = PyImport_Import(pModuleName); | ||
Py_DECREF(pModuleName); | ||
|
||
if (pyth_module == nullptr) { | ||
err_msg += "\nCannot find 'traceback' module, this should not happen"; | ||
throw Swig::DirectorMethodException(err_msg.c_str()); | ||
} | ||
|
||
PyObject *pyth_func = PyObject_GetAttrString(pyth_module, "format_exception"); | ||
Py_DECREF(pyth_module); // PyImport_Import returns a new reference, decrement it | ||
|
||
if (pyth_func || PyCallable_Check(pyth_func)) { | ||
|
||
PyObject *pyth_val = PyObject_CallFunction(pyth_func, "OOO", exc_type, exc_value, exc_tb); | ||
|
||
// traceback.format_exception returns a list, so iterate on that | ||
if (!pyth_val || !PyList_Check(pyth_val)) { // NOLINT(hicpp-signed-bitwise) | ||
err_msg += "\nIn reportPythonError(), traceback.format_exception did not return a list."; | ||
} else { | ||
unsigned long numVals = PyList_Size(pyth_val); | ||
if (numVals == 0) { | ||
err_msg += "\nNo traceback available"; | ||
} else { | ||
err_msg += "\nPython traceback follows:\n```"; | ||
|
||
for (unsigned long itemNum = 0; itemNum < numVals; itemNum++) { | ||
PyObject *item = PyList_GetItem(pyth_val, itemNum); | ||
if (PyUnicode_Check(item)) { // NOLINT(hicpp-signed-bitwise) -- something inside Python code causes warning | ||
std::string traceback_line = PyUnicode_AsUTF8(item); | ||
if (!traceback_line.empty() && traceback_line[traceback_line.length() - 1] == '\n') { | ||
traceback_line.erase(traceback_line.length() - 1); | ||
} | ||
err_msg += "\n" + traceback_line; | ||
} | ||
// PyList_GetItem returns a borrowed reference, do not decrement | ||
} | ||
|
||
err_msg += "\n```"; | ||
} | ||
} | ||
|
||
// PyList_Size returns a borrowed reference, do not decrement | ||
Py_DECREF(pyth_val); // PyObject_CallFunction returns new reference, decrement | ||
} | ||
Py_DECREF(pyth_func); // PyObject_GetAttrString returns a new reference, decrement it | ||
throw Swig::DirectorMethodException(err_msg.c_str()); | ||
} | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For python, I repurposed what Edwin and I wrote for EnergyPlus's PluginInstance::reportPythonError
(cf NREL/EnergyPlus#8181 for where I implemented the traceback bit)
class WrongMethodMeasure(openstudio.measure.ModelMeasure): | ||
def name(self): | ||
return "Wrong Method Measure" | ||
|
||
def modeler_description(self): | ||
return "The arguments method calls a non existing method on the model passed as argument" | ||
|
||
def arguments(self, model: typing.Optional[openstudio.model.Model] = None): | ||
model.nonExistingMethod() | ||
args = openstudio.measure.OSArgumentVector() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test for both python and ruby that calls a wrong method on model passed as argument in arguments()
class BadMeasure(openstudio.measure.ModelMeasure): | ||
def name(self): | ||
return "Bad Measure" | ||
|
||
def modeler_description(self): | ||
return "The arguments method calls another_method which does a raise ValueError" | ||
|
||
def another_method(self): | ||
raise ValueError("oops") | ||
|
||
def arguments(self, model: typing.Optional[openstudio.model.Model] = None): | ||
self.another_method() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both Python and Ruby have this failed measure, that calls another method that throws, so we can check if the traceback is good
// initialize for each test | ||
virtual void SetUp() override { | ||
std::vector<std::string> args; | ||
thisEngine = std::make_unique<openstudio::ScriptEngineInstance>("rubyengine", args); | ||
|
||
(*thisEngine)->registerType<openstudio::measure::OSMeasure*>("openstudio::measure::OSMeasure *"); | ||
(*thisEngine)->registerType<openstudio::measure::ModelMeasure*>("openstudio::measure::ModelMeasure *"); | ||
(*thisEngine)->exec("OpenStudio::init_rest_of_openstudio()"); | ||
} | ||
// tear down after each test | ||
virtual void TearDown() override { | ||
// Want to ensure the engine is reset regardless of the test outcome, or it may throw a segfault | ||
thisEngine->reset(); | ||
} | ||
|
||
std::unique_ptr<openstudio::ScriptEngineInstance> thisEngine; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @kbenne I slightly changed the test you wrote so that the fixture will always reset the engine, regardless of whether the test passed or failed.
std::string expected_exception = fmt::format(R"(SWIG director method error. RuntimeError: oops | ||
["/Users/julien/Software/Others/OpenStudio2/ruby/engine/test/BadMeasure/measure.rb:12:in `another_method'", "/Users/julien/Software/Others/OpenStudio2/ruby/engine/test/BadMeasure/measure.rb:16:in `arguments'"] | ||
|
||
Traceback: | ||
{0}:12:in `another_method' | ||
{0}:16:in `arguments')", | ||
scriptPath.generic_string()); | ||
|
||
openstudio::model::Model model; | ||
try { | ||
measurePtr->arguments(model); | ||
ASSERT_FALSE(true) << "Expected measure arguments(model) to throw"; | ||
} catch (std::exception& e) { | ||
std::string error = e.what(); | ||
EXPECT_EQ(expected_exception, error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ruby traceback example
SWIG director method error. RuntimeError: oops
["/Users/julien/Software/Others/OpenStudio2/ruby/engine/test/BadMeasure/measure.rb:12:in `another_method'", "/Users/julien/Software/Others/OpenStudio2/ruby/engine/test/BadMeasure/measure.rb:16:in `arguments'"]
Traceback:
/Users/julien/Software/Others/OpenStudio2/ruby/engine/test/BadMeasure/measure.rb:12:in `another_method'
/Users/julien/Software/Others/OpenStudio2/ruby/engine/test/BadMeasure/measure.rb:16:in `arguments'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hence why I just said the "$@.to_s"
in EvalString (and now measure.i) is redundant if the traceback is available.
std::string expected_exception = | ||
fmt::format(R"(SWIG director method error. NoMethodError: undefined method `nonExistingMethod' for #<OpenStudio::Model::Model:ADDRESS> | ||
["/Users/julien/Software/Others/OpenStudio2/ruby/engine/test/WrongMethodMeasure/measure.rb:12:in `arguments'"] | ||
|
||
Traceback: | ||
{0}:12:in `arguments')", | ||
scriptPath.generic_string()); | ||
|
||
openstudio::model::Model model; | ||
try { | ||
measurePtr->arguments(model); | ||
ASSERT_FALSE(true) << "Expected measure arguments(model) to throw"; | ||
} catch (std::exception& e) { | ||
std::string error = e.what(); | ||
EXPECT_EQ(expected_exception, stripAddressFromErrorMessage(error)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gotta strip the "0x00283498743eff" address here because it naturally changes everry time
std::string expected_exception = fmt::format(R"(SWIG director method error. In method 'arguments': `ValueError('oops')` | ||
Python traceback follows: | ||
``` | ||
Traceback (most recent call last): | ||
File "{0}", line 17, in arguments | ||
self.another_method() | ||
File "{0}", line 14, in another_method | ||
raise ValueError("oops") | ||
ValueError: oops | ||
```)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python tracebacks!
RuntimeError: SWIG director method error. In method 'arguments': `ValueError('oops')`
Python traceback follows:
` ` `
Traceback (most recent call last):
File "/Users/julien/Software/Others/OpenStudio2/python/engine/test/BadMeasure/./measure.py", line 14, in arguments
self.another_method()
File "/Users/julien/Software/Others/OpenStudio2/python/engine/test/BadMeasure/./measure.py", line 11, in another_method
raise ValueError("oops")
ValueError: oops
` ` `
std::string expected_exception = | ||
fmt::format(R"(SWIG director method error. In method 'arguments': `AttributeError("'Model' object has no attribute 'nonExistingMethod'")` | ||
Python traceback follows: | ||
``` | ||
Traceback (most recent call last): | ||
File "{}", line 14, in arguments | ||
model.nonExistingMethod() | ||
AttributeError: 'Model' object has no attribute 'nonExistingMethod' | ||
```)", | ||
scriptPath.generic_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WrongMethodMeasure for python
The fallback is needed for Stack Too deep
…rting in that case: show 8 early lines + 12 final
warning C4244: ‘initializing’: conversion from ‘Py_ssize_t’ to ‘unsigned long’,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this togethor @jmarrec !
@@ -96,6 +96,71 @@ | |||
}; | |||
|
|||
#if defined SWIGRUBY | |||
%feature("director:except") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also contemplated moving this up to a higher level. I would be fine with that, but I opted to keep it "local" to where the directors are defined. I thought there was a bit of logic in that. Either way is really ok.
CI Results for ded922f:
|
Pull request overview
%feature("director:except")
SwigDirector
we have, not just inMeasure.i
. Maybe in utilities/core?Pull Request Author
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.