Re: It seems that I find a zle -F full CPU bug

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Feb 20, 12:47pm, Bart Schaefer wrote:
} Subject: Re: It seems that I find a zle -F full CPU bug
}
} ... It appears that a widget handlers (zle -F -w) will never get
} anything but a descriptor number.  Without -w, the handler will get either
} the descriptor number or a string describing the error condition (which
} means in order to remove itself on an error, the handler has to know out of
} band which descriptor it is watching?).  Those error-string values are not
} documented.

OK, re-reading this, the handler gets the FD in $1 and the string in $2.
In practice though I'm not sure it ever received anything in $2 except
possibly "hup" (if POLLHUP comes along with POLLIN).  Widget handlers
can't get more than one argument at present so we'll have to document.

For select() we'd have to do extra work to discover why the descriptor
has an "exception" state.  For now I'll settle for passing "err" as $2.
 
} ... In the HAVE_POLL branch, for example, it first looks
} for the POLLIN flags but not for any error flags?  So it will never reach
} the set of tests that examine POLLNVAL et al.?

Just for sanity, I've added POLLERR et al. to the bitmask.

} That aside, dropping the invalid descriptors for the select() case is easy,
} just change the bitflags.  For the poll() case it's going to be more work
} because the fds[] array of structs will have to be rearranged.

More oddness:  Inside the loop, the code specifically covers the case of
the handler removing itself: 

		/*
		 * Copy the details of the watch fds in case the
		 * user decides to delete one from inside the
		 * handler function.
		 */
		int lnwatch = nwatch;
		Watch_fd lwatch_fds = zalloc(lnwatch*sizeof(struct watch_fd));
		memcpy(lwatch_fds, watch_fds, lnwatch*sizeof(struct watch_fd));

Nowever, the fds[] array is only populated once, *outside* the loop:

	fds[0].events = POLLIN;
	for (i = 0; i < nwatch; i++) {
	    fds[i+1].fd = watch_fds[i].fd;
	    fds[i+1].events = POLLIN;
	}
