Bug: on-quit is potentially executed infinite times if in synchronous block

[thatDudo]

When using a $ as a prefix on on-quit and killing the parent terminal window with SIGTERM the lf process receives a SIGHUP I think and this leads to app.quit being called in the signal handler and then a second time in the error handling of runCmdSync. Im not exactly sure how the bug comes to be but the lf process stays open in the background in any case and hogs cpu with some infinite loop.

To reproduce simply include this in your config file and close the terminal window without pressing q for quit. The bug seems a bit unreliable to reproduce but adding a sleep gives enough time for something to happen, whatever it is.

cmd on-quit ${{
    sleep 1
    notify-send aaa
}}

The solution might be to only allow on-quit to use a shell async & prefix.

Related: @kmarius You're right! I've never wrote Go and it turned out that I put the snippet inside a error handler by mistake. Now it does work. However, to some reason, on-quit command gets executed 2 times instead of 1 (which might be undesirable in some cases). Is it because app.quit() is called when the server terminates as well?

Originally posted by @NikitaIvanovV in https://github.com/gokcehan/lf/issues/847#issuecomment-1158226731

[joelim-work]

I can reproduce the issue too, looks like it comes from here: https://github.com/gokcehan/lf/blob/f0dddccb04f9e2f99146a6832ee7798a51cd6256/app.go#L510-L511

This comes from several design choices, which when combined together end up triggering each other to form an infinite loop:

1. When the program is about to quit, on-quit is called to allow users to insert custom behavior. This can be seen as a feature request - additionally there is the decision of whether to call on-quit when terminating gracefully and/or ungracefully.
2. on-quit runs as a synchronous shell command, because of the way the user configured it. Actually on-quit can be any type of expr, for example a listExpr which in turn contains other exprs including synchronous shell commands..
3. When running a synchronous shell command, if the UI cannot be resumed (e.g. the terminal is closed) then the program will terminate. This sounds reasonable, but it also triggers the first step, thereby completing the loop.

So I'm not really sure what can be done about this - one of the above has to be removed to break the loop and thus prevent this kind of issue from happening. Your suggestion of restricting on-quit to asynchronous shell commands is a possibility, but it just adds more complexity to the problem, and it isn't clear to users why there would be such a restriction. I was also considering just reverting #1681, but then I don't know how many users actually depend on on-quit running during ungraceful termination.

I suppose this is kind of a variation of the problem of allowing users to configure custom error-handling, where such handlers can trigger the original error and cause an infinite loop.

[joelim-work]

So after thinking about this some more, I think restricting on-quit to asynchronous shell commands is not a good idea as there might be users whose cleanup is dependent on running in a synchronous context. I'm not sure what those use cases might be, but it feels like too risky of a decision to make.

Reverting #1681 so that on-quit only runs when terminating gracefully (e.g. pressing q) is more preferable IMO - users should not be quitting lf by closing the terminal window, or do anything else deliberately that results in ungraceful termination, and it's reasonable to expect that on-quit and other cleanup won't run in such case.

Yet another approach would be to add a variable that tracks if lf is already trying to quit, which can be used avoid triggering on-quit again and break the loop. I like this solution less though, as it feels like a band-aid solution which acknowledges that running on-quit during ungraceful termination can result in infinite loops, and thus has a flawed design.

[thatDudo]

the UI cannot be resumed (e.g. the terminal is closed)

I see. Then the EOF error in app.ui.resume might be the stdout closing.

About not running on-quit on ungraceful termination, I would prefer if we didnt do that since I use that to close and delete my ueberzugpp process and files and I want to have the ability to quit with alt f4 for example if Im running a synchronous process from lf and its too much of a bother to close the process to return to lf and then quit with q. I dont think adding a quitting variable is that bad of a solution to prevent the app.quit function to be executed twice on principle. Or as an alternative to somehow ensure the entire process halts after app.quit finishes, which it doesnt if app.quit can call app.quit again. I dont know it seems kind of complicated to do something else like remove the error handling in all functions that are called from app.quit or will be called from there in the future.