Using "source" in a function breaks job control

Using "source" in a function breaks job control

[Daniel Hahler]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I've noticed that when using "source" (with a file that has at least one expression)
in a function, it will cause job control to not work as expected anymore.

TEST CASE:
1. echo true > /tmp/foo.zsh
2. vi() { source /tmp/foo.zsh; vim -u NONE -N; }
3. Run "vi"
4. In Vim, press Ctrl-Z to put it into the background.
5. Execute "fg".

It should bring back "vim", but does not.


OUTPUT:
% vi
[1]  + 23415 running    

⎯⎯⎯[~] es:148 (SIGSTOP) jobs:1s
% fg
[1]  + 23415 continued  

⎯⎯⎯[~] es:148 (SIGSTOP)
%


The PID 23415 refers to a "zsh" subprocess:

  |   |-zsh,23316
  |   |   |-vim,23414 -u NONE -N
  |   |   `-zsh,23415

Is this behavior expected?
Why is there a new subprocess being created?


Regards,
Daniel.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iD8DBQFVN+RQfAK/hT/mPgARAohaAKDn9uawn58BjsxIhdDTfRG33A7H3QCg+che
6S9jXhz0oMFCzMTr30fY3Ew=
=4nNo
-----END PGP SIGNATURE-----

Re: Using "source" in a function breaks job control

[Peter Stephenson]

On Wed, 22 Apr 2015 20:11:28 +0200
Daniel Hahler <genml+zsh-workers@thequod.de> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> I've noticed that when using "source" (with a file that has at least
> one expression) 
> in a function, it will cause job control to not work as expected anymore.
> 
> TEST CASE:
> 1. echo true > /tmp/foo.zsh
> 2. vi() { source /tmp/foo.zsh; vim -u NONE -N; }
> 3. Run "vi"
> 4. In Vim, press Ctrl-Z to put it into the background.
> 5. Execute "fg".
>
> It should bring back "vim", but does not.

This may take some tracking down, but I suspect the "source" is
doing something to the current shell status that it's then not undoing.

> The PID 23415 refers to a "zsh" subprocess:
> 
>   |   |-zsh,23316
>   |   |   |-vim,23414 -u NONE -N
>   |   |   `-zsh,23415
> 
> Is this behavior expected?
> Why is there a new subprocess being created?

That bit's correct.  What you're suspending is the shell function, to do
which it has to fork a shell.  Try suspending and then foregrounding:

vi() {
   vim
   print "vim exited with status $?"
}

and you'll see that the last line would be executed in the wrong place
or not at all without that.  Unfortunately it seems to get the status
wrong: it's the one from when vim suspended rather than when it exited.

pws

Re: Using "source" in a function breaks job control

[Peter Stephenson]

On Wed, 22 Apr 2015 20:11:28 +0200
Daniel Hahler <genml+zsh-workers@thequod.de> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> I've noticed that when using "source" (with a file that has at least
> one expression) in a function, it will cause job control to not work
> as expected anymore.
> 
> TEST CASE:
> 1. echo true > /tmp/foo.zsh
> 2. vi() { source /tmp/foo.zsh; vim -u NONE -N; }
> 3. Run "vi"
> 4. In Vim, press Ctrl-Z to put it into the background.
> 5. Execute "fg".
> 
> It should bring back "vim", but does not.

I'm not going to get any further with this tonight, but using a build to
instrument process group handling I posted a few weeks ago it seems that
in the failing case the terminal doesn't get reattached to the vim
process when the shell function is brought to the foreground.

I've omitted irrelevant detail, in particular attaching to the main
interactive shell process.

In the successful case,


+10:11% vi() { vim -u NONE -N; } 
+10:11% vi
Attaching TTY 10 to 20919
[1]  + 20922 suspended  vi
(148)+[1]10:11% ps -fp 20922 
UID        PID  PPID  C STIME TTY          TIME CMD
pws      20922 20794  0 22:11 pts/1    00:00:00 ./zsh
+[1]10:12% ps -fp 20919
UID        PID  PPID  C STIME TTY          TIME CMD
pws      20919 20794  0 22:11 pts/1    00:00:00 vim -u NONE -N
+[1]10:12% fg
[1]  + 20922 continued  vi
Attaching TTY 10 to 20919
Attaching TTY 10 to 20922


