Re: PATCH: ptyread eating CPU on Cygwin

Re: PATCH: ptyread eating CPU on Cygwin

[Sven Wischnowsky]

Bart Schaefer wrote:

> On Oct 19,  1:49pm, Andrej Borsenkow wrote:
> }
> } So I suggest adding "read-with-timeout" possibilty, and making blocking read
> } default. It is also useful to distinguish between EOF and timeout with
> } different return code.
> 
> This sounds right to me, too.

Yes, to me, too.  I hope to find the time at the weekend...

I even think about removin non-blocking ptys altogether -- and add
allow `zpty -w' with a timeout, too.  Hm, the print builtin can't be
given a timeout, but the -t option for it is still unused. Should we?

Bye
 Sven


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

RE: PATCH: ptyread eating CPU on Cygwin

[Andrej Borsenkow]

>
> I even think about removin non-blocking ptys altogether -- and add
> allow `zpty -w' with a timeout, too.  Hm, the print builtin can't be
> given a timeout, but the -t option for it is still unused. Should we?
>

zpty -w with timeout is right stuff. I think, we better leave non-blocking
mode in - may be, there are some exotic cases when it may be useful. And in
case of non-bocking read with pattern add return code 'did not match' (it
cannot happen in case of blocking read without either EOF or timeout).

But we need better semantic for zpty -r then. Without pattern, how much
exactly should it read? A character at a time? A line?

-andrej

RE: PATCH: ptyread eating CPU on Cygwin

[Sven Wischnowsky]

Andrej Borsenkow wrote:

> > I even think about removin non-blocking ptys altogether -- and add
> > allow `zpty -w' with a timeout, too.  Hm, the print builtin can't be
> > given a timeout, but the -t option for it is still unused. Should we?
> 
> zpty -w with timeout is right stuff. I think, we better leave non-blocking
> mode in - may be, there are some exotic cases when it may be useful. And in
> case of non-bocking read with pattern add return code 'did not match' (it
> cannot happen in case of blocking read without either EOF or timeout).
> 
> But we need better semantic for zpty -r then. Without pattern, how much
> exactly should it read? A character at a time? A line?


This is the patch I wrote at the weekend. I won't commit it yet
because it contains several changes in read_poll() and I'd like to get 
comments about them from the one(s) who wrote that. There were some
things in it I think were really wrong, like testing `select() > 1'
and like trying all possible tests if the first tests worked but
returned zero.


Other changes:
- blocking is the default, -b selects non-blocking mode
- -r returns one if nothing could be read and 2 if EOF was encountered
- -t can be combined with -r to do the same as in the read builtin,
  i.e. it doesn't allow to specify a timeout, but it first (tries to)
  checks if input is available and doesn't try to read if there is
  none
- -w can be interrupted with ^C (even if the rest of the patch doesn't 
  make it in, this will be added)

I have not written the code to allow to combine -t with -w (or the
print builtin) yet. Will that have to be as complicated as read_poll()?

Bye
 Sven

diff -u -r ../oz/Doc/Zsh/mod_zpty.yo ./Doc/Zsh/mod_zpty.yo
--- ../oz/Doc/Zsh/mod_zpty.yo	Sat Oct 21 20:35:07 2000
+++ ./Doc/Zsh/mod_zpty.yo	Sat Oct 21 23:57:41 2000
@@ -8,7 +8,7 @@
 xitem(tt(zpty) [ tt(-e) ] [ tt(-b) ] var(name) var(command) [ var(args ...) ])
 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(-r) [ tt(-t) ] 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
@@ -18,7 +18,7 @@
 command in later calls to tt(pty).  With the tt(-e) option given, the
 pseudo-terminal will be set up so that input characters are echoed and 
 with the tt(-b) option given, input and output from and to the
-pseudo-terminal will be blocking.
+pseudo-terminal will be non-blocking.
 
 The second form with the tt(-d) option is used to delete commands
 previously started by supplying a list of their var(name)s.  If no
@@ -34,11 +34,20 @@
 printed to standard output.  With a var(param) argument, the string
 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).
+read matches the var(pattern).  The return value is zero if anything
+could be read (with a var(pattern), the return value will only be zero 
+if the string read matches the pattern or if the command isn't running 
+anymore and at least one character could still be read).  The return
+value is non-zero if nothing could be read and it will be two if this
+is because the command has finished.
 