# endif
	for (;;) {

I believe this means if a handler removes itself, or opens another FD and
adds a new handler for that, this will not be discovered until the loop
restarts (e.g., upon interactive input).

Furthermore, the code assumes that fds[i+1].fd == watch_fds[i].fd for the
lifetime of the loop, which might not be true if e.g. a signal trap adds
or removes (or both) a descriptor handler.  Once again the select case
completely avoids this.

The following patch attempts to take care of most of these issues.  I
tested it with both lilydjwg's print_and_close function example and my
vcs_update_info widget and both worked as expected; print_and_close
sometimes prints both an FD and "hup" and sometimes just an FD, but on
my system at least it no longer busy-waits.

diff --git a/Doc/Zsh/zle.yo b/Doc/Zsh/zle.yo
index 6d3bb4b..c3bab09 100644
--- a/Doc/Zsh/zle.yo
+++ b/Doc/Zsh/zle.yo
@@ -492,26 +492,35 @@ Only available if your system supports one of the `poll' or `select' system
 calls; most modern systems do.
 
 Installs var(handler) (the name of a shell function) to handle input from
-file descriptor var(fd).  When zle is attempting to read data, it will
-examine both the terminal and the list of handled var(fd)'s.  If data
-becomes available on a handled var(fd), zle will call var(handler) with
-the fd which is ready for reading as the only argument.  If the handler
-produces output to the terminal, it should call `tt(zle -I)' before doing
-so (see below).  The handler should not attempt to read from the terminal.
-Note that zle makes no attempt to check whether this fd is actually
+file descriptor var(fd).  Installing a handler for an var(fd) which is
+already handled causes the existing handler to be replaced.  Any number of
+handlers for any number of readable file descriptors may be installed.
+Note that zle makes no attempt to check whether this var(fd) is actually
 readable when installing the handler.  The user must make their own
 arrangements for handling the file descriptor when zle is not active.
 
-If the option tt(-w) is also given, the var(handler) is instead a
-line editor widget, typically a shell function made into a widget using
-tt(zle -N).  In that case var(handler) can use all the facilities of
-zle to update the current editing line.  Note, however, that as handling
-var(fd) takes place at a low level changes to the display will not
-automatically appear; the widget should call tt(zle -R) to force redisplay.
-
-Any number of handlers for any number of readable file descriptors may be
-installed.  Installing a handler for an var(fd) which is already handled
-causes the existing handler to be replaced.
+When zle is attempting to read data, it will examine both the terminal and
+the list of handled var(fd)'s.  If data becomes available on a handled
+var(fd), zle calls var(handler) with the fd which is ready for reading
+as the first argument.  Under normal circumstances this is the only
+argument, but if an error was detected, a second argument provides
+details: `tt(hup)' for a disconnect, `tt(nval)' for a closed or otherwise
+invalid descriptor, or `tt(err)' for any other condition.  Systems that
+support only the `select' system call always use `tt(err)'.
+
+If the option tt(-w) is also given, the var(handler) is instead a line
+editor widget, typically a shell function made into a widget using
+`tt(zle -N)'.  In that case var(handler) can use all the facilities of zle
+to update the current editing line.  Note, however, that as handling var(fd)
+takes place at a low level changes to the display will not automatically
+appear; the widget should call `tt(zle -R)' to force redisplay.  As of this
+writing, widget handlers only support a single argument and thus are never
+passed a string for error state, so widgets must be prepared to test the
+descriptor themselves.
+
+If either type of handler produces output to the terminal, it should call
+`tt(zle -I)' before doing so (see below).  Handlers should not attempt to
+read from the terminal.
 
 If no var(handler) is given, but an var(fd) is present, any handler for
 that var(fd) is removed.  If there is none, an error message is printed
@@ -526,7 +535,8 @@ silently return status 1.
 
 Note that this feature should be used with care.  Activity on one of the
 var(fd)'s which is not properly handled can cause the terminal to become
-unusable.
+unusable.  Removing an var(fd) handler from within a signal trap may cause
+unpredictable behavior.
 
 Here is a simple example of using this feature.  A connection to a remote
 TCP port is created using the ztcp command; see 
@@ -536,6 +546,7 @@ which simply prints out any data which arrives on this connection.  Note
 that `select' will indicate that the file descriptor needs handling
 if the remote side has closed the connection; we handle that by testing
 for a failed read.
+
 example(if ztcp pwspc 2811; then
   tcpfd=$REPLY
   handler+LPAR()RPAR() {
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index b0010fc..442c319 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -525,7 +525,8 @@ raw_getbyte(long do_keytmout, char *cptr)
 #endif
 #ifndef HAVE_POLL
 # ifdef HAVE_SELECT
-    fd_set foofd;
+    fd_set foofd, errfd;
+    FD_ZERO(&errfd);
 # endif
 #endif
 
@@ -613,11 +614,14 @@ raw_getbyte(long do_keytmout, char *cptr)
 	    if (!errtry) {
 		for (i = 0; i < nwatch; i++) {
 		    int fd = watch_fds[i].fd;
+		    if (FD_ISSET(fd, &errfd))
+			continue;
 		    FD_SET(fd, &foofd);
 		    if (fd > fdmax)
 			fdmax = fd;
 		}
 	    }
+	    FD_ZERO(&errfd);
 
 	    if (tmout.tp != ZTM_NONE) {
 		expire_tv.tv_sec = tmout.exp100ths / 100;
@@ -732,9 +736,10 @@ raw_getbyte(long do_keytmout, char *cptr)
 		    Watch_fd lwatch_fd = lwatch_fds + i;
 		    if (
 # ifdef HAVE_POLL
-			(fds[i+1].revents & POLLIN)
+			(fds[i+1].revents & (POLLIN|POLLERR|POLLHUP|POLLNVAL))
 # else
-			FD_ISSET(lwatch_fd->fd, &foofd)
+			FD_ISSET(lwatch_fd->fd, &foofd) ||
+			FD_ISSET(lwatch_fd->fd, &errfd)
 # endif
 			) {
 			/* Handle the fd. */
@@ -765,6 +770,9 @@ raw_getbyte(long do_keytmout, char *cptr)
 			    if (fds[i+1].revents & POLLNVAL)
 				zaddlinknode(funcargs, ztrdup("nval"));
 #  endif
+# else
+			    if (FD_ISSET(lwatch_fd->fd, &errfd))
+				zaddlinknode(funcargs, ztrdup("err"));
 # endif
 			    callhookfunc(lwatch_fd->func, funcargs, 0, NULL);
 			    freelinklist(funcargs, freestr);
@@ -786,6 +794,31 @@ raw_getbyte(long do_keytmout, char *cptr)
 		for (i = 0; i < lnwatch; i++)
 		    zsfree(lwatch_fds[i].func);
 		zfree(lwatch_fds, lnwatch*sizeof(struct watch_fd));
+
+# ifdef HAVE_POLL
+		/* Function may have added or removed handlers */
+		nfds = 1 + nwatch;
+		if (nfds > 1) {
+		    fds = zrealloc(fds, sizeof(struct pollfd) * nfds);
+		    for (i = 0; i < nwatch; i++) {
+			/*
+			 * This is imperfect because it assumes fds[] and
+			 * watch_fds[] remain in sync, which may be false
+			 * if handlers are shuffled.  However, it should
+			 * be harmless (e.g., produce one extra pass of
+			 * the loop) in the event they fall out of sync.
+			 */
+			if (fds[i+1].fd == watch_fds[i].fd &&
+			    (fds[i+1].revents & (POLLERR|POLLHUP|POLLNVAL))) {
+			    fds[i+1].events = 0;	/* Don't poll this */
+			} else {
+			    fds[i+1].fd = watch_fds[i].fd;
+			    fds[i+1].events = POLLIN;
+			}
+			fds[i+1].revents = 0;
+		    }
+		}
+# endif
 	    }
 	}
 # ifdef HAVE_POLL

-- 
Barton E. Schaefer