I suspect but haven't checked that final attachtty is after the vim
exited successfully (which you can't see happening).

In the failing case,


+10:12% vi() { . /tmp/foo.zsh; vim -u NONE -N; }
+10:13% vi
Attaching TTY 10 to 21065
[1]  + 21068 running    
(148)+[1]10:13% ps -fp 21068
UID        PID  PPID  C STIME TTY          TIME CMD
pws      21068 20794  0 22:13 pts/1    00:00:00 ./zsh
+[1]10:13% ps -fp 21065
UID        PID  PPID  C STIME TTY          TIME CMD
pws      21065 20794  0 22:13 pts/1    00:00:00 vim -u NONE -N
+[1]10:13% fg
[1]  + 21068 continued  
Attaching TTY 10 to 21068


There's no attachtty to 21065.  Handling of SIGCONT within the subshell?

pws

Re: Using "source" in a function breaks job control

[Bart Schaefer]

On Apr 22, 10:26pm, Peter Stephenson wrote:
} Subject: Re: Using "source" in a function breaks job control
}
} > 1. echo true > /tmp/foo.zsh
} > 2. vi() { source /tmp/foo.zsh; vim -u NONE -N; }
} > 3. Run "vi"
} > 4. In Vim, press Ctrl-Z to put it into the background.
} > 5. Execute "fg".
} > 
} > It should bring back "vim", but does not.
} 
} I'm not going to get any further with this tonight, but using a build to
} instrument process group handling I posted a few weeks ago it seems that
} in the failing case the terminal doesn't get reattached to the vim
} process when the shell function is brought to the foreground.

It's not the "."/"source" command itself that's the problem, because it
works fine as long as the sourced file does not execute any commands.

vi() { source =(<<<'# Nothing here'); vim -u NONE -N }

suspends/restarts fine.

} In the failing case,
} 
} There's no attachtty to 21065.  Handling of SIGCONT within the subshell?

I don't think so ...  the fact that it prints

zsh: running

when you suspend it, indicates that pn->status == SP_RUNNING when passing
through printjob().  Also (though perhaps not definitive), a TRAPCONT()
handler does not fire in the subshell.

It behaves differently (but still wrong) if the "source" file runs an
external command:

torch% vi() { source =(<<<'/bin/true'); vim -u NONE -N }
torch% vi
zsh: suspended  
torch% fg 
[4]    continued  
torch% 

So there it believes WIFSTOPPED(pn->status) is true, but it still brings
the wrong job into the foreground.  (Actually there's a race condition
here, if I "watch thisjob" with gdb I sometimes get "zsh: running" for
an external command.)

Finally you'll note that it has lost the jobtext in the failing case
above, but in the successful case:

torch% vi() { source =(<<<'# /bin/true'); vim -u NONE -N }
torch% vi
zsh: suspended  vi
torch% 

It has something to do with tracking the job table.

Re: Using "source" in a function breaks job control

[Peter Stephenson]

On Wed, 22 Apr 2015 21:55:39 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> It has something to do with tracking the job table.

Looks like we lost STAT_SUPERJOB in the flags of the job that got
created when we forked.  In the bad case the STAT_ flags are
SUBLEADER|CURSH|INUSE|LOCKED and in the good case there is also
SUPERJOB.  The SUBLEADER in the bad case means that the logic was
probably on roughly the right lines, though that's not a great surprise
since we can see the forked subshell is there.  SUPERJOB can only get
added at one point in execpline(), although it can also be removed but
in that case the job should get WASSUPER which doesn't get removed until
the job is cleared.  So the differences in execpline(), which I never
understood, might be informative.

pws

Re: Using "source" in a function breaks job control

[Bart Schaefer]

On Thu, Apr 23, 2015 at 1:13 PM, Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
> On Wed, 22 Apr 2015 21:55:39 -0700
> Bart Schaefer <schaefer@brasslantern.com> wrote:
>> It has something to do with tracking the job table.
>
> Looks like we lost STAT_SUPERJOB in the flags of the job that got
> created when we forked.

