RE: PATCH: ptyread eating CPU on Cygwin

[Sven Wischnowsky <wischnow@xxxxxxxxxxxxxxxxxxxxxxx>]

Andrej Borsenkow wrote:

> ...
> 
> Currently zsh tries to always read the whole input (until EOF) Actually, in
> pattern-matching mode it even completely ignores EOF. That makes absolutely no
> functional difference between blocking and non-blocking mode but makes
> performance in non-blocking mode terrible.
> 
> As it stands now, we really can just use blocking read.

Oops. That was a thinko in the patch that allowed to leave a `zpty -r'.
This patch should fix that and adds the -t option that allows to test
if a pty command is still running. There is a problem, in that using
-t could make the command be marked as finished (and the file
descriptor be closed) before its output has been read. I've tried to
solve this by using read_poll() in checkptycmd(). It works for me
here.

I've not added Andrej's select()-patch because I don't know how cygwin
behaves when using (real) non-blocking descriptors.

This also changes comptest and nslookup to use `zpty -b' to get
blocking IO on the pty. Hm, should blocking be the default and -b
select non-blocking behaviour?

Bye
 Sven

Index: Doc/Zsh/mod_zpty.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/mod_zpty.yo,v
retrieving revision 1.2
diff -u -r1.2 mod_zpty.yo
--- Doc/Zsh/mod_zpty.yo	2000/05/21 18:27:36	1.2
+++ Doc/Zsh/mod_zpty.yo	2000/10/19 08:43:51
@@ -9,6 +9,7 @@
 xitem(tt(zpty) tt(-d) [ var(names) ... ])
 xitem(tt(zpty) tt(-w) [ tt(-n) ] var(name) var(strings ...))
 xitem(tt(zpty) tt(-r) var(name) [ var(param) [ var(pattern) ] ])
