Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

^C kills the background processes launched at config.fish #6828

Closed
acomagu opened this issue Mar 29, 2020 · 7 comments · Fixed by #6861
Closed

^C kills the background processes launched at config.fish #6828

acomagu opened this issue Mar 29, 2020 · 7 comments · Fixed by #6861
Labels
bug Something that's not working as intended
Milestone

Comments

@acomagu
Copy link

acomagu commented Mar 29, 2020

Ctrl-C seems to kill the background processes lunched at config.fish.

(sh -c 'env -i TERM=$TERM HOME=$(mktemp -d) fish')
~> ps
    PID TTY          TIME CMD
 227597 pts/9    00:00:00 fish
 227605 pts/9    00:00:04 python3
 227670 pts/9    00:00:00 ps
~> echo 'sleep INF &' >~/.config/fish/config.fish
~> fish
Welcome to fish, the friendly interactive shell
Type `help` for instructions on how to use fish
~> ps
    PID TTY          TIME CMD
 227597 pts/9    00:00:00 fish
 227605 pts/9    00:00:16 python3
 227879 pts/9    00:00:00 fish
 227881 pts/9    00:00:00 sleep
 227896 pts/9    00:00:00 ps
~>     # <- Hit Ctrl-C
~> ps
    PID TTY          TIME CMD
 227597 pts/9    00:00:00 fish
 227605 pts/9    00:00:16 python3 <defunct>   # python3 is exited and defunct.
 227879 pts/9    00:00:00 fish
 227937 pts/9    00:00:00 ps
# And no sleep command.

Ctrl-C key seems to kill the background jobs(sleep & python3).

I think this is bug, right?

Env:

~> fish --version
fish, version 3.1.0-436-g911465a8e
~> uname -a
Linux acm-manjaro 4.19.112-1-MANJARO #1 SMP Fri Mar 20 20:32:56 UTC 2020 x86_64 GNU/Linux
@acomagu acomagu changed the title C-c exits the background processes launched at config.fish ^C exits the background processes launched at config.fish Mar 29, 2020
@acomagu acomagu changed the title ^C exits the background processes launched at config.fish ^C kills the background processes launched at config.fish Mar 29, 2020
@krobelus krobelus added the bug Something that's not working as intended label Mar 30, 2020
@krobelus krobelus added this to the fish 3.2.0 milestone Mar 30, 2020
krobelus added a commit to krobelus/fish-shell that referenced this issue Mar 30, 2020
@krobelus
Copy link
Member

krobelus commented Mar 30, 2020

This happens because fish puts the sleep in fish own process group, so it has no job control.
Another sleep started interactively does have its own process group.

$ sleep 9999 &
$ ps To pid,pgid,comm,args
    PID    PGID COMMAND         COMMAND
1395931 1395931 fish            ./fish
1395933 1395931 sleep           sleep INF
1395961 1395961 sleep           sleep 9999

I have a fix in 52b56cc that simply always puts &-suffixed jobs in their own process group.
Should be fine as is but I'll do some more research.

@zanchey
Copy link
Member

zanchey commented Mar 31, 2020

I'm not sure that's the right fix - perhaps job control needs to be enabled when running config.fish.

@krobelus
Copy link
Member

Yeah, on a second look it seems like the wrong fix because it subverts status job-control; a user could explicitly disable job control, in that case we don't want to enable it for background jobs.

The default is to enable job control only for interactive jobs, and not for jobs that are run as part of the prompt, or from config.fish. So the current behavior is consistent.

What are the use cases for starting background jobs (and not disowning them)?

@faho
Copy link
Member

faho commented Mar 31, 2020

status job-control; a user could explicitly disable job control

I'm not sure status job-control is actually useful. We don't use it anywhere anymore (except for tests), and when we did it was for awful things like eval as a fish function.

And it's not a sensible user knob - if you turned off background jobs, a script you started couldn't use background jobs anymore. That's the sort of thing that we don't support, just like we don't have a choice between nullglob or failglob or which function syntaxes you can use.

It should probably be removed.

@krobelus
Copy link
Member

krobelus commented Apr 3, 2020

A new behavior for interactive fish could be:

  1. Set job control for all jobs.
  2. Source config.fish.
  3. Set job control only for interactive jobs.
  4. Read interactive commands.

Which suggests that the user should not toggle job control in their config, because it is stomped in 3.
So a removal of job control toggles wouldn't hurt here.

This way, fish will give processes launched from config a new process group and make them claim the terminal. However, bash only grants a new process group to background jobs and not other jobs in a .bashrc. I'm not yet sure what's the reason for that.

@ridiculousfish
Copy link
Member

fish is behaving as designed here: the default behavior is that only interactively-run jobs get their own pgroup.

bash behaves the same: if you put sleep 10000 & into .bashrc, the sleep ends up in bash's pgroup. Control-C does not kill sleep, but this is because of the signal mask, not the pgroup:

When job control is not in effect, asynchronous commands ignore SIGINT and SIGQUIT in addition to these inherited handlers.

I don't know what the justification is for this behavior - perhaps it's for just this case! But masking off INT and QUIT would be straightforward to add to fish.

@krobelus please do not always give background jobs their own pgroup, this would make fish diverge further from other shells in a place that's already confusing. I would rather that we don't change pgroup behavior at all until we have real concurrent execution.

status job-control is indeed confusing; however I am not sure what to replace it with.

@krobelus
Copy link
Member

krobelus commented Apr 5, 2020

@ridiculousfish thanks, I had completely missed that bash keeps those background jobs in its own process group. Turns out the signal masking is part of POSIX, see this stackoverflow answer.
I have added that in #6861.

krobelus added a commit to krobelus/fish-shell that referenced this issue Apr 7, 2020
krobelus added a commit to krobelus/fish-shell that referenced this issue Apr 7, 2020
krobelus added a commit to krobelus/fish-shell that referenced this issue Apr 7, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something that's not working as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants