Retropie-setup: CTRL+C during scraping may trigger the exit of the main script later on

[cmitu]

(Started from https://github.com/RetroPie/RetroPie-Setup/pull/2520#issuecomment-433611505).

I've noticed that using Ctrl+C while scraping, either using scraper or skyscraper, has the side effect of exiting the RetroPie-Setup script after choosing any subsequent option that produces any output. The script exits with exit code 141 (SIGPIPE), seemingly because of the tee command that pipes the output to the RetroPie-Setup log.

Steps to reproduce (using scraper, since it's fewer clicks):

1. Open the RetroPie-Setup script, then open the scraper module (Configuration/Tools).
2. Start a scraping session via Scrape all system or Scrape chosen system.
3. Interrupt the scraping with Ctrl+C. The script correctly shows that _Scraping was aborted_ and returns to the scraper selection GUI.
4. Choose Upgrade scraper to the last version (last option in the GUI). The RetroPie-Setup script exits. ($?=141)

The problem doesn't appear when running the scraper gui with sudo ./retropie_packages.sh scraper gui.

The problem seems to be in
https://github.com/RetroPie/RetroPie-Setup/blob/2b7e724dbe8d2a24a34a7dcf10870d2dcf4dc669/scriptmodules/admin/setup.sh#L373-L384
tee catches somehow the SIGINT and stops, then any output from the rp_callModule "$choice" gui call will trigger a SIGPIPE in the gzip call and ends the script.

A possible fix would be to replace the tee call with

{
# call anything here
} | (tee -i >(gzip --stdout >"$logfilename"))

which doesn't trigger the script to exit, but still mangles the next gui command that has any output (after which GUI options are working fine)

Do you think this warrants more investigation/time or should be something that belongs to the scriptmodule to handle ?

[joolswills]

I will look into it. Thanks.

[hhromic]

Hi guys, I got entertained by this interesting puzzle, and I think I found out the problem.
It turns out that everything in the same cgroup receives the SIGINT and bails out accordingly. In this case, @cmitu correctly identified the piece of code and the processes involved are the userfunction, tee and gzip. @cmitu also correctly hinted that tee can be given the -i option for ignoring interrupt signals, however gzip is also involved and the one still aborting and mangling the log output. However gzip doesn't have such an option for ignoring signals, fortunately we can use the setsid command that launches a process in a new session (cgroup) and therefore gzip will not abort on the original SIGINT.

I made an isolated shell script that reproduces the original issue and shows the proposed solution:

#!/usr/bin/env bash

function userfunc() {  # this is just any user function with a trap, like scraper
  trap "trap INT; return" INT
  dd if=/dev/zero of=/dev/null bs=1M count=100000
  trap INT
}

function orig_broken() {  # this is the current code prone to fail on SIGINT
  {
    userfunc
    echo "userfunc retval: $?"
  } &> >(tee >(gzip --stdout >output.log.gz))
}

function fix_proposal() {  # this is the proposed solution using "tee -i" and "setsid"
  {
    userfunc
    echo "userfunc retval: $?"
  } &> >(tee -i >(setsid gzip --stdout >output.log.gz))
}

# if we CTRL+C during each execution, we randomly will exit the entire script or
# continue with no output (tee/gzip died too), the output.log.gz file will be most
# of the times empty because gzip/tee will abort without flushing
for i in $(seq 10); do
  echo "Starting $i"
  orig_broken
  echo "retval: $?"
done

# with the fix proposal, no matter how much we CTRL+C, the userfunc will
# abort gracefully and gzip and tee will cleanly finish-up the output.log.gz file
for i in $(seq 10); do
  echo "Starting $i"
  fix_proposal
  echo "retval: $?"
done

I hope this shed some light :)

[cmitu]

@hhromic Thanks for digging into this. I wonder in this case - wouldn't it be easier to use setsid on the child processes - and have a new cgroup - to catch SIGINT there, without propagating it downstream (tee or gzip).

[hhromic]

That is a tempting idea, and I also tried it. However this won't work because setsid doesn't make the process the "leader", i.e. goes to background and won't receive CTRL+C keystrokes. You can try it in my example by adding setsid before dd and trying the orig_broken function. There is an option for running the command in the foreground (setsid -c, grab the controlling terminal) however this won't work inside a bash script ("Operation not permitted") because bash is already the process leader. From the manpage of the syscall (https://linux.die.net/man/2/setsid):

EPERM  The  process group ID of any process equals the PID of the call-
       ing process.  Thus, in particular, setsid() fails if the calling
       process is already a process group leader.

When you use setsid with gzip, you don't need gzip to be a process leader because it is just compressing the output of tee. Also, by fixing it at the RetroPie-level, you ensure any scriptmodule is protected without modifying them. I noticed there are many usages of tee >(gzip --stdout >"$logfilename") around the admin/setup.sh script, probably all of them should be modified to use tee -i and setsid gzip.

Lastly, did you try this solution in your particular failing scenario? I hope I reproduced the problem in the small snippet of bash script, but I would like to know if it actually solves your particular bug. Cheers!

Edit: the setsid command is in the util-linux package, I don't have a RetroPie image with me here, so not sure it is installed by default. If not, and this solution is accepted, probably util-linux needs to be added to the RetroPie depends list. Just checked, yes it is contained in the image :).

---

If you want to learn more, there is a nice explanation about leadership of processes in cgroups here: https://blog.nelhage.com/2011/02/changing-ctty/. However, I wouldn't recommend to use this util for this context, just to understand the problem.

[cmitu]

Lastly, did you try this solution in your particular failing scenario? I hope I reproduced the problem in the small snippet of bash script, but I would like to know if it actually solves your particular bug. Cheers!

I tested the fix and it seems to work fine - interrupting the scraping session with Ctrl+C doesn't seem to have any other side effect now. I also had a test case built for this and it seems to fix that too.

[hhromic]

Sounds good! Lets wait then for @joolswills comments for further proceeding with a proper fix

[joolswills]

Thanks. Looks good but will have a test also :)

[joolswills]

Would this work ? Wouldn't this then run them both in a new cgroup ? If so I prefer it as it's a tiny bit shorter :)

function fix_proposal() {  # this is the proposed solution using "tee -i" and "setsid"
  {
    userfunc
    echo "userfunc retval: $?"
  } &> >(setsid tee >(gzip --stdout >output.log.gz))
}

[cmitu]

It doesn't seem to work for the original test report (using scraper), gzip gets the same SIGPIPE. I think the sub-shell created for gzip still runs in the same cgroup as the main script.

[hhromic]

I also thought about making it as elegant as possible like that, but unfortunately that won't work because gzip is started by the shell, not tee, and will receive the CTRL+C and mangle the output.
In that case, setsid launches tee in the new cgroup but the shell still launches gzip in its own cgroup for the redirection. Another variation that will work is to use setsid both for tee and gzip:

>(setsid tee >(setsid gzip --stdout >output.log.gz))

Which is arguably not less short than the original fix proposal, but at least doesn't blind tee of receiving signals. I thought actually about it and I think this behaviour would be more robust.

[hhromic]

@cmitu is correct, as I just explained almost at the same time :)

[joolswills]

Thanks. I'd like to try a couple more things also - will get back to you :-)

[joolswills]

Apologies for delays on this. Will try and find some time in the next few days. I will probably end up using your solution :-)

[hhromic]

Just for the record, I think the definitive version in my opinion should be this one:

>(setsid tee >(setsid gzip --stdout >"$logfilename"))

And it should be used in all blocks that execute interruptable functions in scriptmodules/admin/setup.sh, which are actually all of the ones using tee and gzip.
Namely, the following lines:

$ grep -n 'gzip --stdout' scriptmodules/admin/setup.sh   # grep 'tee' gives the same
131:    } &> >(tee >(gzip --stdout >"$logfilename"))
196:                } &> >(tee >(gzip --stdout >"$logfilename"))
207:                } &> >(tee >(gzip --stdout >"$logfilename"))
216:                } &> >(tee >(gzip --stdout >"$logfilename"))
228:                } &> >(tee >(gzip --stdout >"$logfilename"))
303:                } &> >(tee >(gzip --stdout >"$logfilename"))
316:                } &> >(tee >(gzip --stdout >"$logfilename"))
331:                } &> >(tee >(gzip --stdout >"$logfilename"))
384:        } &> >(tee >(gzip --stdout >"$logfilename"))   # identified by mitu
423:    } &> >(tee >(gzip --stdout >"$logfilename"))
546:                } &> >(tee >(gzip --stdout >"$logfilename"))
572:                } &> >(tee >(gzip --stdout >"$logfilename"))

[cmitu]

@hhromic I think the default behavior in setup spawned functions is to not trap Ctrl+C (INT) and just interrupt the script. The only scriptmodules that have a trap for INT are just 4 - https://github.com/RetroPie/RetroPie-Setup/search?q=trap&unscoped_q=trap. For scraper and skyscraper, they're only invoked from the gui action and the other 2 are not available through the GUI AFAIK.

[hhromic]

Mmm good point. I was advocating more on robustness (the main script won't be interrupted if any function is interrupted), but if the default behaviour is to be interrupted and stopped by design, then yes I agree that in that case only the GUI-based invocations should be protected.

[hhromic]

Hi again, I thought more about what @cmitu mentioned.
Using setsid in tee/gzip as proposed won't change the default behaviour for the functions that are not trapped. In these cases, interrupting the called processes will still interrupt the entire RP script as expected. However, without the fix, gzip will get interrupted as well and the output log ends garbled.
On the other hand, using setsid as proposed makes tee/gzip finish gracefully always, properly writing the log and still interrupting the entire script if not trapped.

That being said, I strongly recommend that setsid is used with tee/gzip in all the locations where logs are being created/captured as pointed in my other comment (https://github.com/RetroPie/RetroPie-Setup/issues/2524#issuecomment-439867981) for robustness.

Edit: I did test what I'm explaining in this comment :) The test is to remove the trap lines from the test snippet, you will see the difference and the need to still use setsid everywhere tee/gzip is used.

[joolswills]

I'm happy if anyone wants to improve the commit message - comments welcome

https://github.com/RetroPie/RetroPie-Setup/pull/2545

[joolswills]

This is now fixed.