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

Job handling is broken. #3952

Closed
layus opened this Issue Apr 12, 2017 · 10 comments

Comments

Projects
None yet
7 participants
@layus
Copy link

layus commented Apr 12, 2017

I have an issue running the command sudo echo lol | grep lol. This command is not as trivial as it seems because grep is a fish function, calling command grep --color=auto $argv behind the scene.

Now, a simple run of that command gives

layus@klatch ~/p/fish> sudo echo lol | grep lol
# Nothing happens, job is blocked. Entering ^Z.
Job 1, 'sudo echo lol | grep lol' has stopped
layus@klatch ~/p/fish> jobs
Job	Group	CPU	State	Command
1	7318	0%	stopped	sudo echo lol | grep lol
2	7319	0%	stopped	command grep --color=auto $argv
layus@klatch ~/p/fish> fg
Send job 1, 'sudo echo lol | grep lol' to foreground
[sudo] password for layus : 
layus@klatch ~/p/fish> fg
Send job 2, 'command grep --color=auto $argv' to foreground
lol
layus@klatch ~/p/fish> 

This behavior is not changed by #3922, and may relate to #206. We see that fish starts two jobs for one user job, and that these jobs are managed separately. Digging in the source code, we can find that one job is created for the top-level pipeline.

exec_job starts processes for each step of the pipeline. When it reaches grep, it detects a fish function and calls internal_exec_helper to start its body.
That function parses the function body and starts a new job to to handle its content.
That new job is not aware of its parent context, and start with a new pgid. Fish gives that process control over the terminal, removing it from sudo.

When sudo starts, it receives a SIGTTOU and gets stopped. Fish blocks because the grep job is still running, and unrelated to the sudo job.

Running ps to compare fish to other shells gives

 PPID   PID  PGID   SID TTY      TPGID STAT   UID   TIME COMMAND
===== ===== ===== ===== ======== ===== ===== ==== ====== =========
    1   764   764   764 ?           -1 Ss       0   0:00 sshd [...]
  764 31954 31954 31954 ?           -1 Ss       0   0:00  \_ sshd: me [priv]
31954 31956 31954 31954 ?           -1 S     1000   0:00  |   \_ sshd: me@pts/0
31956 31957 31957 31957 pts/0     7319 Ss    1000   0:00  |       \_ -zsh
31957  7306  7306 31957 pts/0     7319 S     1000   0:00  |           \_ ./fish -d4
 7306  7317  7306 31957 pts/0     7319 S     1000   0:00  |               \_ ./fish -d4
 7306  7318  7318 31957 pts/0     7319 T        0   0:00  |               \_ sudo echo lol
 7306  7319  7319 31957 pts/0     7319 S+    1000   0:00  |               \_ grep --color=auto lol
  764  7390  7390  7390 ?           -1 Ss       0   0:00  \_ sshd: me [priv]
 7390  7392  7390  7390 ?           -1 S     1000   0:00  |   \_ sshd: me@pts/4
 7392  7393  7393  7393 pts/4     7470 Ss    1000   0:00  |       \_ -zsh
 7393  7460  7460  7393 pts/4     7470 S     1000   0:00  |           \_ zsh
 7460  7470  7470  7393 pts/4     7470 S+       0   0:00  |               \_ sudo echo lol
 7460  7471  7470  7393 pts/4     7470 S+    1000   0:00  |               \_ grep --color=auto lol
  764  7474  7474  7474 ?           -1 Ss       0   0:00  \_ sshd: me [priv]
 7474  7476  7474  7474 ?           -1 S     1000   0:00      \_ sshd: me@pts/5
 7476  7477  7477  7477 pts/5     7530 Ss    1000   0:00          \_ -zsh
 7477  7519  7519  7477 pts/5     7530 S     1000   0:00              \_ bash
 7519  7530  7530  7477 pts/5      7530 S+       0   0:00                  \_ sudo echo lol
 7519  7531  7530  7477 pts/5      7530 S+    1000   0:00                  \_ grep lol

We can see that the current TPGID is 7319 while the sudo command has a PGID of 7318. It's status is T, which means that the process is "stopped, either by a job control signal or because it is being traced". Here it is because of the aforementioned SIGTTOU.
We see that other shells give the same PGID to all the subcommands.

The extra fish process is the "keepalive fork" used by fish. I still wonder why fish needs it while other shells do not, but it is not important.

From a job control point of view, fish shows two running jobs in jobs output, while bash and zsh will show only one, even if zsh seems to distinguish between the processes of the job. (^Z was added for clarity, but does not appear in the terminal)

ZSH:
--(layus@klatch)--> grep () { command grep --color=auto "$@"; }   
--(layus@klatch)-->  sudo echo lol | grep lol                      
[sudo] password for layus: ^Z
zsh: running    sudo echo lol | grep --color=auto lol
--(layus@klatch)--> jobs
[1]  + suspended           sudo echo lol | 
       suspended (signal)  grep --color=auto lol

