Playing well with REPLs

[bcoconni]

JSBSim has traditionally been employed within monolithic applications (such as FlightGear), where any JSBSim error generally resulted in the termination of the entire program. This assumption has simplified error handling within JSBSim itself: errors were considered unrecoverable since the calling application was expected to terminate when they occurred.

These days are gone now that we are providing a Python module and a Matlab S-function. This has opened up new possibilities for using JSBSim in interactive Read-Eval-Print-Loop (REPL) environments. This shift necessitates a reassessment and upgrade of our error handling and object state management strategies.

Specifically, this issue aims to discuss the following key requirements for robust JSBSim usage within REPLs (including, but not limited to, Python and Matlab):

1. REPL Stability: Under no circumstances should JSBSim cause the REPL to crash or terminate unexpectedly. Sudden REPL terminations are extremely frustrating for users, as it can lead to the loss of unsaved work and create a perception of poor software quality.
2. Robust Object State Management: Users working in a REPL environment often engage in exploratory, trial-and-error workflows. JSBSim objects may therefore exist in various intermediate or inconsistent states due to user errors. JSBSim should handle these states gracefully, allowing users to recover and continue their work without requiring, for example, the complete destruction and recreation of their instance of FGFDMExec. This implies the need for mechanisms to:
   • Detect and report errors clearly and informatively.
   • Provide methods for users to query the state of an object?
   • Offer options to reset or correct invalid object configurations, such as the ability to call FGFDMExec::Load again after a previous failure.
   • Prevent operations that would lead to crashes or undefined behavior when an object is in an invalid state. Typically prevent executing FGFDMExec::Run if FGFDMExec::Load has failed.

To address these requirements, the following work areas should be addressed:

1. Memory Management: Implement more rigorous memory management practices to prevent leaks, especially when the execution is leaving the happy path (i.e. the ideal execution flow of a program, where all conditions are met, and no errors occur).
   • For example, pull requests Fix memory leaks in FGSwitch #1230 and Memory leak fix and general improvements of FGDistributor #1245 addressed memory leaks that occurred during error conditions, which was previously not a concern since the operating system would automatically reclaim all reserved memory upon program termination.
   • The straightforward way to address this requirement is to replace all the calls to new and delete by smart pointers (i.e. unique_ptr and shared_ptr).
2. Error Reporting: Ensure that error messages are clear, informative, and provide users with a clear understanding of the severity of the error. There should be no silently ignored or handled errors.
   • When JSBSim makes assumptions to address an error and continue execution, it should provide a message explaining the corrective action taken.
   • The on-going work on the new logging system is also addressing that point by adding a severity level (i.e. LogLevel) to each error message.
3. Exception Handling: Implement robust exception handling, ensuring that exceptions are caught where appropriate, or converted to a REPL-compatible error format (e.g., converting C++ exceptions to Python exceptions).

This issue serves as a starting point for discussing the necessary modifications and best practices to ensure that JSBSim can be used effectively and reliably in interactive environments. Future pull requests that contribute to this objective should reference this issue, to maintain a clear connection to the requirements and goals outlined here.

[bcoconni]

The straightforward way to address this requirement is to replace all the calls to new and delete by smart pointers (i.e. unique_ptr and shared_ptr).

Due to the design of JSBSim, this task is trivial in most cases and can be made by a newcomer to JSBSim so I'm flagging this issue as a good first issue.

[XueSongTap]

Can I work on this？

[bcoconni]

@XueSongTap, yes sure !

Just make sure to avoid submitting a huge PR with all the classes migrated to smart pointers at once. I'd rather suggest to start with a PR that just contains the changes for 1 or 2 classes and start the discussion from there.

I don't know how familiar you are with JSBSim code but FYI JSBSim is designed to allocate all its memory during initialization which is done in 2 steps:

1. General initialization where all the classes inheriting FGModel are instantiated.
2. Loading of the XML files where the allocated memory depends on the content of the XML files.

While running the simulation loop, JSBSim does not allocate nor release any memory.

All the memory is released when the object FGFDMExec is deleted.

[agodemar]

That's a great idea @bcoconni - I also envision interesting use cases in Julia within Jupyter notebooks.

[XueSongTap]

I'd like to contribute to this issue by starting with migrating a couple of components to use smart pointers. Based on the repository structure and the recommendations in this thread, I plan to work on:

models/flight_control/FGGain.cpp/FGGain.h
models/flight_control/FGSummer.cpp/FGSummer.h
These seem like relatively simple control system components that would make a good starting point. I believe they're fairly self-contained and would help establish patterns that could be applied to other components.

Since this would be my first contribution to JSBSim, I'd appreciate any guidance or feedback. Could someone from the team confirm if these would be appropriate components to begin with?

Thank you!

[bcoconni]

I plan to work on:

models/flight_control/FGGain.cpp/FGGain.h
models/flight_control/FGSummer.cpp/FGSummer.h

I agree FGGain.h is a good start but FGSummer.h does not allocate memory dynamically so there is nothing to change there.

