Re: PATCH (?): Re: suspend while loop.

Re: PATCH (?): Re: suspend while loop.

[Sven Wischnowsky]

Bart Schaefer wrote:

> ...
> 
> It's particularly ugly that you can't ^C out of the loop after the ^Z.
> The shell should not hang uninterrupibly unless the user has really
> wrapped a figurative rope around its figurative neck.

Yep, that's what I was thinking (more or less ;-).

> Does anyone see any problems with the following compromise patch?

It's at least a whole lot better than the previous state.

> ...
> 
> The thing that most worries me is that zread() can invoke zle, which can
> invoke shell functions, which ....

No, it only uses getkey() which doesn't call functions. It's just a
read-one-char that works while zle is active. But it should be
possible to avoid the child_unblock() when `izle != 0', I think.

What I was worried about are complicated jobs with loops and
suspending, but the job-control-tests still work.

> Another question is, is there a more generic way to do this that covers all
> builtin commands and not just `read'?  Or is `read' really the only one
> that needs it?

I can't think of another one right now. We only have to handle
builtins that can potentially block indefinitely, right? (vared comes
to mind, but that can't be used in such a pipe...)

Bye
 Sven


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

Re: PATCH (?): Re: suspend while loop.

[Bart Schaefer]

On May 19,  9:43am, Sven Wischnowsky wrote:
} Subject: Re: PATCH (?): Re: suspend while loop.
}
} Bart Schaefer wrote:
} 
} > The thing that most worries me is that zread() can invoke zle, which can
} > invoke shell functions, which ....
} 
} No, it only uses getkey() which doesn't call functions. It's just a
} read-one-char that works while zle is active. But it should be
} possible to avoid the child_unblock() when `izle != 0', I think.

I don't think that's worth the effort and the messiness; `read' is going
to spend the vast majority of its time in the read-one-character loops,
so as long as the signal changes are outside those loops it should be OK.

BTW, I just noticed this:

	echo -n foo | read line
	echo $?
	echo $line

By comparison, bash also gives $? == 1 but NOT $line == foo.

(Why is lack of a trailing newline an error?)

} > Another question is, is there a more generic way to do this [...?]
} 
} We only have to handle builtins that can potentially block
} indefinitely, right?

Well ... there's the "command1 | builtin ; command2" thing where interrupt
kills command1 but not builtin, causing builtin to exit on EOF and thereby
allowing command2 to run.  Ideally the interrupt would affect command2 as
well.  If builtin is a loop or other construct this already works, so the
questions are (1) can any other builtins be used in a pipe, and (2) can
any syntactic construct other than a pipeline produce the same effect.

Given that modules can do just about anything they want, the answer to (1)
is "yes" even if there's currently no example ...

However, that doesn't affect 3.0.8, so I think I'm going to go ahead with
11461 for that release (unless somebody hollers today to stop me).

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

Re: PATCH (?): Re: suspend while loop.

[Bart Schaefer]

On May 19,  8:30am, Bart Schaefer wrote:
} Subject: Re: PATCH (?): Re: suspend while loop.
}
} However, that doesn't affect 3.0.8, so I think I'm going to go ahead with
} 11461 for that release (unless somebody hollers today to stop me).

Here's one other oddment:

zagzig% cat | while read line; do echo $line; done; :
^Z^C
zsh: suspended  cat | 
zsh: running    while read line; do; echo $line; done
zagzig% jobs
[1]  + suspended  cat | 
       running    while read line; do; echo $line; done
zagzig% fg
[1]  + continued  cat | while read line; do; echo $line; done
foo
zagzig% 


According to Etc/BUGS, the `; :' forces zsh to fork before starting the
while loop.  So in that case I would have expected ^Z to work, but it
doesn't.  Following the ^Z with a ^C appears to background the job, but
that hasn't really happened either; the while loop is not running, it's
already gotten a failure return from read.  Is it sitting there waiting
for "cat" to die?   (Which cat does, from a closed stdout, I think, as
soon as you give it any input.)

I'm still not going to try to deal with this for 3.0.8 except to mention
it in BUGS.

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

Re: suspend while loop.

[Sven Wischnowsky]

Bart Schaefer wrote:

> On May 16, 12:02pm, Bart Schaefer wrote:
> } Subject: Re: suspend while loop.
> }
> } > Z(4):akr@serein% Src/zsh -f
> } > serein% cat |while read line; do echo $line; done
> } > <C-z>
> } > 
> } > `cat' is suspended.  But zsh wait something yet...
> } 
> } I think you can't break out of `read' with ^Z.  Try it without the cat|
> } and you'll find you can't suspend the loop.
> 
> Does anyone have any insights on this?  I was about to announce 3.0.8,
> but discovered that this loop is not suspendable there either.  I don't
> have much time for debugging it myself this week ...
> 
> If it doesn't appear to be easily fixed, I'm going to go ahead and put
> 3.0.8 out anyway.

I can't see how we could (really) make in suspendible. For that we
would need to be able to get out of the read, back to the place where
we can create the sub-shell for the loop. Then, on continuing, we
would need to be able to go into the same read and continue with
it. But the first thing can only be done by returing from the
read. Everyone can easily test this by adding child_unblock() before
the read() in zread() and child_unblock()s after it. This makes the
loop suspendible, but when it is continued, the loop exits immediately
because the read that was suspended returned non-zero.

Depending on personal taste it is either ugly that the cat gets
suspended or that we can't ^C out of the loop after the ^Z.

I've no idea how to solve this.

Bye
 Sven


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

PATCH (?): Re: suspend while loop.

[Bart Schaefer]

On May 18, 10:37am, Sven Wischnowsky wrote:
} Subject: Re: suspend while loop.
}
} Everyone can easily test this by adding child_unblock() before
} the read() in zread() and child_unblock()s after it. This makes the
                                s/un// ?
} loop suspendible, but when it is continued, the loop exits immediately
} because the read that was suspended returned non-zero.

That would be OK, but the overhead of blocking and unblocking signals
around every one-character read() makes me cringe.  

Furthermore, SIGCHLD will cause the read() to return -1 with EINTR,
which we can't have happening in normal operation.

} Depending on personal taste it is either ugly that the cat gets
} suspended or that we can't ^C out of the loop after the ^Z.

It's particularly ugly that you can't ^C out of the loop after the ^Z.
The shell should not hang uninterrupibly unless the user has really
wrapped a figurative rope around its figurative neck.

Does anyone see any problems with the following compromise patch?  This
makes `read' (the builtin, not read(2)) interruptible with ^C even when
it is at the right-hand-end of a pipeline.  For comparison, before and
after the patch try:

zsh% cat | read line ; echo $?
^C

In the before case, echo runs and prints 1.  In the after case, echo does
not run.  This is consistent with

zsh% read line ; echo $?
^C

which, in both before and after cases, does not run echo.

The thing that most worries me is that zread() can invoke zle, which can
invoke shell functions, which ....

Another question is, is there a more generic way to do this that covers all
builtin commands and not just `read'?  Or is `read' really the only one
that needs it?

Index: Src/builtin.c
===================================================================
@@ -3536,6 +3536,7 @@
     first = 1;
     bslash = 0;
     while (*args || (ops['A'] && !gotnl)) {
+	sigset_t s = child_unblock();
 	buf = bptr = (char *)zalloc(bsiz = 64);
 	/* get input, a character at a time */
 	while (!gotnl) {
@@ -3573,6 +3574,7 @@
 		bptr = buf + blen;
 	    }
 	}
+	signal_setmask(s);
 	if (c == '\n' || c == EOF)
 	    gotnl = 1;
 	*bptr = '\0';
@@ -3627,7 +3629,8 @@
     buf = bptr = (char *)zalloc(bsiz = 64);
     /* any remaining part of the line goes into one parameter */
     bslash = 0;
-    if (!gotnl)
+    if (!gotnl) {
+	sigset_t s = child_unblock();
 	for (;;) {
 	    c = zread(izle);
 	    /* \ at the end of a line introduces a continuation line, except in
@@ -3662,6 +3665,8 @@
 		bptr = buf + blen;
 	    }
 	}
+	signal_setmask(s);
+    }
     while (bptr > buf && iwsep(bptr[-1]))
 	bptr--;
     *bptr = '\0';
@@ -3718,8 +3723,8 @@
 	case 1:
 	    /* return the character read */
 	    return STOUC(cc);
-#if defined(EAGAIN) || defined(EWOULDBLOCK)
 	case -1:
+#if defined(EAGAIN) || defined(EWOULDBLOCK)
 	    if (!retry && readfd == 0 && (
 # ifdef EAGAIN
 		    errno == EAGAIN
@@ -3733,9 +3738,11 @@
 		) && setblock_stdin()) {
 		retry = 1;
 		continue;
-	    }
-	    break;
+	    } else
 #endif /* EAGAIN || EWOULDBLOCK */
+	    if (errno == EINTR && !(errflag || retflag || breaks || contflag))
+		continue;
+	    break;
 	}
 	return EOF;
     }

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