BASH:
layus@klatch:~$ grep () { command grep --color=auto "$@"; }
layus@klatch:~$ sudo echo lol | grep lol
[sudo] password for layus: ^Z

[1]+  Stopped                 sudo echo lol | grep lol
layus@klatch:~$ jobs
[1]+  Stopped                 sudo echo lol | grep lol

We need to make function invocation part of the same job as their context. I believe that one command line should create one job in the jobs view. That would also immediately fix the SIGTTOU issue for sudo because both processes will have the same PGID and will both have control over the terminal.
Because they are handled exctly like function, begin/end blocks would be fixed at the same time.


On current master (version 2.5.0-259-gc0de8afa-dirty, aka c0de8af).

@krader1961 krader1961 added the bug label Apr 13, 2017

@krader1961 krader1961 added this to the fish-future milestone Apr 13, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Apr 13, 2017

Job control in fish is definitely idiosyncratic. I can't be bothered to decide if this is a duplicate of one of the already open issues on this topic.

Your description of the problem, @layus, is fantastic. Any chance we can entice you to take a stab at fixing some of the job control issues even if not this specific one? For that matter, your approach to such matters suggests to me that you could contribute meaningful improvements to fish in practically any area if you are comfortable making changes to C/C++ code.

@layus

This comment has been minimized.

Copy link

layus commented Apr 13, 2017

Thanks @krader1961 :-).

I can't be bothered to decide if this is a duplicate of one of the already open issues on this topic.

Me neither, that's why I opened a new Issue.

What bothers me is that the parsing and execution are tighly coupled. I would rather refactor that code to separate parsing and execution. What I hoped to find in fish is a cleaner shell implementation than zsh. I am a bit disappointed to find this very complex code in exec_job.

I will probably take time to improve on this issue, but I secretly hoped that sharing my discovery would trigger someone else to do it. Fish is not my top priority, and I already spent way too much time on these issues. that being said, leaving things as-is will probably itch me too much ;-).

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Apr 13, 2017

I would rather refactor that code to separate parsing and execution.

Yes, please. I made a huge number of lint cleanups a year ago including some refactoring. But I mostly left the job management code untouched because, frankly, many of the functions are so large and have so many deeply nested blocks they're borderline incomprehensible. Refactoring that code before making functional changes is a very good idea. In fact, I consider it borderline mandatory. If you make lint-all you'll see what I mean.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jul 25, 2017

Fixes a race condition in output redirection in job chain
I'm not sure if this happens on all platforms, but under WSL with the
existing codebase, processes in the job chain that pipe their
stdout/stderr to the next process in the job could terminate before the
next job started (on fast enough machines for quick enough jobs).

This caused issues like fish-shell#4235 and possibly fish-shell#3952, at least for external
commands. What was happening is that the first process was finishing
before the second process was fully set up. fish would then try to
assign (in both the child and the parent) the process group id belonging
to the process group leader to the new process; and if the first process
had already terminated, it would have ended its process group with it as
well before that happened.

I'm not sure if there was already a mechanism in place for ensuring that
a process remains running at least as long as it takes for the next
process in the chain to join its group, etc., but if that code was
there, it wasn't working in my test setup (WSL).

This patch definitely needs some review; I'm not sure how I should
handle non-external commands (and external commands executed via
posix_spawn). I don't know if they are affected by the race condition in
the first place, but when I tried to add the same "wait for next command
in chain to run before unblocking" that would cause black screens
requiring ctrl+c to bypass.

The "unblock previous command" code was originally run by the next child
to be forked, but was then moved to the shell code instead, making it
more-centrally located and less error-prone.

Note that additional headers may be required for the mmap system call on
other platforms.

krader1961 added a commit that referenced this issue Aug 6, 2017

Fixes a race condition in output redirection in job chain
I'm not sure if this happens on all platforms, but under WSL with the
existing codebase, processes in the job chain that pipe their
stdout/stderr to the next process in the job could terminate before the
next job started (on fast enough machines for quick enough jobs).

This caused issues like #4235 and possibly #3952, at least for external
commands. What was happening is that the first process was finishing
before the second process was fully set up. fish would then try to
assign (in both the child and the parent) the process group id belonging
to the process group leader to the new process; and if the first process
had already terminated, it would have ended its process group with it as
well before that happened.

I'm not sure if there was already a mechanism in place for ensuring that
a process remains running at least as long as it takes for the next
process in the chain to join its group, etc., but if that code was
there, it wasn't working in my test setup (WSL).

This patch definitely needs some review; I'm not sure how I should
handle non-external commands (and external commands executed via
posix_spawn). I don't know if they are affected by the race condition in
the first place, but when I tried to add the same "wait for next command
in chain to run before unblocking" that would cause black screens
requiring ctrl+c to bypass.