That was the needed clue.  The problem is that list_pipe_job is wrong
following the return from source().  The complications are that (1)
list_pipe_job is static to exec.c whereas source() is in init.c, and
(2) when I tried exporting list_pipe_job so it could be saved/restored
in source(), it fixed this issue but caused the test for "status of
recently exited background jobs is recorded" to fail (which confuses
me, because I can't see any reason source() would get involved in that
test).

Perhaps source() should be using execsave()/execrestore() instead of
the stack of local copies of the job state globals that it maintains?

Re: Using "source" in a function breaks job control

[Peter Stephenson]

On Thu, 23 Apr 2015 23:22:55 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Thu, Apr 23, 2015 at 1:13 PM, Peter Stephenson
> <p.w.stephenson@ntlworld.com> wrote:
> > On Wed, 22 Apr 2015 21:55:39 -0700
> > Bart Schaefer <schaefer@brasslantern.com> wrote:
> >> It has something to do with tracking the job table.
> >
> > Looks like we lost STAT_SUPERJOB in the flags of the job that got
> > created when we forked.
> 
> That was the needed clue.  The problem is that list_pipe_job is wrong
> following the return from source().  The complications are that (1)
> list_pipe_job is static to exec.c whereas source() is in init.c, and
> (2) when I tried exporting list_pipe_job so it could be saved/restored
> in source(), it fixed this issue but caused the test for "status of
> recently exited background jobs is recorded" to fail (which confuses
> me, because I can't see any reason source() would get involved in that
> test).

The variable needs adding to the set in execlist(). I tried a few
similarly named variables last night but not that one.

Apparently a few more verses need adding to the Nibelungenlied.

 *   Allen Edeln gebiet ich Andacht,
 *   Hohen und Niedern von Heimdalls Geschlecht;
 *   Ich will list_pipe's Wirken kuenden
 *   Die aeltesten Sagen, der ich mich entsinne...

> Perhaps source() should be using execsave()/execrestore() instead of
> the stack of local copies of the job state globals that it maintains?

It's possible --- the current split between source() and execlist() is
certainly icky --- though I'm not sure whether or not source() needs to
be as boxed in as eval and similar or if it'll create subtle problems.
(Actually, if it didn't, it would be a new record.)

pws

diff --git a/Src/exec.c b/Src/exec.c
index 2a8185c..60b79c6 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1146,7 +1146,7 @@ execlist(Estate state, int dont_change_job, int exiting)
     Wordcode next;
     wordcode code;
     int ret, cj, csp, ltype;
-    int old_pline_level, old_list_pipe;
+    int old_pline_level, old_list_pipe, old_list_pipe_job;
     zlong oldlineno;
     /*
      * ERREXIT only forces the shell to exit if the last command in a &&
@@ -1159,10 +1159,11 @@ execlist(Estate state, int dont_change_job, int exiting)
     cj = thisjob;
     old_pline_level = pline_level;
     old_list_pipe = list_pipe;
+    old_list_pipe_job = list_pipe_job;
     oldlineno = lineno;
 
     if (sourcelevel && unset(SHINSTDIN))
-	pline_level = list_pipe = 0;
+	pline_level = list_pipe = list_pipe_job = 0;
 
     /* Loop over all sets of comands separated by newline, *
      * semi-colon or ampersand (`sublists').               */
@@ -1397,6 +1398,7 @@ sublist_done:
     }
     pline_level = old_pline_level;
     list_pipe = old_list_pipe;
+    list_pipe_job = old_list_pipe_job;
     lineno = oldlineno;
     if (dont_change_job)
 	thisjob = cj;

Re: Using "source" in a function breaks job control

[Peter Stephenson]

On Fri, 24 Apr 2015 16:25:42 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> Apparently a few more verses need adding to the Nibelungenlied.
> 
>  *   Allen Edeln gebiet ich Andacht,
>  *   Hohen und Niedern von Heimdalls Geschlecht;
>  *   Ich will list_pipe's Wirken kuenden
>  *   Die aeltesten Sagen, der ich mich entsinne...

I beg your pardon, it looks like it's the Elder Edda.