In FGGain.h, you 'll notice that the type of Gain is FGParameter_ptr:

jsbsim/src/models/flight_control/FGGain.h

Lines 225 to 227 in e1ea4e8

	private:
	FGTable* Table;
	FGParameter_ptr Gain;

And FGParameter_ptr is a typedef for an home-made shared pointer SGSharedPtr.

jsbsim/src/math/FGParameter.h

Line 72 in e1ea4e8

typedef SGSharedPtr<FGParameter> FGParameter_ptr;

We might consider replacing the pointers using SGSharedPtr later with C++ standard smart pointers but for the moment, I suggest we leave them aside since they already are smart pointers.

Since this would be my first contribution to JSBSim, I'd appreciate any guidance or feedback. Could someone from the team confirm if these would be appropriate components to begin with?

Sure, I suggest you submit a PR with your proposal and we'll discuss your code from there.

[bcoconni]

Some further thoughts about exceptions in JSBSim.

I have this intuition that we should only throw exceptions when reading the XML files i.e. when calling the method FGModel::Load() and its siblings. In this context, throwing an exception means that JSBSim cannot load the data and aborts. That's OK, the user is expected to fix the problem and try again. For that scenario, we still need to add a try/catch in FGFDMExec::LoadScript and FGModel::LoadModel to clean everything up before returning but basically we are managing.

But what about exceptions thrown during the simulation loop, such as the one below ?

jsbsim/src/models/FGPropagate.cpp

Lines 360 to 363 in e1ea4e8

	case eBuss1:
	case eBuss2:
	case eLocalLinearization:
	throw("Can only use Buss (1 & 2) or local linearization integration methods in for rotational position!");

Here the simulation loop will abort and leave JSBSim in an unpredictable state. Hopefully, the caller has an exception handler or it will be terminated by the OS. And even so, assuming the user will fix the property simulation/integrator/position/translational (or whatever property was improperly set), I am very skeptical that the simulation could be resumed.

For the particular case of this exception, I think that the error (setting the property to an illegal value) should be intercepted earlier i.e. at the very moment the property is modified, when an error message could be issued and the property left to its current legal value which would allow the simulation to continue. So we should be able to avoid throwing an exception in this case.

Generally speaking, the idea is to use exceptions only during loading and avoid using them otherwise. I am not sure this policy is reasonable or even applicable to JSBSim but it makes sense to me.

Thoughts ?

[gallonmate]

That makes sense to keep exceptions for XML loading.

Expanding on your idea. I noticed logging is configurable to different levels. So what if exceptions are made configurable to different options as well? And maybe having either a delegate, a function, variable, or a class, etc - basically some way that can universally be handled by the REPLs as a way to gracefully handle JSBSim errors/exceptions.

For example if someone is running a standalone instance of JSBSim, it defaults to a basic level of exceptions, throwing the exceptions where it makes sense to. And then when JSBSim is used with REPLs or attached/embedded/etc with other programs, those users can select their desired level of exception. So that they can turn off JSBSim exceptions completely, and in place off JSBSim exceptions, then a delegate or some other means is called, so the REPLs/other programs can see there is an JSBSim error/exception and halt their simulation gracefully without crashing everything.

[bcoconni]

[...] basically some way that can universally be handled by the REPLs as a way to gracefully handle JSBSim errors/exceptions.

Maybe I'm misunderstanding your point but we are already doing that, at least for some methods of the Python classes: C++ exceptions are converted to Python's.

jsbsim/python/ExceptionManagement.h

Lines 33 to 57 in e1ea4e8

	void convertJSBSimToPyExc()
	{
	try {
	if (!PyErr_Occurred())
	throw;
	}
	catch (const JSBSim::TrimFailureException& e) {
	PyErr_SetString(trimfailure_error, e.what());
	}
	catch (const GeographicLib::GeographicErr& e) {
	PyErr_SetString(geographic_error, e.what());
	}
	catch (const JSBSim::BaseException& e) {
	PyErr_SetString(base_error, e.what());
	}
	catch (const JSBSim::FloatingPointException& e) {
	PyErr_SetString(e.getPyExc(), e.what());
	}
	catch (const std::string &msg) {
	PyErr_SetString(PyExc_RuntimeError, msg.c_str());
	}
	catch (const char* msg) {
	PyErr_SetString(PyExc_RuntimeError, msg);
	}
	}

For example if someone is running a standalone instance of JSBSim, it defaults to a basic level of exceptions, throwing the exceptions where it makes sense to. And then when JSBSim is used with REPLs or attached/embedded/etc with other programs, those users can select their desired level of exception. So that they can turn off JSBSim exceptions completely, and in place off JSBSim exceptions, then a delegate or some other means is called, so the REPLs/other programs can see there is an JSBSim error/exception and halt their simulation gracefully without crashing everything.

Well everything is possible, for that we could replace the keyword throw by a function which would trigger throw depending on some parameter. However the question is: what should we do if the user decide the exception should be ignored ? We cannot throw but we cannot continue the simulation either (otherwise we wouldn't need to throw).

[gallonmate]

Maybe I'm misunderstanding your point but we are already doing that, at least for some methods of the Python classes: C++ exceptions are converted to Python's.

My thoughts were regarding this mentioned in the opening Issue:

REPL Stability: Under no circumstances should JSBSim cause the REPL to crash or terminate unexpectedly. Sudden REPL terminations are extremely frustrating for users, as it can lead to the loss of unsaved work and create a perception of poor software quality.

I'm unfamiliar with how python, REPLs, or other programs are using JSBSim. But I am familiar with how Unreal Engine currently uses JSBSim. My thoughts in my previous comment, was to see if there is some idea that aligns with you (or anyone reading) to universally handle and solve this crash problem when JSBSim is used by various software.

In the case of Unreal Engine, JSBSim exceptions will crash the Unreal Engine editor. This is not ideal and is the same issue regarding REPL termination quoted above.

However the question is: what should we do if the user decide the exception should be ignored ?

I guess that depends on how the various REPLs and applications are using JSBSim, and their ability to handle/respond to the notification of an error. (How many of them experience crashing when JSBSim throws an exception?)

With Unreal Engine, I can think of two ways to deal with the errors when JSBSim exceptions are disabled.

1. Unreal Engine can be notified of a jsbsim error by using a delegate or some other method, and then Unreal Engine ends the JSBSim simulation when that error delegate/method is called.

2. Within JSBSim, instead of throwing an exception, it returns/exits from whichever step it was in FGFDMExec::Run(void). Although I'm not sure there is an easy feasible way to do that, to be able return back to Run() function, when some of the thrown exceptions could be many functions deep into the code at that time. At least not without doing a lot of tedious coding to make it work right.

If a user decides to ignore/disable JSBSim exceptions and has nothing setup in their application to handle the error, then yes that's a new problem. Or if a user runs standalone JSBSim and disables/ignores exceptions, that's also a problem. I wonder if there is some method that makes it more difficult to disable exceptions. Perhaps these changes can be used only in build/make files for JSBSim?

I realize my thoughts are somewhat vague and that I'm not an experienced programmer, so please ignore my thoughts if it doesn't seem applicable outside of Unreal Engine and/or if it's creating unnecessary mental work. These changes are not necessary for me, but I thought I would mention them in case it's useful.

[bcoconni]

I'm unfamiliar with how python, REPLs, or other programs are using JSBSim. But I am familiar with how Unreal Engine currently uses JSBSim. My thoughts in my previous comment, was to see if there is some idea that aligns with you (or anyone reading) to universally handle and solve this crash problem when JSBSim is used by various software.

In the case of Unreal Engine, JSBSim exceptions will crash the Unreal Engine editor. This is not ideal and is the same issue regarding REPL termination quoted above.

I've indeed mainly focused on REPLs but the Unreal Engine plugin is also a plain candidate: JSBSim must not crash the Unreal Engine editor, nor any UE app that uses JSBSim. So yes, it would be much appreciated that the JSBSim/UE community give some feedback about their experience with JSBSim and its coupling with UE.

By the way, the logging system redesign is also targeted at the UE editor: the goal is to have an implementation of the FGLogger class that plays well with the UE editor. So if there are features that are specific to UE and its editor and that are needed to avoid crashes or get better logging in the UE editor, let's talk about them!

1. Unreal Engine can be notified of a jsbsim error by using a delegate or some other method, and then Unreal Engine ends the JSBSim simulation when that error delegate/method is called.

Could you please elaborate about what a delegate is ? I'm not familiar with this concept.

If a user decides to ignore/disable JSBSim exceptions and has nothing setup in their application to handle the error, then yes that's a new problem. Or if a user runs standalone JSBSim and disables/ignores exceptions, that's also a problem. I wonder if there is some method that makes it more difficult to disable exceptions. Perhaps these changes can be used only in build/make files for JSBSim?

That's the reason why I am advocating to remove all exceptions from the simulation loop, although I'm not sure it's feasible or even reasonable. Exceptions that have not been thrown do not need to be caught. That's one step further than being able to disabling them: I'd remove them altogether and return some error codes or similar things instead.

In the case where we might not be able to convince ourselves that an exception can be removed, then we need to add a "global" try/catch handler in FGFDMExec::Run() where, as you suggested, we may decide to rethrow or to discard, and possibly make this decision during the build process. This would prevent exceptions from "escaping" JSBSim for those who don't want or can't handle them but that would not solve the other problem I've mentioned: in the current state of JSBSim code, you would not be able to resume the simulation unless you're prepared to have glitches and weird behaviors.

I realize my thoughts are somewhat vague and that I'm not an experienced programmer, so please ignore my thoughts if it doesn't seem applicable outside of Unreal Engine and/or if it's creating unnecessary mental work. These changes are not necessary for me, but I thought I would mention them in case it's useful.

I'm not a professional programmer either so we're all learning as we're walking. All the feedbacks are welcome and that's really the point of this ticket.