The "unblock previous command" code was originally run by the next child
to be forked, but was then moved to the shell code instead, making it
more-centrally located and less error-prone.

Note that additional headers may be required for the mmap system call on
other platforms.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Aug 21, 2017

Fixes job handling involving functions and terminal control
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.
@ThomasAH

This comment has been minimized.

Copy link

ThomasAH commented Sep 6, 2017

I guess this is the same issue:
ssh foo.example.com ps auxww | grep something
does not print anything and seems to hang when it wants the passphrase of an ssh key or the password of the target server.

After Ctrl-C I get a new prompt, but nothing else is printed.
When I now try to exit this shell, I get a warning, that a job is running in the background, which is above command.

Workaround for me is to load the ssh key into ssh-agent, which I usually do anyway, but I have more than one key and some are loaded with an automatic timeout.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Sep 8, 2017

Fixes job handling involving functions and terminal control
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.
@danieljabailey

This comment has been minimized.

Copy link

danieljabailey commented Apr 18, 2018

I think I am seeing this issue too.

I tried fish 2.7.1 and still have issues with things like this:

yes foo | grep foo | sed s/foo/foo/

which should just start printing "foo" repeatedly. Instead it hangs until I interrupt with Ctrl-c, then starts printing foo, until I interrupt again.

I have also found with similar pipelines that it hangs until I interrupt with Ctrl-c and then lots of buffered output appears, but the whole command terminates with a single Ctrl-c

@paneq

This comment has been minimized.

Copy link

paneq commented Jul 18, 2018

I have the same issue. Pipe such as:

rake routes | grep collapse

hangs. When I go with Ctrl-c I see:

Job 2, 'rake routes | grep collapse' has stopped
@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Oct 4, 2018

@faho I think with #5219 this is actually partially addressed. It's not a timing issue that was allowing sudo to work for me:

image

It was working so long as sudo did not attempt to read/write from/to the terminal, which it will avoid doing if it doesn't need to prompt for a password (e.g. NOPASSWD is set in visudo for this user/group or the password is cached by an agent for the session). (On master it seems to be broken even in that case.)

@faho

This comment has been minimized.

Copy link
Member

faho commented Oct 4, 2018

It was working so long as sudo did not attempt to read/write from/to the terminal, which it will avoid doing if it doesn't need to prompt for a password

@mqudsi: That's how it always was? The reason this sudo is ever stopped is because it gets SIGTTIN, which of course it won't get if it doesn't attempt to access the terminal.

There at least was a time where it was possible that sudos pgroup got the terminal instead of greps (which was the timing issue I was referring to and assumed was your problem), but it still shouldn't hang because of this if it doesn't access the terminal in the first place?

On master it seems to be broken even in that case.

I can confirm that does not happen for me. If the credentials are cached, sudo echo lol | grep lol works. If they aren't (e.g. after sudo -K), it doesn't.

screenshot_20181004_201647

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Oct 4, 2018

That's not the error I get under master, and the first thing I do on any of my machines is add NOPASSWD to visudo (so no terminal access):

mqudsi@ZBook /m/c/U/m/r/fish-shell> sudo echo lol | grep lol
Usage: grep [OPTION]... PATTERN [FILE]...
Try 'grep --help' for more information.

It's as if it's executed without any parameters rather than without any input.

@faho

This comment has been minimized.

Copy link
Member

faho commented Oct 4, 2018

It's as if it's executed without any parameters rather than without any input.

Uh, yeah. That's what I get when I forget the search string - sudo echo lol | grep. Even if we missed the redirection, grep lol should still read from the terminal.

So there's something very weird (I kinda can't bring myself to say "fishy" here) going on there!

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 8, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 9, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 10, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 10, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 10, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 10, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 10, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 11, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 13, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 13, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 13, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 15, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 18, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 18, 2018

