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

$last_pid oddities in test context #5832

Closed
mqudsi opened this issue Apr 20, 2019 · 1 comment
Closed

$last_pid oddities in test context #5832

mqudsi opened this issue Apr 20, 2019 · 1 comment
Labels
bug Something that's not working as intended
Milestone

Comments

@mqudsi
Copy link
Contributor

mqudsi commented Apr 20, 2019

I'm trying to add an interactive test for the following behavior:

function disown_jobs
    sleep 1&
    set -l sleep_pid $last_pid
    jobs -c
    disown $sleep_pid
    jobs -c
end

disown_jobs

but it behaves differently when executed by the test driver, as best as I can tell due to an incorrect last_pid.

With this change to the tests:

diff --git a/tests/jobs.in b/tests/jobs.in
index 7be84be4b..adc947222 100644
--- a/tests/jobs.in
+++ b/tests/jobs.in
@@ -21,3 +21,18 @@ function foo
     jobs -c
 end
 foo
+
+killall -9 sleep
+
+# Verify `jobs` both works in a function and does *not* contain disowned jobs
+function disown_jobs
+    sleep 1&
+    set -l sleep_pid $last_pid
+    echo \$last_pid: $sleep_pid
+    echo pidof sleep: (pidof sleep)
+    jobs -c
+    disown $sleep_pid
+    jobs -c
+end
+
+disown_jobs

This is the output:

--- jobs.out    2019-04-19 19:26:51.102060200 -0500
+++ jobs.tmp.ou

t        2019-04-19 19:27:06.819177300 -0500
@@ -8,3 +8,9 @@
 sleep
 Command
 sleep
+$last_pid: 2261
+pidof sleep: 7554
+Command
+sleep
+Command
+sleep
Error output differs for file jobs.in. Diff follows:
--- jobs.err    2019-04-19 19:26:51.102060200 -0500
+++ jobs.tmp.err        2019-04-19 19:27:06.819177300 -0500
@@ -2,3 +2,4 @@
 fg: No suitable job: 3
 bg: Could not find job '3'
 disown: 'foo' is not a valid job specifier
+disown: Could not find job '2261'

But when executed in build/fish, the output is as follows:

mqudsi@ZBOOK ~/r/fish-shell> # Verify `jobs` both works in a function and does *not* contain disowned jobs
mqudsi@ZBOOK ~/r/fish-shell> function disown_jobs
                                     sleep 1&
                                     set -l sleep_pid $last_pid
                                     echo \$last_pid: $sleep_pid
                                     echo pidof sleep: (pidof sleep)
                                     jobs -c
                                     disown $sleep_pid
                                     jobs -c
                             end
mqudsi@ZBOOK ~/r/fish-shell>
mqudsi@ZBOOK ~/r/fish-shell> disown_jobs
$last_pid: 10504
pidof sleep: 10504
Command
sleep
jobs: There are no jobs

There is #5036, but that shouldn't be the case here as the interactive tests should have full job control (as compared to the invocation tests) and anyway, adding status job-control interactive to jobs.in does not change anything.


I added ps -p $sleep_pid to the test:

PID TTY          TIME CMD
18450 tty1     00:00:00 make 
@faho
Copy link
Member

faho commented May 5, 2019

The output of just jobs instead of jobs -c would be more helpful here.

@faho faho added the bug Something that's not working as intended label May 5, 2019
@faho faho added this to the fish-future milestone May 5, 2019
@faho faho closed this as completed in 3255999 Mar 26, 2021
@zanchey zanchey modified the milestones: fish-future, fish 3.3.0 Mar 27, 2021
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue May 22, 2021
When a job is placed in the background, fish will set the `$last_pid`
variable. Prior to this change, `$last_pid` was set to the process group
leader of the job. However this caussed problems when the job ran in
fish's process group, because then fish itself would be the process group
leader and commands like `wait` would not work.

Switch `$last_pid` to be the actual last pid of the pipeline. This brings
it in line with the `$!` variable from zsh and bash.

This is technically a breaking change, but it is unlikely to cause
problems, because `$last_pid` was already rather broken.

Fixes fish-shell#5036
Fixes fish-shell#5832
Fixes fish-shell#7721
ridiculousfish added a commit that referenced this issue May 25, 2021
When a job is placed in the background, fish will set the `$last_pid`
variable. Prior to this change, `$last_pid` was set to the process group
leader of the job. However this caussed problems when the job ran in
fish's process group, because then fish itself would be the process group
leader and commands like `wait` would not work.

Switch `$last_pid` to be the actual last pid of the pipeline. This brings
it in line with the `$!` variable from zsh and bash.

This is technically a breaking change, but it is unlikely to cause
problems, because `$last_pid` was already rather broken.

Fixes #5036
Fixes #5832
Fixes #7721
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2021
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

No branches or pull requests

3 participants