-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.
+If the tt(-r) option is combined with the tt(-t) option, tt(zpty) will 
+test if input is available before trying to read.  If no input is
+available, tt(zpty) immediately returns the value `tt(1)'.
+
+The tt(-t) option without the tt(-r) 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
diff -u -r ../oz/Functions/Misc/nslookup ./Functions/Misc/nslookup
--- ../oz/Functions/Misc/nslookup	Sat Oct 21 20:35:17 2000
+++ ./Functions/Misc/nslookup	Sat Oct 21 21:38:28 2000
@@ -24,7 +24,7 @@
     [[ -z "$pager" ]] && pager="${opager:-more}"
 (( $#pmpt )) || pmpt=(-p '> ')
 
-zpty -b nslookup nslookup "$@"
+zpty nslookup nslookup "$@"
 
 zpty -r nslookup line '*
 > '
diff -u -r ../oz/Src/Modules/zpty.c ./Src/Modules/zpty.c
--- ../oz/Src/Modules/zpty.c	Sat Oct 21 20:35:13 2000
+++ ./Src/Modules/zpty.c	Sun Oct 22 00:08:00 2000
@@ -34,7 +34,6 @@
  * upper bound on the number of bytes we read (even if we are give a
  * pattern). */
 
-#define READ_LEN 1024
 #define READ_MAX (1024 * 1024)
 
 typedef struct ptycmd *Ptycmd;
@@ -46,9 +45,10 @@
     int fd;
     int pid;
     int echo;
-    int block;
+    int nblock;
     int fin;
     int read;
+    char *old;
 };
 
 static Ptycmd ptycmds;
@@ -264,7 +264,7 @@
 #endif /* __SVR4 */
 
 static int
-newptycmd(char *nam, char *pname, char **args, int echo, int block)
+newptycmd(char *nam, char *pname, char **args, int echo, int nblock)
 {
     Ptycmd p;
     int master, slave, pid;
@@ -380,14 +380,15 @@
     p->fd = master;
     p->pid = pid;
     p->echo = echo;
-    p->block = block;
+    p->nblock = nblock;
     p->fin = 0;
     p->read = -1;
+    p->old = NULL;
 
     p->next = ptycmds;
     ptycmds = p;
 
-    if (!block)
+    if (nblock)
 	ptynonblock(master);
 
     return 0;
@@ -447,8 +448,8 @@
 static int
 ptyread(char *nam, Ptycmd cmd, char **args)
 {
-    int blen = 256, used = 0, ret = 1;
-    char *buf = (char *) zhalloc((blen = 256) + 1);
+    int blen, used, ret = 1;
+    char *buf;
     Patprog prog = NULL;
 
     if (*args && args[1]) {
@@ -466,9 +467,19 @@
 	    return 1;
 	}
     }
+    if (cmd->old) {
+	used = strlen(cmd->old);
+	buf = (char *) zhalloc((blen = 256 + used) + 1);
+	strcpy(buf, cmd->old);
+	zsfree(cmd->old);
+	cmd->old = NULL;
+    } else {
+	used = 0;
+	buf = (char *) zhalloc((blen = 256) + 1);
+    }
     if (cmd->read != -1) {
-	buf[0] = (char) cmd->read;
-	buf[1] = '\0';
+	buf[used] = (char) cmd->read;
+	buf[used + 1] = '\0';
 	used = 1;
 	cmd->read = -1;
     }
@@ -486,68 +497,61 @@
 	}
 	buf[used] = '\0';
 
-#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 (!prog && (ret <= 0 || (ret == 1 && buf[used - 1] == '\n')))
+	    break;
+    } while (!errflag && !breaks && !retflag && !contflag &&
+	     used < READ_MAX && (prog ? (!ret || !pattry(prog, buf)) : 1));
 
-	if (ret < 0 && (cmd->block
+    if (prog && ret < 0 &&
 #ifdef EWOULDBLOCK
-			|| errno != EWOULDBLOCK
+	errno == EWOULDBLOCK
 #else
 #ifdef EAGAIN
-			|| errno != EAGAIN
+	errno == EAGAIN
 #endif
 #endif
-			))
-	    break;
-#endif
-
-	if (!prog && ret <= 0)
-	    break;
-    } while (!errflag && !breaks && !retflag && !contflag &&
-	     (prog ? (used < READ_MAX && (!ret || !pattry(prog, buf))) :
-	      (used < READ_LEN)));
+	) {
+	cmd->old = ztrdup(buf);
+	used = 0;
 
+	return 1;
+    }
     if (*args)
 	setsparam(*args, ztrdup(metafy(buf, used, META_HREALLOC)));
     else {
 	fflush(stdout);
 	write(1, buf, used);
     }
-    return !used;
+    return (used ? 0 : cmd->fin + 1);
 }
 
 static int
 ptywritestr(Ptycmd cmd, char *s, int len)
 {
-    int written;
+    int written, all = 0;
 
-    for (; len; len -= written, s += written) {
-	if ((written = write(cmd->fd, s, len)) < 0
-#if 0
-	    /* Same as above. */
-	    &&
-	    (cmd->block
+    for (; !errflag && !breaks && !retflag && !contflag && len;
+	 len -= written, s += written) {
+	if ((written = write(cmd->fd, s, len)) < 0 && cmd->nblock &&
 #ifdef EWOULDBLOCK
-			|| errno != EWOULDBLOCK
+	    errno == EWOULDBLOCK
 #else
 #ifdef EAGAIN
-			|| errno != EAGAIN
-#endif
+	    errno == EAGAIN
 #endif
-	     )
 #endif
 	    )
-	    return 1;
+	    return !all;
 	if (written < 0) {
 	    checkptycmd(cmd);
 	    if (cmd->fin)
 		break;
 	    written = 0;
 	}
+	if (written > 0)
+	    all += written;
     }
-    return 0;
+    return (all ? 0 : cmd->fin + 1);
 }
 
 static int
@@ -582,8 +586,9 @@
 bin_zpty(char *nam, char **args, char *ops, int func)
 {
     if ((ops['r'] && ops['w']) ||
-	((ops['r'] || ops['w']) && (ops['d'] || ops['e'] || ops['t'] ||
+	((ops['r'] || ops['w']) && (ops['d'] || ops['e'] ||
 				    ops['b'] || ops['L'])) ||
+	(ops['w'] && ops['t']) ||
 	(ops['n'] && (ops['b'] || ops['e'] || ops['r'] || ops['t'] ||
 		      ops['d'] || ops['L'])) ||
 	(ops['d'] && (ops['b'] || ops['e'] || ops['L'] || ops['t'])) ||
@@ -602,7 +607,10 @@
 	    return 1;
 	}
 	if (p->fin)
+	    return 2;
+	if (ops['t'] && p->read == -1 && !read_poll(p->fd, &p->read, 1))
 	    return 1;
+
 	return (ops['r'] ?
 		ptyread(nam, p, args + 1) :
 		ptywrite(p, args + 1, ops['n']));
@@ -652,7 +660,7 @@
 	    checkptycmd(p);
 	    if (ops['L'])
 		printf("%s %s%s%s ", nam, (p->echo ? "-e " : ""),
-		       (p->block ? "-b " : ""), p->name);
+		       (p->nblock ? "-b " : ""), p->name);
 	    else if (p->fin)
 		printf("(finished) %s: ", p->name);
 	    else
diff -u -r ../oz/Src/utils.c ./Src/utils.c
--- ../oz/Src/utils.c	Sat Oct 21 20:35:10 2000
+++ ./Src/utils.c	Sun Oct 22 00:17:51 2000
@@ -1315,7 +1315,7 @@
 mod_export int
 read_poll(int fd, int *readchar, int polltty)
 {
-    int ret = 0;
+    int ret = -1;
     long mode = -1;
     char c;
 #ifdef HAVE_SELECT
@@ -1360,31 +1360,26 @@
     polltty = 0;
 #endif
 #ifdef HAVE_SELECT
-    if (!ret) {
-	expire_tv.tv_sec = expire_tv.tv_usec = 0;
-	FD_ZERO(&foofd);
-	FD_SET(fd, &foofd);
-	if (select(fd+1, (SELECT_ARG_2_T) &foofd, NULL, NULL, &expire_tv)
-	    > 1)
-	    ret = 1;
-    }
+    expire_tv.tv_sec = expire_tv.tv_usec = 0;
+    FD_ZERO(&foofd);
+    FD_SET(fd, &foofd);
+    ret = select(fd+1, (SELECT_ARG_2_T) &foofd, NULL, NULL, &expire_tv);
 #else
 #ifdef FIONREAD
-    if (!ret) {
-	ioctl(fd, FIONREAD, (char *)&val);
-	if (val)
+    if (ret < 0) {
+	ioctl(fd, FIONREAD, (char *) &val);
+	if (val >= 0)
 	    ret = 1;
     }
 #endif
 #endif
 
-    if (!ret) {
+    if (ret < 0) {
 	/*
 	 * Final attempt: set non-blocking read and try to read a character.
 	 * Praise Bill, this works under Cygwin (nothing else seems to).
 	 */
-	if ((polltty || setblock_fd(0, fd, &mode))
-	    && read(fd, &c, 1) > 0) {
+	if ((polltty || setblock_fd(0, fd, &mode)) && read(fd, &c, 1) > 0) {
 	    *readchar = STOUC(c);
 	    ret = 1;
 	}
@@ -1397,7 +1392,7 @@
 	settyinfo(&ti);
     }
 #endif
-    return ret;
+    return (ret > 0);
 }
 
 /**/

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

Re: PATCH: ptyread eating CPU on Cygwin

[Bart Schaefer]

On Oct 19,  1:49pm, Andrej Borsenkow wrote:
}
} So I suggest adding "read-with-timeout" possibilty, and making blocking read
} default. It is also useful to distinguish between EOF and timeout with
} different return code.

This sounds right to me, too.

-- 
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: PATCH: ptyread eating CPU on Cygwin

[Andrej Borsenkow]

> 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?
>

Yes, I'd say. I've tried to find reasonable usage for non-blocking mode and
failed. Consider "zpty -r foo bar pat" in non-blocking mode. It reads whatever
was available and returns error code if it did not match. So, what can program
now do with 'bar' content? Request additional read? That returns us to the
same busy loop problem. Or just through away. (This was discussed when zpty
was born).

So I suggest adding "read-with-timeout" possibilty, and making blocking read
default. It is also useful to distinguish between EOF and timeout with
different return code.

-andrej

RE: PATCH: ptyread eating CPU on Cygwin

[Sven Wischnowsky]

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@informatik.hu-berlin.de

RE: PATCH: ptyread eating CPU on Cygwin

[Andrej Borsenkow]

>
> I'm not Sven, but HAVE_SELECT does not imply having FD_ZERO or an fd_set
> typedef.
>
> On the other hand, I've just noticed that other code assumes in several
> places that it does imply those things, so it's probably OK.
>

Yes, it was just copy'n'paste.

> However, using a NULL timeout structure in the select() call has turned a
> non-blocking read into a fully blocking one.  If you're going to do that,
> you might as well throw out the select() call and just set the descriptor
> back to blocking state before calling read().  I suspect there's a good
> reason it was a non-blocking read in the first place, though.
>

Erm. You are right, of course.

Non-blocking read made sense in initial implemetation, because it was the
condition to exit read loop (without pattern matching) - 9390:

+    do {
+	while ((ret = read(cmd->fd, buf + used, 1)) == 1) {
+	    if (++used == blen) {
+		buf = hrealloc(buf, blen, blen << 1);
+		blen <<= 1;
+	    }
+	}
+	buf[used] = '\0';
+    } while (prog && !pattry(prog, buf));

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.

-andrej

Re: PATCH: ptyread eating CPU on Cygwin

[Bart Schaefer]

On Oct 18,  3:30pm, Andrej Borsenkow wrote:
}
} O.K., this is very simple patch that makes ptyread use select. Sven,
} could you look at conditions there?

I'm not Sven, but HAVE_SELECT does not imply having FD_ZERO or an fd_set
typedef.

On the other hand, I've just noticed that other code assumes in several
places that it does imply those things, so it's probably OK.

However, using a NULL timeout structure in the select() call has turned a
non-blocking read into a fully blocking one.  If you're going to do that,
you might as well throw out the select() call and just set the descriptor
back to blocking state before calling read().  I suspect there's a good
reason it was a non-blocking read in the first place, though.

-- 
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   

PATCH: ptyread eating CPU on Cygwin

[Andrej Borsenkow]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

>
> The problem I was having was with putting the terminal temporarily into raw
> mode without the full majesty of zle and setting a timeout there (in
> bin_read()).  select is working fine in the main terminal code.  I didn't
> have the week necessary to resolve all the possible ways of setting
> timeouts, so I gave up.  I expect it can be done.
>

O.K., this is very simple patch that makes ptyread use select. Sven, could you
look at conditions there? It should be O.K. (blocking select can return either
positive value or -1 if interrupted, in which case we simply exit read loop).

With this patch 'make check' has acceptable CPU load.

Attached due to line length.

-andrej

[-- Attachment #2: zsh-zpty.diff --]
[-- Type: application/octet-stream, Size: 986 bytes --]

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/18 11:26:20
@@ -446,6 +446,9 @@
     int blen = 256, used = 0, ret = 1;
     char *buf = (char *) zhalloc(blen + 1);
     Patprog prog = NULL;
+#ifdef HAVE_SELECT
+    fd_set foofd;
+#endif
 
     if (*args && args[1]) {
 	char *p;
@@ -468,7 +471,16 @@
 	    if (cmd->fin)
 		break;
 	}
-	if ((ret = read(cmd->fd, buf + used, 1)) == 1) {
+#ifdef HAVE_SELECT
+        FD_ZERO(&foofd);
+        FD_SET(cmd->fd, &foofd);
+#endif
+        if (
+#ifdef HAVE_SELECT
+	    (ret = select(cmd->fd+1, (SELECT_ARG_2_T) &foofd, NULL, NULL, NULL) > 0) &&
+#endif
+
+	(ret = read(cmd->fd, buf + used, 1)) == 1) {
 	    if (++used == blen) {
 		buf = hrealloc(buf, blen, blen << 1);
 		blen <<= 1;