pws

Re: Using "source" in a function breaks job control

[Bart Schaefer]

On Apr 24,  4:25pm, Peter Stephenson wrote:
}
} Apparently a few more verses need adding to the [Elder Edda].

Indeed, now we just have to figure these two out:

torch% vi() { source =(<<<false); vim -u NONE -N; print "vim exited $?" }
torch% vi
zsh: suspended  

Note we don't have the jobtext there.  If there's no "source" then we
get the correct jobtext ("suspended  vi").  There may be one more global
not getting the right treatment in execlist()?  The jobtext as displayed
there comes from the loop over jn->procs in printjob().  Perhaps the
remaining issue is that list_pipe_job isn't restored soon enough.

And then:

torch% fg
[1]  + continued  
vim exited 148
torch% 

As you noted a few messages back, this is the exit status from the signal
stop rather than from the actual exit.

Re: Using "source" in a function breaks job control

[Peter Stephenson]

On Fri, 24 Apr 2015 09:21:30 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Apr 24,  4:25pm, Peter Stephenson wrote:
> }
> } Apparently a few more verses need adding to the [Elder Edda].
> 
> Indeed, now we just have to figure these two out:
> 
> torch% vi() { source =(<<<false); vim -u NONE -N; print "vim exited $?" }
> torch% vi
> zsh: suspended  
> 
> Note we don't have the jobtext there.  If there's no "source" then we
> get the correct jobtext ("suspended  vi").  There may be one more global
> not getting the right treatment in execlist()?  The jobtext as displayed
> there comes from the loop over jn->procs in printjob().  Perhaps the
> remaining issue is that list_pipe_job isn't restored soon enough.

list_pipe_text needs handling.  It's a fxied 80-character array, but I
don't see why we can't just allocate it as needed and free it after.

It would be better to move the save and restore out of execlist(), but I
don't dare do that --- apart from traps, nothing else is currently
saving and restoring list_pipe's Wirken properly.  So this would impact
functions and a lot else.

This doesn't fix the exit status.

pws

diff --git a/Src/exec.c b/Src/exec.c
index 60b79c6..31c80a7 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1147,6 +1147,7 @@ execlist(Estate state, int dont_change_job, int exiting)
     wordcode code;
     int ret, cj, csp, ltype;
     int old_pline_level, old_list_pipe, old_list_pipe_job;
+    char *old_list_pipe_text;
     zlong oldlineno;
     /*
      * ERREXIT only forces the shell to exit if the last command in a &&
@@ -1160,10 +1161,16 @@ execlist(Estate state, int dont_change_job, int exiting)
     old_pline_level = pline_level;
     old_list_pipe = list_pipe;
     old_list_pipe_job = list_pipe_job;
+    if (*list_pipe_text)
+	old_list_pipe_text = ztrdup(list_pipe_text);
+    else
+	old_list_pipe_text = NULL;
     oldlineno = lineno;
 
-    if (sourcelevel && unset(SHINSTDIN))
+    if (sourcelevel && unset(SHINSTDIN)) {
 	pline_level = list_pipe = list_pipe_job = 0;
+	*list_pipe_text = '\0';
+    }
 
     /* Loop over all sets of comands separated by newline, *
      * semi-colon or ampersand (`sublists').               */
@@ -1399,6 +1406,12 @@ sublist_done:
     pline_level = old_pline_level;
     list_pipe = old_list_pipe;
     list_pipe_job = old_list_pipe_job;
+    if (old_list_pipe_text) {
+	strcpy(list_pipe_text, old_list_pipe_text);
+	zsfree(old_list_pipe_text);
+    } else {
+	*list_pipe_text = '\0';
+    }
     lineno = oldlineno;
     if (dont_change_job)
 	thisjob = cj;

Re: Using "source" in a function breaks job control

[Peter Stephenson]

On Fri, 24 Apr 2015 09:21:30 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> torch% fg
> [1]  + continued  
> vim exited 148
> torch% 
> 
> As you noted a few messages back, this is the exit status from the signal
> stop rather than from the actual exit.

This is much more basic --- the source has got nothing to do with it
and any vim inside a function shows the same effect.  I think, in fact,
it's down to simple process logic.

I tried using

vi() { vim -u NONE -N; print "vim exited $?" }

and that prints the wrong status if vim was suspended --- despite the
fact I can see the status of the vim process being updated in addbgstatus().

Then it occurred to me that actually that's inevitable, if I've
understood what's going on.  vim was suspended from the parent shell,
and remains a child of that --- but we've forked at that point, and the
print is going to happen in the forked copy.  That can never see the
status of the exited vim unless there's some complex communication about
process statuses with the parent shell which even the Seeress of the
myth didn't foresee.

Then it occurred to me that because it hasn't got the updated status of
the vim, the subshell will continue for ever more to think it has status
148, so without the "print" it's going to exit with that status when it
gets to the end of the function.  It's that subshell status that the
main shell reports (reasonably enough, you'd have thought) as the
overall status of the job.

Then I got confused and stopped.

We could do some clever stuff with piping status information when we
restart the subshell to update it about the exit status of the thing
that got suspended.  That might not be *that* hairy given that by
definition we have control of both main shell and subshell at the point
where they get forked and presumably (though I don't know where this is)
where the subshell gets retstarted.  It would involve more understanding
of the list_pipe code than I have, though.

A simple alternative might be for the subshell to set the status of the
process that caused it to fork to 0.  But this breaks any logic
depending on the exit status --- at least the 148 tells you something
funny is going on.

pws

Re: Using "source" in a function breaks job control

[Bart Schaefer]

On Apr 28, 11:18am, Peter Stephenson wrote:
} Subject: Re: Using "source" in a function breaks job control
}
} vi() { vim -u NONE -N; print "vim exited $?" }
} 
} and that prints the wrong status if vim was suspended --- despite the
} fact I can see the status of the vim process being updated in addbgstatus().
} 
} Then it occurred to me that actually that's inevitable, if I've
} understood what's going on.  vim was suspended from the parent shell,
} and remains a child of that --- but we've forked at that point, and the
} print is going to happen in the forked copy.  That can never see the
} status of the exited vim unless there's some complex communication about
} process statuses with the parent shell which even the Seeress of the
} myth didn't foresee.

Ok, that makes perfect sense.  For point of contrast, given an equivalent
function, bash stops vim but the function immediately continues without
stopping; ksh 93u-1 (ubuntu) gets really confused:

$ vi() { vim -u NONE -N; echo $?; }
$ vi
[2] + Stopped                  vi
$ jobs
[2] + Stopped                  vi
[1] -  Running                 vi
$ 

Note that $? does not reflect that vim has been suspended.  Both jobs
are actually stopped even though job 1 still says "Running".

$ fg %1    
ksh: fg: no such job
$ fg
vi
0
$ 

Job 2 is the function, job 1 is vim, but there's no way to bring vim
back into the foreground again except:

$ ps x | grep vim
20687 pts/0    T      0:00 vim -u NONE -N
$ kill -CONT 20687; wait
(vim resumes and I can exit from it)
$ 

(Also if I run "vim" directly from the command line and stop it and
"echo $?" I get 276 in ksh93, 148 in bash and zsh.)  

} Then it occurred to me that because it hasn't got the updated status of
} the vim, the subshell will continue for ever more to think it has status
} 148, so without the "print" it's going to exit with that status when it
} gets to the end of the function.  It's that subshell status that the
} main shell reports (reasonably enough, you'd have thought) as the
} overall status of the job.

This also makes sense.

} We could do some clever stuff with piping status information when we
} restart the subshell to update it about the exit status of the thing
} that got suspended.

I don't think that's necessary, and it I suspect it creates weird issues
when things are backgrounded instead of foregrounded.  Documentation is
probably sufficient here, it's (emperically) pretty unusual for the user
of an interactive shell to care about these exit values as long as all
the processes stop and restart in the right order.

} A simple alternative might be for the subshell to set the status of the
} process that caused it to fork to 0.  But this breaks any logic
} depending on the exit status --- at least the 148 tells you something
} funny is going on.

Perhaps there's some way to preserve that status without propagating it
as the exit of the whole subshell?