diff --git a/app/forefire/ForeFire.cpp b/app/forefire/ForeFire.cpp index d9c72bf..5a36a03 100644 --- a/app/forefire/ForeFire.cpp +++ b/app/forefire/ForeFire.cpp @@ -85,12 +85,14 @@ int ForeFire::startShell(int argc, char* argv[]) { string listenCommand = "listenHTTP[]"; executor.ExecuteCommand(listenCommand); - // Keep the main thread alive indefinitely + // Keep the main thread alive while the server runs. `quit[]` used to + // end the process from inside the library; now it asks, and this is + // the loop that answers, so a served quit[] still stops the server. cout << "(Press Ctrl+C to exit)" << endl; - while (true) { - sleep(3600); + while (!Command::quitRequested()) { + sleep(1); } - + return 0; } else { @@ -160,6 +162,10 @@ void ForeFire::FFShell(ifstream* inputStream) { if (line.empty()) continue; executor.ExecuteCommand(line); + // `quit[]` no longer ends the process from inside the library, so + // the shell is what leaves on request. + if (Command::quitRequested()) + break; } } } diff --git a/src/Command.cpp b/src/Command.cpp index d250916..49d1092 100644 --- a/src/Command.cpp +++ b/src/Command.cpp @@ -29,6 +29,7 @@ namespace libforefire double Command::endTime = 0; bool Command::firstCommand = true; + bool Command::quitAsked = false; size_t Command::refTabs = 0; FFPoint *Command::lastReadLoc = 0; @@ -1116,13 +1117,14 @@ namespace libforefire } catch (...) { - if (getDomain()->commandOutputs) - { - cout << getDomain()->getDomainID() << ": " - << "**** ERROR IN SAFE TOPOLOGY MODE, QUITING ****" << endl; - } - // TODO supersafe mode ? - quit(arg, numTabs); + // Reports an error rather than ending the process. This + // used to call quit(), so an internal failure here took + // the host down with exit status 0 — a coupled run or a + // batch job recorded success while having stopped early. + // The caller now sees the failure and chooses. + cerr << getDomain()->getDomainID() << ": " + << "**** ERROR IN SAFE TOPOLOGY MODE ****" << endl; + return error; } } } @@ -2423,6 +2425,11 @@ namespace libforefire while (std::getline(*inputStream, line)) { + // A `quit[]` earlier in the script stops the file here rather than + // ending the process, so the caller still gets to return normally. + if (quitAsked) + break; + if (!inTripleQuote) { /* Skip comments and empty lines when *not* inside a multiline literal */ @@ -2959,15 +2966,40 @@ namespace libforefire int Command::quit(const string &arg, size_t &numTabs) { - + // No exit() here. `quit` is in the command table, so it is reachable + // from the Python binding and from the HTTP server, and a library + // that ends its host's process skips every destructor, `finally` and + // atexit handler on the way out — and used to do it with status 0, so + // a batch run reported success having done nothing. The session is + // released and the request is recorded; whoever is driving the shell + // decides what to do about it. delete currentSession.fd; + currentSession.fd = 0; delete currentSession.outStrRep; + currentSession.outStrRep = 0; delete currentSession.sim; - delete currentSession.params; - exit(0); + currentSession.sim = 0; + + // currentSession.params is deliberately not deleted: it is the + // SimulationParameters singleton, owned by GetInstance() and shared + // with every other holder. Deleting it left GetInstance() handing out + // a dangling pointer, which only went unnoticed because exit() + // followed on the next line. + + quitAsked = true; return normal; } + bool Command::quitRequested() + { + return quitAsked; + } + + void Command::clearQuitRequest() + { + quitAsked = false; + } + void Command::setOstringstream(ostringstream *oss) { currentSession.outStream = oss; diff --git a/src/Command.h b/src/Command.h index bd0f8b3..3c9af95 100644 --- a/src/Command.h +++ b/src/Command.h @@ -121,6 +121,8 @@ class Command { }; static bool firstCommand; + /*! \brief set by quit(), read by quitRequested() */ + static bool quitAsked; static size_t refTabs; static FFPoint* lastReadLoc; @@ -176,7 +178,14 @@ class Command { static int systemExec(const string&, size_t&); /*! \brief command to run a system trough pipe */ static int clear(const string&, size_t&); - /*! \brief command to quit the ForeFire shell */ + /*! \brief releases the session and asks the host to stop + * + * Does NOT terminate the process. This is library code, and the host + * process is not ours to end: a `quit[]` arriving over the Python binding + * or the HTTP server used to take the interpreter down with it, skipping + * every destructor and `finally`, and exiting 0 so a batch job reported + * success. Callers driving a shell should check quitRequested() and stop. + */ static int quit(const string&, size_t&); /*! \brief command to quit the ForeFire shell */ static int listenHTTP(const string&, size_t &) ; @@ -232,6 +241,16 @@ class Command { public: + /*! \brief whether a `quit[]` has asked the host to stop + * + * Set by quit() and cleared by clearQuitRequest(). A shell reading + * commands should stop its loop when this is true; a library embedder is + * free to ignore it and carry on, which is the difference between asking + * and terminating. */ + static bool quitRequested(); + /*! \brief forgets a pending quit request */ + static void clearQuitRequest(); + // Reference time static double refTime; diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 18ecabd..c734d50 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -12,7 +12,8 @@ add_executable(forefire_unit_tests test_flux_models.cpp test_http_server.cpp test_model_registry.cpp - test_propagation_models.cpp) + test_propagation_models.cpp + test_quit_command.cpp) target_link_libraries(forefire_unit_tests PRIVATE forefireL) target_include_directories(forefire_unit_tests PRIVATE @@ -25,8 +26,15 @@ endif() # doctest groups its cases into suites; registering one CTest entry per suite # keeps `ctest` output useful without needing doctest's CMake integration. -foreach(_suite "model registry" "propagation models" "flux models" "http server") +foreach(_suite "model registry" "propagation models" "flux models" "http server" + "quit command") string(REPLACE " " "_" _suite_id "${_suite}") add_test(NAME "unit.${_suite_id}" COMMAND forefire_unit_tests --test-suite=${_suite} --no-skipped-summary) + # Requiring doctest's closing line, not just a zero exit status. A test + # that ends the process mid-run — which is exactly what quit[] used to do, + # with exit(0) — otherwise reports as a pass, because ctest sees 0 and the + # summary that never printed is not something it looks for. + set_tests_properties("unit.${_suite_id}" PROPERTIES + PASS_REGULAR_EXPRESSION "\\[doctest\\] Status: SUCCESS!") endforeach() diff --git a/tests/unit/test_quit_command.cpp b/tests/unit/test_quit_command.cpp new file mode 100644 index 0000000..47216a3 --- /dev/null +++ b/tests/unit/test_quit_command.cpp @@ -0,0 +1,75 @@ +/** + * @file test_quit_command.cpp + * @brief `quit[]` must not end the process it is running inside. + * @copyright Copyright (C) 2025 ForeFire, Fire Team, SPE, CNRS/Universita di Corsica. + * @license This program is free software; See LICENSE file for details. (See LICENSE file). + * + * `quit` sits in the script command table, so it is reachable from the Python + * binding and from the HTTP server, and it used to call exit(0). A host got no + * traceback, no exception, no `finally`, no destructors, and a status of 0 — + * a batch job recorded success having stopped early. See #160. + * + * The test for that is the whole file running to completion: if `quit[]` still + * called exit(0), the cases below this one would not report, and ctest would + * see a suite that passed nothing rather than a suite that failed. The + * `--test-suite` entry in CMakeLists is what turns that into a visible result. + */ + +#include "doctest/doctest.h" + +#include "Command.h" + +#include + +using libforefire::Command; + +TEST_SUITE("quit command") { + +TEST_CASE("quit[] returns instead of ending the process") { + Command executor; + Command::clearQuitRequest(); + + std::string command = "quit[]"; + executor.ExecuteCommand(command); + + // Reaching this line at all is the assertion that matters. + CHECK(Command::quitRequested()); +} + +TEST_CASE("the quit request can be cleared and the session reused") { + // A host that decides to ignore the request has to be able to carry on, + // which is the difference between asking and terminating. + Command executor; + Command::clearQuitRequest(); + REQUIRE_FALSE(Command::quitRequested()); + + std::string quitCommand = "quit[]"; + executor.ExecuteCommand(quitCommand); + REQUIRE(Command::quitRequested()); + + Command::clearQuitRequest(); + CHECK_FALSE(Command::quitRequested()); + + // The interpreter still works after a quit: setParameter and getParameter + // round-trip, which they could not do if quit had freed the parameters + // singleton as it used to. + std::string setCommand = "setParameter[ff_test.after.quit=42]"; + executor.ExecuteCommand(setCommand); + CHECK(libforefire::SimulationParameters::GetInstance() + ->getParameter("ff_test.after.quit") == "42"); +} + +TEST_CASE("a second quit is harmless") { + // quit() deletes the session objects and nulls them; running it twice must + // not double free what the first call released. + Command executor; + Command::clearQuitRequest(); + + std::string command = "quit[]"; + executor.ExecuteCommand(command); + executor.ExecuteCommand(command); + + CHECK(Command::quitRequested()); +} + +} /* TEST_SUITE */