Prevent early reap of proc in parent job
When a parent job spawns new jobs due to the evaluation of a new
function (which shouldn't be the case in the first place), we end up
with two distinct jobs sharing one pgrp (to fix fish-shell#3952). This can lead to
early termination of a pgrp if finished parent job children are reaped
before future processes in either the parent or future child jobs can
join it.

While the parent job is under construction, require that waitpid(2)
calls for the child job be done by process id and not job pgrp.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 19, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 19, 2018

Prevent early reap of proc in parent job
When a parent job spawns new jobs due to the evaluation of a new
function (which shouldn't be the case in the first place), we end up
with two distinct jobs sharing one pgrp (to fix fish-shell#3952). This can lead to
early termination of a pgrp if finished parent job children are reaped
before future processes in either the parent or future child jobs can
join it.

While the parent job is under construction, require that waitpid(2)
calls for the child job be done by process id and not job pgrp.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 19, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 19, 2018

Prevent early reap of proc in parent job
When a parent job spawns new jobs due to the evaluation of a new
function (which shouldn't be the case in the first place), we end up
with two distinct jobs sharing one pgrp (to fix fish-shell#3952). This can lead to
early termination of a pgrp if finished parent job children are reaped
before future processes in either the parent or future child jobs can
join it.

While the parent job is under construction, require that waitpid(2)
calls for the child job be done by process id and not job pgrp.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 20, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

When a parent job spawns new jobs due to the evaluation of a new
function (which shouldn't be the case in the first place), we end up
with two distinct jobs sharing one pgrp (to fix fish-shell#3952). This can lead to
early termination of a pgrp if finished parent job children are reaped
before future processes in either the parent or future child jobs can
join it.

While the parent job is under construction, require that waitpid(2)
calls for the child job be done by process id and not job pgrp.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 20, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

When a parent job spawns new jobs due to the evaluation of a new
function (which shouldn't be the case in the first place), we end up
with two distinct jobs sharing one pgrp (to fix fish-shell#3952). This can lead to
early termination of a pgrp if finished parent job children are reaped
before future processes in either the parent or future child jobs can
join it.

While the parent job is under construction, require that waitpid(2)
calls for the child job be done by process id and not job pgrp.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 22, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

When a parent job spawns new jobs due to the evaluation of a new
function (which shouldn't be the case in the first place), we end up
with two distinct jobs sharing one pgrp (to fix fish-shell#3952). This can lead to
early termination of a pgrp if finished parent job children are reaped
before future processes in either the parent or future child jobs can
join it.

While the parent job is under construction, require that waitpid(2)
calls for the child job be done by process id and not job pgrp.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 24, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

When a parent job spawns new jobs due to the evaluation of a new
function (which shouldn't be the case in the first place), we end up
with two distinct jobs sharing one pgrp (to fix fish-shell#3952). This can lead to
early termination of a pgrp if finished parent job children are reaped
before future processes in either the parent or future child jobs can
join it.

While the parent job is under construction, require that waitpid(2)
calls for the child job be done by process id and not job pgrp.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 27, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

When a parent job spawns new jobs due to the evaluation of a new
function (which shouldn't be the case in the first place), we end up
with two distinct jobs sharing one pgrp (to fix fish-shell#3952). This can lead to
early termination of a pgrp if finished parent job children are reaped
before future processes in either the parent or future child jobs can
join it.

While the parent job is under construction, require that waitpid(2)
calls for the child job be done by process id and not job pgrp.

Closes fish-shell#3952.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Oct 27, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

When a parent job spawns new jobs due to the evaluation of a new
function (which shouldn't be the case in the first place), we end up
with two distinct jobs sharing one pgrp (to fix fish-shell#3952). This can lead to
early termination of a pgrp if finished parent job children are reaped
before future processes in either the parent or future child jobs can
join it.

While the parent job is under construction, require that waitpid(2)
calls for the child job be done by process id and not job pgrp.

Closes fish-shell#3952.

@mqudsi mqudsi modified the milestones: fish-future, fish-3.0 Oct 27, 2018

@mqudsi mqudsi closed this in #5219 Oct 27, 2018

mqudsi added a commit that referenced this issue Oct 27, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

When a parent job spawns new jobs due to the evaluation of a new
function (which shouldn't be the case in the first place), we end up
with two distinct jobs sharing one pgrp (to fix #3952). This can lead to
early termination of a pgrp if finished parent job children are reaped
before future processes in either the parent or future child jobs can
join it.

While the parent job is under construction, require that waitpid(2)
calls for the child job be done by process id and not job pgrp.

Closes #3952.

ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Nov 24, 2018

Associate external commands in functions with extant pgrps
When a function is encountered by exec_job, a new context is created for
its execution from the ground up, with a new job and all, ultimately
resulting in a recursive call to exec_job from the same (main) thread.

Since each time exec_job encounters a new job with external commands
that needs terminal control it creates a new pgrp and gives it control
of the terminal (tcsetpgrp & co), this effectively takes control away
from the previously spawned external commands which may be (and likely
are) expecting to still have terminal access.

This commit attempts to detect when such a situation arises by handling
recursive calls to exec_job (which can only happen if the pipeline
included a function) by borrowing the pgrp from the (necessarily still
active) parent job and spawning new external commands into it.

When a parent job spawns new jobs due to the evaluation of a new
function (which shouldn't be the case in the first place), we end up
with two distinct jobs sharing one pgrp (to fix fish-shell#3952). This can lead to
early termination of a pgrp if finished parent job children are reaped
before future processes in either the parent or future child jobs can
join it.

While the parent job is under construction, require that waitpid(2)
calls for the child job be done by process id and not job pgrp.

Closes fish-shell#3952.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment