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

When called from another shell script, fish can't read #3805

Closed
nrnrnr opened this issue Feb 3, 2017 · 16 comments
Closed

When called from another shell script, fish can't read #3805

nrnrnr opened this issue Feb 3, 2017 · 16 comments
Labels
Milestone

Comments

@nrnrnr
Copy link

@nrnrnr nrnrnr commented Feb 3, 2017

  • Have you checked if problem occurs with fish 2.4.0?
  • Tried fish without third-party customizations (check sh -c 'env HOME=$(mktemp -d) fish')?

fish version installed (fish --version): fish, version 2.3.1

OS/terminal used: Debian Linux (jessie), xterm

I wrote a script using #!/usr/bin/fish. The script calls read. Works fine from command line but not when called from another script.

Reproduction steps

nr@homedog ~/net> call-fish-read 
fish: Couldn't grab control of terminal
tcsetpgrp: Interrupted system call

nr@homedog ~/net> fish-read
say something⏎                                                                  asdf

asdf
nr@homedog ~/net> /usr/bin/fish --version
fish, version 2.3.1-dirty

Scripts are attached in a zip file, but it may be easier to view source here.

Here is fish-read:

#!/usr/bin/fish

read -p 'echo -n say something >&2 ' -l -s answer
echo $answer

and here is call-fish-read:

#!/bin/ksh

mumble=$(fish-read)
echo $mumble

N.B. I downloaded a nightly of fish 2.4.0 from SUSE, and behavior is even more frustrating---the shell just hangs trying to read file descriptor 3. Not sure what's up with that (I poked at it with strace -f.)

Would love to have a workaround that enables me to use a fish script as a tool in other scripts.
demo-scripts.zip

May be related to #3223 and #1258.

@krader1961 krader1961 added the bug label Feb 3, 2017
@krader1961 krader1961 added this to the fish-future milestone Feb 3, 2017
@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Feb 3, 2017

This is definitely broken. So far I've only gotten as far as finding that it is blocked inside the call to reader_push() from read_interactive(). I'm very surprised no one has noticed this before.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Feb 3, 2017

I dug a little deeper. Fish is hanging on the tcsetpgrp() and tcsetattr() calls in reader_interactive_init(). If I just comment those out it gets much farther and reports

<W> fish: Current terminal parameters have rows and/or columns set to zero.
<W> fish: The stty command can be used to correct this (e.g., stty rows 80 columns 24).

It then hangs and you still can't type anything and press [enter] to have it read and echoed. But you can press [ctrl-C] and it will terminate the read.

Basically, fish's handling of the controlling tty, process groups, etc. has a number of quirks. Issue #1258 is definitely relevant. See also #2980.

@faho
Copy link
Member

@faho faho commented Mar 31, 2017

So, here's what I found out:

  • When fish starts, it tries to get into its own process group (setpgid in reader_interactive_init)

  • After this, it tries to get this group control of the terminal (tcsetpgrp)

  • That fails if there is still a foreground group that fish is not a member of, by getting a SIGTTOU, which will (unless otherwise handled) stop the process (i.e. fish)

  • At least in my specific case, the old group continues to stay in the foreground, so if fish hadn't switched there wouldn't be an (apparent) problem

  • Process groups are removed when no process in them remains, so there should be something left in the old one, presumably the shell that started fish - this also explains why it works with fish, since that gives the processes it starts their own foreground group

So I see two things we could do here:

  • Don't switch process groups - I can't see a reason for it and it seems to fix at least this case, maybe every one of these

  • Only perform the tcsetpgrp if there is no (other) foreground group (in that case tcgetpgrp returns "some value larger than 1 that is not presently a process group ID") - in this case we'd have to stop doing stuff we can only do if we're in control

@krader1961: Any opinion?

@zanchey
Copy link
Member

@zanchey zanchey commented Jun 19, 2017

Interestingly the code to start a new process group and grab control of the terminal was removed from fish_pager in 264c065 back in 2006.

Otherwise this behaviour dates back to before the source was under revision control.

I can't see a strong reason that fish tries to start its own process group; the launching process should do that.

There's definitely some confusion in the code; once again shell_pgid is set to a PID, not a PGID. Perhaps the setpgid line was included to fix failing tcsetpgrp calls when handed a PID rather than a PGID??

I suggest we remove the process group switching and trial it in master for a time.

@faho
Copy link
Member

@faho faho commented Jun 20, 2017

I suggest we remove the process group switching and trial it in master for a time.

Yes, please!

@faho faho marked this as a duplicate of #1258 Jul 24, 2017
@krader1961 krader1961 marked this as a duplicate of #4238 Jul 24, 2017
@krader1961 krader1961 marked this as a duplicate of #4261 Jul 28, 2017
@jlsjonas
Copy link

@jlsjonas jlsjonas commented Aug 14, 2017

Has there been any progress on this? it's quite an annoying behaviour when (what's presumably is causing the issue in #4261 ) fish is set as your default shell

@mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Aug 20, 2017

I just ran into a related issue with the tcsetpgrp and tcsetattr in reader_interactive_shell under WSL. This one is a real head scratcher: microsoft/WSL#1653

The same code worked before and after recompiling, it doesn't now.

