FGScript::LoadScript function throws exception instead of returning false

[AirshipSim]

I'm submitting a ...

• [X ] bug report

Describe the issue

If the initialization file given in a script has a typo, or can not be found, the FGScript::LoadScript function throws an exception. When JSBSim is used as a dll, this crashes the main program.

What is the current behavior?

Currently, the FGScript::LoadScript function throws an exception in a couple of corner cases, instead of simply returning false like it does in dozen other cases.

Steps to reproduce:
In a command prompt, check that the following example script works fine:
> JSBSim.exe --script=scripts/c1721.xml

Now, go to scripts/c1721.xml, and add a typo to the initialization file, normally named reset00:

<use aircraft="c172r" initialize="reset00"/> => <use aircraft="c172r" initialize="reset00typo"/>

Run the command prompt again:
> JSBSim.exe --script=scripts/c1721.xml

The end of your output should look like this:

    Function: aero/coefficient/Cnda
    Function: aero/coefficient/Cndr
Could not open file: Path "aircraft/c172r/reset00typo.xml"
FATAL ERROR: JSBSim terminated with an exception.
The message was: File: Path "aircraft/c172r/reset00typo" could not be read.

File: Path "aircraft/c172r/reset00typo" could not be read.

As can be seen, an exception is thrown together with three redundant error messages:

1. Could not open file...
2. The message was: ... could not be read.
3. File: Path ... could not be read

This behavior is probably an oversight / bug, as there are no visible reason for FGInitialCondition::Load to throw exception instead of returning false. It is innocuous when JSBSim is used in cli / integrated code, but revealed when JSBSim is compiled as dll.

What is the expected behavior?

Would be expected that LoadScript simply returns false, in which case the program would still log two error messages and would still terminate properly. As a dll, JSBSim would not crash the main program.

Note that in the code, LoadScript() already returns false in a dozen of cases, without throwing. But at some point, it calls bool FGInitialCondition::Load() ! IC->Load(...).

jsbsim/src/input_output/FGScript.cpp

Lines 195 to 200 in 98c0333

	auto IC = FDMExec->GetIC();
	if ( ! IC->Load( initialize )) {
	FGLogging log(LogLevel::ERROR);
	log << "Initialization unsuccessful\n";
	return false;
	}

In a couple of cases, it is that latter function, which could also simply return false, which throws a LogException instead.

jsbsim/src/initialization/FGInitialCondition.cpp

Lines 1021 to 1034 in 98c0333

	// Make sure that the document is valid
	if (!document) {
	LogException err;
	err << "File: " << init_file_name << " could not be read.\n";
	throw err;
	}
	if (document->GetName() != "initialize") {
	LogException err;
	err << "File: " << init_file_name << " is not a reset file.\n";
	throw err;
	}
	bool result = false;

What is the motivation / use case for changing the behavior?

Using JSBSim as a dll without adding code in the main program to catch the exception. At the moment, the main program must use two paths of failure, one if LoadScript returns false (as expected from an API), and another one if LoadScript throws an exception (should remain internal to JSBSim):

try {
	if (!Exec->LoadScript(SGPath{ ResetPath}, dt, SGPath{ InitPath }))
	{
		return false; 
	}
}
catch (const JSBSim::LogException&) {
	return false;
}

Please tell us about your environment:

• OS: all
• JSBSim version: repo main branch as of today
• Language if applicable (C++, Python, Julia, ...)

Other information

• I propose to either remove the exceptions entirely from bool FGInitialCondition::Load(const SGPath& rstfile, bool useAircraftPath), as a user typo does not seem to warrant an exception,

• or to add a try / catch in this same function, where the exceptions are thrown (func would still return false == error value). This way the exception remains internal to JSBSim.

Any feedback welcome!

[bcoconni]

Thanks for reporting this topic @AirshipSim

There are several layers to this onion 😄 :

• Crashes caused by uncaught exceptions thrown by JSBSim
• Whether an exception should be thrown or not
• Error recovery: what is the procedure a user should follow when JSBSim reports an error, either an exception or an error code.

Uncaught exceptions

My opinion is that it is the responsibility of the caller to manage the exceptions thrown by the callee and at the very least to put in place a safety net for uncaught exceptions. This is typical defensive programming and a crash generated by an uncaught exception thrown by JSBSim is certainly not JSBSim's fault. All exceptions thrown by JSBSim are inheriting from JSBSim::BaseException so that they can be identified properly and processed accordingly.

Exception vs error code ?

There used to be times where JSBSim was calling exit() (see commits 24b76b7 and 9362e4f) and that was evil: a library should always give the opportunity to its caller to recover. Some of the existing exceptions are the legacy of that exit() policy where the calls to exit() have been downgraded to throwing exceptions.

Exceptions vs error codes is a classical debate and, to your point, consistency should be taken into account. Error codes can be ignored or swallowed while exceptions are harder to ignore. On the other hand, when properly processed, error codes give a much cleaner code than exception management.

That is a topic that can definitely be discussed and there is most likely a number of places in JSBSim where an error code could be used to manage errors instead of exceptions.

Error recovery

JSBSim is historically not that good at error recovery. Memory is managed in a lot of places by (not so ?) good old new/delete pairs and unfortunately not every call to new is matched by a call to delete, especially when dealing with errors. That's a topic this is being discussed in the issue #1248.

In addition, when JSBSim interrupts the initialization/loading of an object the object is very likely left in a half baked state and there is no guarantee that a new call to FGScript::LoadScript would behave the expected way even if the error that caused the exception has been corrected in between. My advice is that if an error (whether exception or error code) occurs during initialization, your better course of action is to restart the process from scratch with a pristine new instance of FGFDMExec. From that point of view, throwing an exception sends IMHO a much stronger message than an error code that can inadvertently be ignored.

Way forward

The error catcher shall certainly not be in JSBSim, at least not without a good reaon. Either we decide to replace the exception by an error code or the caller will have to do its job properly (i.e. manage exceptions).

And while we are investigating whether an exception is appropriate or not for this case, we could take the opportunity to check if the error is properly managed in JSBSim.

[AirshipSim]

Thanks for your feedback @bcoconni

Your comment clarifies a lot. Now, to narrow it down to our particular case, please note the following:

Here is how LoadScript handles the failure to read document, which is an Element*, deduced from the parameter const SGPath& script:

bool FGScript::LoadScript(const SGPath& script, double default_dT, const SGPath& initfile)
{
...
  Element* document = XMLFileRead.LoadXMLDocument(script);

  if (!document) {
    cerr << "File: " << script << " could not be loaded." << endl;
    return false;
  }
....

The failure is handled with a return false, consistent with the function error handling. Notice the 3rd parameter const SGPath& initfile. It is then passed to IC->Load(), the function below, through the parameter const SGPath& rstfile:

bool FGInitialCondition::Load(const SGPath& rstfile, bool useAircraftPath) // rstfile is initfile
{
...
  FGXMLFileRead XMLFileRead;
  Element* document = XMLFileRead.LoadXMLDocument(init_file_name);

  // Make sure that the document is valid
  if (!document) {
    LogException err;
    err << "File: " << init_file_name << " could not be read.\n";
    throw err;
  }
...

We have the exact same variable declaration Element* document playing the same role, but this time !document is handled through exception. And the function still returns false for other kinds of errors... It looks like an oversight, or maybe a degraded "exit()" case from back in the days.

I would say, for consistency, this exception specifically should be replaced by a return false. It would then trigger a return false also for LoadScript. Of course, a PR should verify that nothing is left in an inconsistent state, and that all tests still pass. Since the exception is not handled at all at the moment, it is hard to imagine a less consistent state.

My opinion is that it is the responsibility of the caller to manage the exceptions thrown by the callee and at the very least to put in place a safety net for uncaught exceptions. This is typical defensive programming and a crash generated by an uncaught exception thrown by JSBSim is certainly not JSBSim's fault.

Well, we agree that in my original example, the first caller of FGInitialCondition::Load( ) is indeed FGScript::LoadScript( ). So the exception should have at least been handled in LoadScript( ) then.

Forward

I propose to write a PR for this particular case, changing the exception for a return false in FGInitialCondition::Load, and testing that loading a script with a typo still fails gracefully. Replacing the exception is better because it also allows to call FGInitialCondition::Load directly from a main program, without stumbling again on exceptions.

Any feedback appreciated

[seanmcleod70]

Just some context in terms of the number of locations, and 'module' in JSBSim that throw C++ exceptions.

https://github.com/search?q=repo%3AJSBSim-Team%2Fjsbsim%20throw&type=code

[AirshipSim]

Just some context in terms of the number of locations, and 'module' in JSBSim that throw C++ exceptions.

The question is, are those exceptions already guarded by try catch guards or not...

And are they redundant with another error handling or not...

[seanmcleod70]

Some are, but there definitely are a bunch that aren't.

There are also cases like these, i.e. a catch and then a throw.

jsbsim/src/models/propulsion/FGEngine.cpp

Lines 201 to 211 in 98c0333

	// Load thruster
	local_element = parent_element->FindElement("thruster");
	if (local_element) {
	try {
	LoadThruster(exec, local_element);
	} catch (std::string& str) {
	throw("Error loading engine " + Name + ". " + str);
	}
	} else {
	cerr << "No thruster definition supplied with engine definition." << endl;
	}

For this particular case I set a breakpoint, and then walked back up the stack to see if there were any catch blocks further up the stack.

And there weren't until I got back to main. Also note LoadScript() being on the stack frame.

And here is an example in the ctor for FGFDMExec, so using JSBSim as a library you can get an exception thrown from it's ctor.

jsbsim/src/FGFDMExec.cpp

Lines 137 to 144 in 98c0333

	// this is to catch errors in binding member functions to the property tree.
	try {
	Allocate();
	} catch (const BaseException& e) {
	LogException err;
	err << endl << "Caught error: " << e.what() << endl;
	throw err;
	}

[AirshipSim]

Thanks @seanmcleod70 ,

what is your opinion on the particular case that I described in this issue, and my proposition concerning FGInitialCondition::Load( )?

[seanmcleod70]

what is your opinion on the particular case that I described in this issue, and my proposition concerning FGInitialCondition::Load( )?

It looks like you're possibly trying to prevent the dyke from breaking by plugging holes with your fingers.

So, you have a specific proposal for FGScript::LoadScript() to handle an exception from FGInitialCondition::Load(), so that is 1 finger, but by chance I decided to set a breakpoint in FGEngine::Load() and confirmed that it can also throw an exception that would bubble up to FGScript::LoadScript().

How many fingers do you have? 😉

[AirshipSim]

Sorry, this is the proposition I am talking about:

Forward

I propose to write a PR for this particular case, changing the exception for a return false in FGInitialCondition::Load, and testing that loading a script with a typo still fails gracefully. Replacing the exception is better because it also allows to call FGInitialCondition::Load directly from a main program, without stumbling again on exceptions.

Any feedback appreciated

Should I write a PR for this case, which seems to me clear-cut and a way to improve consistency, or it's useless because nobody is going to validate it anyway for some reason?

[seanmcleod70]

Yes, that was the proposition I was referring to. So you modify FGInitialCondition::Load so that there is no exception if there is a typo in the initial conditions xml file. But as I pointed out, what about FGEngine::Load which can also bubble up an exception to FGScript::LoadScript?

You want to prevent an exception from FGScript::LoadScript but you will then have to find all possible exceptions that can be thrown from code it calls. Or instead of modifying FGInitialCondition::Load you would have to put a try-catch inside FGScript::LoadScript to prevent exceptions propagating. Although as you said, the advantage of your proposal was that other clients who might want to call FGInitialCondition::Load directly and not want to deal with exceptions.

Something to consider is that we have a large API surface, search for JSBSIM_API and you'll see how many classes we export. So, it's not the case that we have say a small API surface of only 10 functions, which then call into a large set of classes that implement JSBSim such that we could for example decide that we're not going to expose exceptions past the API boundary. In which case we could then in those 10 API functions wrap a try-catch block around further calls into the JSBSim implementation to convert any exceptions into error return codes.

Rather, without a major rework of exceptions within JSBSim your best bet if your program code is using JSBSim as a library is to handle/warp the handful of JSBSim APIs that you actually use.

[AirshipSim]

Although as you said, the advantage of your proposal was that other clients who might want to call FGInitialCondition::Load directly.

Yes, that allows me to have hot reload of initial conditions. That is my use case right now, let's forget about LoadScript( ) for the time being, this is only the way it first appeared to me. I have no intent to look at the 170 throws, I can only contribute as I go.

I already have a try catch on JSBSim function calls, that is not about my code, it is about the consistency of JSBSim. This. Either the current code is considered correct by the code maintainers and PR will be discarded if any, or this improvement is welcome as a positive step, even if only a drop in the ocean.

A go / no-go would be appreciated.

[seanmcleod70]

So looking at the consistency just of handling XMLFileRead.LoadXMLDocument(), there are only 5 instances:

• throw
• return false
• throw
• return NULL
• return false

So if we are going to potentially change 1 of the throws, we should definitely look at the other throw.

Just to be clear, FGScript::LoadScript() can still bubble up an exception from the call to XMLFileRead.LoadXMLDocument() itself over and above other exceptions later on like when loading the engine file etc.

Since XMLFileRead.LoadXMLDocument() calls into readXML() which throws exceptions for a number of cases, none of which are caught on the way back to FGScript::LoadScript().

jsbsim/src/simgear/xml/easyxml.cxx

Lines 254 to 305 in 98c0333

	void readXML (istream &input, XMLVisitor &visitor, const string &path)
	{
	XML_Parser parser = XML_ParserCreate(0);
	XML_SetUserData(parser, &visitor);
	XML_SetElementHandler(parser, start_element, end_element);
	XML_SetCharacterDataHandler(parser, character_data);
	XML_SetProcessingInstructionHandler(parser, processing_instruction);
	visitor.setParser(parser);
	visitor.setPath(path);
	visitor.startXML();
	char buf[16384];
	while (!input.eof()) {
	if (!input.good()) {
	std::stringstream s;
	s << "Problem reading input file " << path << endl;
	visitor.setParser(0);
	XML_ParserFree(parser);
	cerr << endl << s.str() << endl;
	throw JSBSim::BaseException(s.str());
	}
	input.read(buf,16384);
	if (!XML_Parse(parser, buf, input.gcount(), false)) {
	std::stringstream s;
	s << "In file " << path << ": line " << XML_GetCurrentLineNumber(parser) << endl
	<< "XML parse error: " << XML_ErrorString(XML_GetErrorCode(parser));
	cerr << endl << s.str() << endl;
	visitor.setParser(0);
	XML_ParserFree(parser);
	throw JSBSim::BaseException(s.str());
	}
	}
	// Verify end of document.
	if (!XML_Parse(parser, buf, 0, true)) {
	std::stringstream s;
	s << "In file " << path << ": line " << XML_GetCurrentLineNumber(parser) << endl
	<< "XML parse error: " << XML_ErrorString(XML_GetErrorCode(parser));
	cerr << endl << s.str() << endl;
	visitor.setParser(0);
	XML_ParserFree(parser);
	throw JSBSim::BaseException(s.str());
	}
	visitor.setParser(0);
	XML_ParserFree(parser);
	visitor.endXML();
	}

[AirshipSim]

Yes, it's a rabbit hole. This is why I can only propose this small step forward but stay away from large refactoring proposition. Can still add a try/catch to guard loadxmldoc in IC::Load, now that you pointed it out.

[bcoconni]

Not sure what the plan is with regard to the PR but it cannot simply be a replacement of one throw instruction by one return false. You also need to inspect all locations where the code is called from and make sure they are dealing appropriately with the return code (i.e. test and act according to the value which may mean reporting another error code).

That's one of the drawbacks of error return codes, you need to make sure that they are properly dealt with at all stages of all possible call stacks that lead to the throw at stake. And if you don't want to report to the caller then you have to recover which comes with its own challenges (see my previous comment).

While the exception is the easier option for the lazy programmer: you throw and let others decide how they should manage the issue. If no one cares then boom!

I am fine with addressing one single throw in the PR but am voting against the "hide everything under the carpet" option:

try {
	if (!Exec->LoadScript(SGPath{ ResetPath}, dt, SGPath{ InitPath }))
	{
		return false; 
	}
}
catch (const JSBSim::LogException&) {
	return false;
}

The problem with try/catch is that once they are there, it's very difficult to get rid of them. You can never be sure that they are not intercepting an exception thrown by a convoluted call stack. I'd rather keep the code as it is currently.

[bcoconni]

Also I am not sure that preventing all exceptions from escaping JSBSim is a reasonable policy.

For example in C++, exceptions are the only means to interrupt a constructor from completing its task. Of course there are possible workarounds but they require the caller to "manually" destroy the object instance if an error is reported which comes with the potential for memory leaks.

Not saying that JSBSim is currently using that "feature" but I don't think we want to put in place a policy that would prevent us from doing so.

[seanmcleod70]

Preventing any C++ exception crossing an API boundary is more common in cases where the library in question is designed to be a pure C API but internally uses C++ for the implementation. For example a number of the early Windows APIs are pure C APIs callable from C and easily callable from other languages like Delphi etc. where they have a fairly common pattern of returning some opaque handle, which gets returned from some create function and which is then passed into subsequent function which uses the context of the handle to retrieve state to use during the operation.

For example from GDI (Graphics Device Interface).

HDC hdcScreen = GetDC(hWnd); // Get screen DC
DrawText(hdc, text, -1, &rect, DT_CENTER | DT_WORDBREAK);

Which internally can be implemented using C++ classes/instances along these lines:

class GDI
{
private:
   state..
public:
   DrawText(....);
}

HDC GetDC(HWND hwnd)
{
   return new GDI();
}

int DrawText(HDC hdc, ....)
{
   try {
      GDI* pGDI = reinterpret_cast<GDI>(hdc);
      return pGDI->DrawText(...);
   } catch (...) {
      return -1;
   }
}

Given the C API, called from lots of different languages etc. a C++ exception can't be allowed to propagate across the API boundary.

However, JSBSim is a C++ library so unless there is some documentation explicitly stating that it doesn't throw any C++ exceptions ever, including catching any standard library exceptions from things like new etc. then any users of the C++ based JSBSim library have to be prepared to deal with C++ exceptions.