+xitem(tt(zpty) tt(-t) var(name))
 item(tt(zpty) [ tt(-L) ])(
 In the first form, the var(command) is started with the var(args) as
 arguments.  The command runs under a newly assigned pseudo-terminal; this
@@ -34,6 +35,10 @@
 read will be put in the parameter named var(param).  If the
 var(pattern) is also given, output will be read until the whole string 
 read matches the var(pattern).
+
+The tt(-t) option can be used to test whether the command var(name) is 
+still running.  It returns a zero value if the command is running and
+a non-zero value otherwise.
 
 The last form without any arguments is used to list the commands
 currently defined.  If the tt(-L) option is given, this is done in the
Index: Functions/Misc/nslookup
===================================================================
RCS file: /cvsroot/zsh/zsh/Functions/Misc/nslookup,v
retrieving revision 1.4
diff -u -r1.4 nslookup
--- Functions/Misc/nslookup	2000/05/09 11:56:20	1.4
+++ Functions/Misc/nslookup	2000/10/19 08:43:51
@@ -24,7 +24,7 @@
     [[ -z "$pager" ]] && pager="${opager:-more}"
 (( $#pmpt )) || pmpt=(-p '> ')
 
-zpty nslookup nslookup "$@"
+zpty -b nslookup nslookup "$@"
 
 zpty -r nslookup line '*
 > '
Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.21
diff -u -r1.21 utils.c
--- Src/utils.c	2000/10/02 18:40:36	1.21
+++ Src/utils.c	2000/10/19 08:43:52
@@ -1312,7 +1312,7 @@
  */
 
 /**/
-int
+mod_export int
 read_poll(int fd, int *readchar, int polltty)
 {
     int ret = 0;
Index: Src/Modules/zpty.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/zpty.c,v
retrieving revision 1.11
diff -u -r1.11 zpty.c
--- Src/Modules/zpty.c	2000/06/27 14:25:05	1.11
+++ Src/Modules/zpty.c	2000/10/19 08:43:52
@@ -48,6 +48,7 @@
     int echo;
     int block;
     int fin;
+    int read;
 };
 
 static Ptycmd ptycmds;
@@ -381,6 +382,7 @@
     p->echo = echo;
     p->block = block;
     p->fin = 0;
+    p->read = -1;
 
     p->next = ptycmds;
     ptycmds = p;
@@ -434,7 +436,9 @@
 static void
 checkptycmd(Ptycmd cmd)
 {
-    if (kill(cmd->pid, 0) < 0) {
+    if (cmd->read != -1)
+	return;
+    if (!read_poll(cmd->fd, &cmd->read, 1) && kill(cmd->pid, 0) < 0) {
 	cmd->fin = 1;
 	zclose(cmd->fd);
     }
@@ -444,7 +448,7 @@
 ptyread(char *nam, Ptycmd cmd, char **args)
 {
     int blen = 256, used = 0, ret = 1;
-    char *buf = (char *) zhalloc(blen + 1);
+    char *buf = (char *) zhalloc((blen = 256) + 1);
     Patprog prog = NULL;
 
     if (*args && args[1]) {
@@ -462,6 +466,12 @@
 	    return 1;
 	}
     }
+    if (cmd->read != -1) {
+	buf[0] = (char) cmd->read;
+	buf[1] = '\0';
+	used = 1;
+	cmd->read = -1;
+    }
     do {
 	if (!ret) {
 	    checkptycmd(cmd);
@@ -476,11 +486,10 @@
 	}
 	buf[used] = '\0';
 
-	/**** Hm. If we leave the loop when ret < 0 the user would have
-	 *    to make sure that `zpty -r' is tried more than once if
-	 *    there will be some output and we only got the ret == -1
-	 *    because the output is not yet available.
-	 *    The same for the `write' below. */
+#if 0
+	/* This once used the following test, to make sure to return
+	 * non-zero if there are no characters to read.  That looks
+	 * like a thinko now, because it disables non-blocking ptys. */
 
 	if (ret < 0 && (cmd->block
 #ifdef EWOULDBLOCK
@@ -492,8 +501,9 @@
 #endif
 			))
 	    break;
+#endif
 
-	if (!prog && !ret)
+	if (!prog && ret <= 0)
 	    break;
     } while (!errflag && !breaks && !retflag && !contflag &&
 	     (prog ? (used < READ_MAX && (!ret || !pattry(prog, buf))) :
@@ -514,7 +524,10 @@
     int written;
 
     for (; len; len -= written, s += written) {
-	if ((written = write(cmd->fd, s, len)) < 0 &&
+	if ((written = write(cmd->fd, s, len)) < 0
+#if 0
+	    /* Same as above. */
+	    &&
 	    (cmd->block
 #ifdef EWOULDBLOCK
 			|| errno != EWOULDBLOCK
@@ -522,8 +535,10 @@
 #ifdef EAGAIN
 			|| errno != EAGAIN
 #endif
+#endif
+	     )
 #endif
-	     ))
+	    )
 	    return 1;
 	if (written < 0) {
 	    checkptycmd(cmd);
@@ -567,11 +582,11 @@
 bin_zpty(char *nam, char **args, char *ops, int func)
 {
     if ((ops['r'] && ops['w']) ||
-	((ops['r'] || ops['w']) && (ops['d'] || ops['e'] ||
+	((ops['r'] || ops['w']) && (ops['d'] || ops['e'] || ops['t'] ||
 				    ops['b'] || ops['L'])) ||
-	(ops['n'] && (ops['b'] || ops['e'] || ops['r'] ||
+	(ops['n'] && (ops['b'] || ops['e'] || ops['r'] || ops['t'] ||
 		      ops['d'] || ops['L'])) ||
-	(ops['d'] && (ops['b'] || ops['e'] || ops['L'])) ||
+	(ops['d'] && (ops['b'] || ops['e'] || ops['L'] || ops['t'])) ||
 	(ops['L'] && (ops['b'] || ops['e']))) {
 	zwarnnam(nam, "illegal option combination", NULL, 0);
 	return 1;
@@ -607,6 +622,18 @@
 	    deleteallptycmds();
 
 	return ret;
+    } else if (ops['t']) {
+	Ptycmd p;
+
+	if (!*args) {
+	    zwarnnam(nam, "missing pty command name", NULL, 0);
+	    return 1;
+	} else if (!(p = getptycmd(*args))) {
+	    zwarnnam(nam, "no such pty command: %s", *args, 0);
+	    return 1;
+	}
+	checkptycmd(p);
+	return p->fin;
     } else if (*args) {
 	if (!args[1]) {
 	    zwarnnam(nam, "missing command", NULL, 0);
@@ -649,7 +676,7 @@
 }
 
 static struct builtin bintab[] = {
-    BUILTIN("zpty", 0, bin_zpty, 0, -1, 0, "ebdrwLn", NULL),
+    BUILTIN("zpty", 0, bin_zpty, 0, -1, 0, "ebdrwLnt", NULL),
 };
 
 /**/
Index: Test/comptest
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/comptest,v
retrieving revision 1.8
diff -u -r1.8 comptest
--- Test/comptest	2000/07/04 00:45:07	1.8
+++ Test/comptest	2000/10/19 08:43:52
@@ -16,7 +16,7 @@
   (( OPTIND > 1 )) && shift $(( OPTIND - 1 ))
 
   export PS1="<PROMPT>"
-  zpty zsh "$comptest_zsh" -f
+  zpty -b zsh "$comptest_zsh" -f
 
   zpty -r zsh log1 "*<PROMPT>*" || { 
     print "first prompt hasn't appeared."

--
Sven Wischnowsky                         wischnow@xxxxxxxxxxxxxxxxxxxxxxx