I've commented out both the tcsetpgrp and tcsetattr calls locally, and would like to trial this in master as discussed above. From my previous work with job control on fish' end, all the child process has to do is try to read or write from/to the terminal and the kernel will take care of the rest, either aborting the process with SIGTTIN or receiving SIGTTOU and hanging until the parent process gives it control of the terminal.

If this takes care of BashOnWindows#1653 and #3805, maybe we can give it a try?

I'll check other shells to see if they have similar code or if they just rely on the parent process to do the right thing.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Aug 21, 2017
While the idiomatic fix to fish' myriad of job control issues would be
to parse all functions prior to beginning the job pipeline so that
everything in the command line can be executed in the context of a
single job, that would require a huge effort to rewrite the core job
flow in fish and does not make sense at this time.

Instead, this patch fixes fish-shell#3952 and fish-shell#206 (but notably not fish-shell#4238) by
having jobs that are part of a single command pipeline, including those
that are functions executing external commands, use the same process
group. This prevents a (parent|child) from crashing with SIGTTIN or
hanging at SIGTTOU because it has a different process group than the
process currently in control of the terminal.

Additionally, since this fix involves removing the code that forces fish
to run in its own process group (which IMHO never made sense, as job
control is the job of the shell, not the process being launched), it
also fixes fish-shell#3805 and works around BashOnWindows#1653.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Sep 8, 2017
While the idiomatic fix to fish' myriad of job control issues would be
to parse all functions prior to beginning the job pipeline so that
everything in the command line can be executed in the context of a
single job, that would require a huge effort to rewrite the core job
flow in fish and does not make sense at this time.

Instead, this patch fixes fish-shell#3952 and fish-shell#206 (but notably not fish-shell#4238) by
having jobs that are part of a single command pipeline, including those
that are functions executing external commands, use the same process
group. This prevents a (parent|child) from crashing with SIGTTIN or
hanging at SIGTTOU because it has a different process group than the
process currently in control of the terminal.

Additionally, since this fix involves removing the code that forces fish
to run in its own process group (which IMHO never made sense, as job
control is the job of the shell, not the process being launched), it
also fixes fish-shell#3805 and works around BashOnWindows#1653.
@mqudsi mqudsi closed this in 55b3c45 Sep 10, 2017
@faho faho modified the milestones: fish-future, fish-3.0 Jan 11, 2018
@IsaacOscar

This comment was marked as off-topic.

@mqudsi

This comment was marked as off-topic.

@IsaacOscar

This comment was marked as off-topic.

@mqudsi

This comment was marked as off-topic.

@IsaacOscar

This comment was marked as off-topic.

@mqudsi

This comment was marked as off-topic.

@faho

This comment was marked as off-topic.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 2, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should elimante the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 8, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should elimante the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 8, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should elimante the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 8, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 8, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 9, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 10, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 10, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 11, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 13, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 15, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 18, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 19, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 20, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 22, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 24, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 27, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 27, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
@mqudsi mqudsi closed this in #5219 Oct 27, 2018
mqudsi added a commit that referenced this issue Oct 27, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes #4178. Fixes #3805. Fixes #5210.
mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 31, 2018
If fish detects that it was started with a pgrp of 0 (which appears to
oddly be the case when run under firejail), create new process group for
fish and give it control of the terminal.

This selectively reverts 55b3c45 in cases where an invalid pgrp is
detected. Note that this is known to cause problems in other cases, such
as fish-shell#3805 and microsoft/WSL#1653, although the former may have been
ameliorated or even addressed by the recent job control overhaul, so
that's why we are careful to only assign fish to its own pgroup if an
invalid pgroup was detected and not as the normal case.
faho added a commit that referenced this issue Nov 13, 2018
If fish detects that it was started with a pgrp of 0 (which appears to
oddly be the case when run under firejail), create new process group for
fish and give it control of the terminal.

This selectively reverts 55b3c45 in cases where an invalid pgrp is
detected. Note that this is known to cause problems in other cases, such
as #3805 and microsoft/WSL#1653, although the former may have been
ameliorated or even addressed by the recent job control overhaul, so
that's why we are careful to only assign fish to its own pgroup if an
invalid pgroup was detected and not as the normal case.
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Nov 24, 2018
* Instead of reaping all child processes when we receive a SIGCHLD, try
reaping only processes belonging to process groups from fully-
constructed jobs, which should eliminate the need for the keepalive
process entirely (WSL's lack of zombies not withstanding) as now
completed processes are not reaped until the job has been fully
constructed (i.e.  all processes launched), which means their process
group should still be around for new processes to join.

* When `tcgetpgrp()` calls return 0, attempt to `tcsetpgrp()` before
invoking failure handling code.

* When forking a builtin and not running interactively, do not bail if
unable to set/restore terminal attributes.

Fixes fish-shell#4178. Fixes fish-shell#3805. Fixes fish-shell#5210.
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Nov 24, 2018
If fish detects that it was started with a pgrp of 0 (which appears to
oddly be the case when run under firejail), create new process group for
fish and give it control of the terminal.

This selectively reverts 55b3c45 in cases where an invalid pgrp is
detected. Note that this is known to cause problems in other cases, such
as fish-shell#3805 and microsoft/WSL#1653, although the former may have been
ameliorated or even addressed by the recent job control overhaul, so
that's why we are careful to only assign fish to its own pgroup if an
invalid pgroup was detected and not as the normal case.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants