Odd job-reporting buglet (Re: Change in suspend behavior)

Odd job-reporting buglet (Re: Change in suspend behavior)

[Bart Schaefer]

On May 10,  3:35pm, Bart Schaefer wrote:
}
} 	for i in bar foo; do mycmd $i & done; wait; fg
} 
} The "wait" is just there to be sure that all the jobs have a chance to
} start up (and then stop again on SIGTTOU or SIGTTIN) before the "fg"
} is executed.

Change that loop just a little bit and some really strange things happen:

schaefer[501] for i in bar foo; do cat & wait; done
[2] 27451
[2]  + suspended (tty input)  cat
[1] 27452
[1]  + suspended (tty input)  cat
zsh: suspended (tty input)  cat | 
zsh: running                for i in bar foo; do; cat & wait; done

Eh?  It thinks "cat" has been piped to the for loop!  Also, how did job
number 2 get a smaller process ID than job number 1?

schaefer[502] jobs -l
[1]  + 27452 suspended (tty input)  cat | 
       27453 running                for i in bar foo; do; cat & wait; done
[2]  - 27451 suspended (tty input)  cat

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: Odd job-reporting buglet (Re: Change in suspend behavior)

[Sven Wischnowsky]

Bart Schaefer wrote:

> ...
> 
> Change that loop just a little bit and some really strange things happen:
> 
> schaefer[501] for i in bar foo; do cat & wait; done
> [2] 27451
> [2]  + suspended (tty input)  cat
> [1] 27452
> [1]  + suspended (tty input)  cat
> zsh: suspended (tty input)  cat | 
> zsh: running                for i in bar foo; do; cat & wait; done
> 
> Eh?  It thinks "cat" has been piped to the for loop!  Also, how did job
> number 2 get a smaller process ID than job number 1?
> 
> schaefer[502] jobs -l
> [1]  + 27452 suspended (tty input)  cat | 
>        27453 running                for i in bar foo; do; cat & wait; done
> [2]  - 27451 suspended (tty input)  cat

Aaarrrrggghhh!

The reason is pretty clear (to some): the sub-job returns and is
stopped, so execpline() things it has to do all that super/sub-job
handling it does when the sub-job was suspended with ^Z.

One way to try to fix this is to make execpline() check if the sub-job
really was suspended because of a ^Z (SIGTSTP).  And only then create a
super-job and whatnot.

The other way to try to fix this would be to check if the sub-job was
ASYNC (i.e. it had a `&' at the end), pass that information up to (the
right) execpline() and there decide to not wrap it into a super-job.

The patch below (not committed and certainly not to be committed before
we've discussed this some more) does the first for me (check for SIGTSTP).

I think both solutions have their pros and cons.  Opinions?

Bye
  Sven

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.26
diff -u -r1.26 exec.c
--- Src/exec.c	2001/03/30 16:51:54	1.26
+++ Src/exec.c	2001/05/11 08:47:30
@@ -1064,11 +1064,26 @@
 		    jn->stat & STAT_DONE &&
 		    lastval2 & 0200)
 		    killpg(mypgrp, lastval2 & ~0200);
+#if 0
+                /* This does the wrong thing if the sub-job was stopped
+                 * for some other reason than the user hitting ^Z.  In that
+                 * case we don't want it to put the whole loop (or whatever)
+                 * into a super-job. */
 		if (!list_pipe_child && !lpforked && !subsh && jobbing &&
 		    (list_pipe || last1 || pline_level) &&
 		    ((jn->stat & STAT_STOPPED) ||
 		     (list_pipe_job && pline_level &&
 		      (jobtab[list_pipe_job].stat & STAT_STOPPED)))) {
+#endif
+		if (!list_pipe_child && !lpforked && !subsh && jobbing &&
+		    (list_pipe || last1 || pline_level) &&
+		    (((jn->stat & STAT_STOPPED) &&
+                      WIFSTOPPED(jn->procs->status) &&
+                      WSTOPSIG(jn->procs->status) == SIGTSTP) ||
+		     (list_pipe_job && pline_level &&
+		      (jobtab[list_pipe_job].stat & STAT_STOPPED) &&
+                      WIFSTOPPED(jobtab[list_pipe_job].procs->status) &&
+                      WSTOPSIG(jobtab[list_pipe_job].procs->status) == SIGTSTP))) {
 		    pid_t pid;
 		    int synch[2];
 

-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: Odd job-reporting buglet (Re: Change in suspend behavior)

[Bart Schaefer]

On May 11, 10:47am, Sven Wischnowsky wrote:
}
} Bart Schaefer wrote:
} 
} > schaefer[501] for i in bar foo; do cat & wait; done
} > [2] 27451
} > [2]  + suspended (tty input)  cat
} > [1] 27452
} > [1]  + suspended (tty input)  cat
} > zsh: suspended (tty input)  cat | 
} > zsh: running                for i in bar foo; do; cat & wait; done
} 
} The reason is pretty clear (to some): the sub-job returns and is
} stopped, so execpline() things it has to do all that super/sub-job
} handling it does when the sub-job was suspended with ^Z.

This happens only because of the `wait' command, right?  It does *not*
happen when I do `for i in bar foo; do; cat & sleep 5; done'.
 
} One way to try to fix this is to make execpline() check if the sub-job
} really was suspended because of a ^Z (SIGTSTP).

One can send TSTP with a `kill'.  Testing for TSTP has to be combined with
a test for whether the job is the foreground job at the time.

} The other way to try to fix this would be to check if the sub-job was
} ASYNC (i.e. it had a `&' at the end), pass that information up to (the
} right) execpline() and there decide to not wrap it into a super-job.

That sounds closer to the right thing to me, but wouldn't a test for "is
the foreground job" be equivalent (and possibly not need any more info
to be passed around)?

Or maybe it would be possible to know that we're in a `wait' command and
simply not do any of this processing?  But that would be equivalent to
"is this the foreground job" as well (as the `wait' would be the fore-
ground job in that case, not the job that got the signal).

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: Odd job-reporting buglet (Re: Change in suspend behavior)

[Sven Wischnowsky]

We were both on the wrong track.  The code once handled that correctly
but then came the change in 6819 that made

  foo | while true; do bar; done

suspendable (with a quickly finishing bar).  The change made execpline()
test the state of `foo', by adding:

		     (list_pipe_job && pline_level &&
		      (jobtab[list_pipe_job].stat & STAT_STOPPED)))) {

But in cases like the one we're discussing now, the list_pipe_job was
set when starting the loop and ended up pointing to a job in the loop.

So the patch below tries to fix this at a completely different place --
setting up list_pipe_job only if we are building a job for a real pipe
into a loop or something (not for a loop not inside a pipe).

Seems to work for me, without breaking what I said above, so I'm going
to commit it.

Bye
  Sven

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.26
diff -u -r1.26 exec.c
--- Src/exec.c	2001/03/30 16:51:54	1.26
+++ Src/exec.c	2001/05/14 08:49:23
@@ -973,10 +973,10 @@
      * stopped, the top-level execpline() didn't get the pid for the
      * sub-shell because it was overwritten. */
     if (!pline_level++) {
-	list_pipe_job = newjob;
         list_pipe_pid = 0;
 	nowait = 0;
 	simple_pline = (WC_PIPE_TYPE(code) == WC_PIPE_END);
+	list_pipe_job = (simple_pline ? 0 : newjob);
     }
     lastwj = lpforked = 0;
     execpline2(state, code, how, opipe[0], ipipe[1], last1);

-- 
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de