Uh oh!
There was an error while loading. Please reload this page.
Stop quit[] from ending the host process - #181
Open
HugoFara wants to merge 1 commit into
Open
Conversation
quit is in the script command table, so it is reachable from the Python binding and from the HTTP server, and it called exit(0). A host got no exception and no traceback, its finally blocks and atexit handlers never ran, buffered output was lost, and the status was 0 — so a batch job recorded success having stopped early. quit now releases the session and records the request; quitRequested() exposes it and clearQuitRequest() forgets it. The shell in app/forefire is what leaves: both the interactive loop and the HTTP listen loop check it, so a served quit[] still stops the server. executeLoop stops reading a script after one, rather than the process disappearing mid-file. currentSession.params is no longer deleted. It is the SimulationParameters singleton, owned by GetInstance() and shared with every other holder, so deleting it left GetInstance() handing out a dangling pointer. That went unnoticed only because exit() followed on the next line; without the exit it would be a use-after-free on the next parameter read. The safe topology error path called quit() too, so an internal failure terminated the host on its own, with status 0. It reports and returns error instead. The message moves to stderr and is no longer gated on commandOutputs: a returned error nobody is obliged to check should not also be silent. tests/unit/test_quit_command.cpp covers it, and the CTest entries now require doctest's closing 'Status: SUCCESS!' line. Without that a suite whose process exits mid-run reports as a pass, because ctest sees 0 and does not look for the summary that never printed — the first version of this test passed happily with exit(0) still in place. Verified with the reproduction from #160: before quit[] / AFTER quit[] / FINALLY ran / ATEXIT ran, exit 0 against the previous output, which was 'before quit[]' and a dead interpreter. Unit suite 5/5; runff KML and NetCDF match. Closes#160
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#160.
quitsits in the script command table, so it is reachable from the Python binding and from the HTTP server, and it calledexit(0).Running the reproduction from the issue, before and after:
All four failures in one line of code: no exception to catch,
finallyandatexitskipped, buffered output lost — the left column does not even showbefore quit[]without an explicitflush=True, becauseexit()flushes C streams and not Python's io layer — and a status of 0, so a batch job records success having stopped early.What changed
quitreleases the session and records the request.Command::quitRequested()exposes it,Command::clearQuitRequest()forgets it. A shell stops when it sees it; an embedder is free to ignore it and carry on, which is the difference between asking and terminating.The shell in
app/forefireis what leaves now. Both the interactive loop and the HTTP listen loop check, so aquit[]served over HTTP still stops the server — that would otherwise have been functionality removed rather than fixed.executeLoopstops reading a script after one instead of the process vanishing mid-file.Two things found while fixing it
currentSession.paramswas being deleted, and it is the singleton.quitdiddelete currentSession.params, which isSimulationParameters::GetInstance()— shared with every other holder and owned by the function-local static. It went unnoticed only becauseexit()came on the next line. Without the exit it is a use-after-free on the next parameter read, so removing the exit without removing this delete would have traded one bug for a worse one. It is no longer deleted.The first version of my test passed with the bug still in place. A suite whose process calls
exit(0)mid-run reports as a pass: ctest sees status 0, and the doctest summary that never printed is not something it checks for. The CTest entries now require doctest's closingStatus: SUCCESS!line, which turns an early exit into a failure. Negative control, withexit(0)restored:That property is worth having for every suite, not just this one, so it is applied to all five.
The safe topology path — @filippi, this is the part for you
Command.cpp:1125calledquit()from acatch (...)in the atmospheric step loop, under// TODO supersafe mode ?. So an internal failure ended the host process on its own, with status 0.It now reports and returns
error. This changes behaviour in coupled runs: where a safe-topology failure previously terminated, it now returns an error and the caller continues. That is the correct shape for a library, but it is your path and the call is yours — if a hard stop needs to stay reachable there, say so and I will make it a distinct command the binding does not expose, as #160 suggests.The message also moves to
stderrand is no longer gated oncommandOutputs.ExecuteCommanddiscards command return values today, so a returned error that is also silent would be strictly worse than what it replaces.Making
ExecuteCommandpropagate status is the obvious follow-up and is not attempted here — it changes a signature used by the CLI,CLibForeFire,EventCommandand the binding.Verification
exit(0)failsunit.quit_command.runff: KML and NetCDF both match the references within tolerance.This pull request, including its code changes and this description, was generated by Claude Opus 5, and reviewed manually before submitting.