Speed up 'read', fixing macOS hang (take 2)

This fixes a hanging bug that could occur on macOS when using the 'read' command to read from a FIFO and encountering end-of-file without a final newline character. It also makes the 'read' command perform 15-25% faster on macOS and Linux. The previous version (ff385e5a) failed on SunOS/Solaris/Illumos because those systems apparently don't (fully) support the POSIX standard recv(2) syscall with MSG_PEEK[*], which is the feature that iffe detects under the 'socket_peek' identifier. On Illumos, using that methods causes a compilation failure (unknown identifier MSG_PEEK); on Solaris 11.4, that method causes multiple regressions in tests/io.sh, suggesting the method compiles but doesn't work at all. Instead, SunOS/Solaris/Illumos requires the method using ioctl(2)+I_PEEK and select(2). No other system that ksh currently builds on requires this method, so it is now only used on SunOS/Solaris/Illumos. So far, this version of sfpkrd() has been tested to work correctly on Linux, macOS, FreeBSD, NetBSD, OpenBSD, HP-UX, Solaris, and OmniOS (an Illumos distribution). It still fails to peek on Cygwin, but in the exact same way it failed before, so that's no loss. To test, run the 'io' test set: bin/shtests -p io src/lib/libast/sfio/sfpkrd.c: sfpkrd(): - Remove long-obsolete Mac OS X and Solaris bug workarounds. - Remove methods that are no longer needed. On systems with a POSIX compliant recv(2), the only thing that is required to avoid regressions is the code that was conditional upon the socket_peek feature test, which tests for the correct functioning of the recv(2) syscall. This has now been made mandatory for non-SunOS/Solaris/Illumos systems (using an #error directive if it is not detected), with the other methods removed. The result performs 15-25% faster on macOS and Linux while passing all the regression tests. On macOS, avoiding the select(2) method fixes the hanging bug. On SunOS/Solaris/Illumos (the '__sun' identifier), the method using ioctl(2)+I_PEEK and select(2) (iffe feature IDs: stream_peek and lib_select) is preserved. Resolves: https://github.com/ksh93/ksh/issues/118 (again) [*] https://pubs.opengroup.org/onlinepubs/9699919799/functions/recv.html
2025-02-13 11:42:21 +00:00 · 2020-08-19 21:58:46 +01:00 · 2020-08-19 21:58:46 +01:00 · 9ba2c2e0df
commit 9ba2c2e0df
parent 569c1bb9c1
4 changed files with 57 additions and 139 deletions
--- a/5
+++ b/5
@ -3,6 +3,11 @@ For full details, see the git log at: https://github.com/ksh93/ksh

 Any uppercase BUG_* names are modernish shell bug IDs.

+2020-08-19:
+
+- Sped up the 'read' command on most systems by 15-25%. Fixed a hanging bug
+  on reading from a FIFO that could occur on macOS.
+
 2020-08-17:

 - 'command -p' incorrectly used the hash table entry (a.k.a. tracked alias)
--- a/src/cmd/ksh93/include/version.h
+++ b/src/cmd/ksh93/include/version.h
@ -17,4 +17,4 @@
 *                  David Korn <dgk@research.att.com>                   *
 *                                                                      *
 ***********************************************************************/
-#define SH_RELEASE	"93u+m 2020-08-17"
+#define SH_RELEASE	"93u+m 2020-08-19"
--- a/src/cmd/ksh93/tests/io.sh
+++ b/src/cmd/ksh93/tests/io.sh
@ -583,5 +583,19 @@ then	exec 3>&-
 	err_exit "Open file descriptor leaks out of subshell"
 fi

+# ======
+# On unpatched ksh on macOS, 'read' used to block when reading from a FIFO and there was no final newline.
+if	mkfifo "$tmp/fifo_no_lf"
+then	trap 'sleep_pid=0; kill "$ksh_pid"; err_exit "'\''read'\'' hangs on EOF without final linefeed when reading from FIFO"' TERM
+	(sleep 1; kill "$$") &
+	sleep_pid=$!
+	"$SHELL" -c 'print -n foo >$0 & while read f; do :; done <$0' "$tmp/fifo_no_lf" &
+	ksh_pid=$!
+	wait "$ksh_pid"
+	trap - TERM
+	((sleep_pid)) && kill "$sleep_pid"
+else	err_exit "mkfifo failed; cannot test reading from FIFO"
+fi
+
 # ======
 exit $((Errors<125?Errors:125))
--- a/src/lib/libast/sfio/sfpkrd.c
+++ b/src/lib/libast/sfio/sfpkrd.c
@ -20,11 +20,20 @@
 *                                                                      *
 ***********************************************************************/
 #include	"sfhdr.h"
-#if !_PACKAGE_ast
-#ifndef FIONREAD
-#if _sys_ioctl
-#include	<sys/ioctl.h>
+
+/* The following are only needed on Solaris/Illumos, as it doesn't support POSIX recv(2) with MSG_PEEK.
+ * On at least macOS and Linux, sfpkrd() runs significantly faster if we disable these. */
+#if !__sun
+#undef _stream_peek
+#undef _lib_select
 #endif
+
+/* Non-SunOS systems require POSIX recv(2) with MSG_PEEK, which is detected as 'socket_peek'. */
+#if !_socket_peek && !__sun
+#if __APPLE__
+#error The socket_peek feature is required. (Hey Apple, revert your src__lib__libast__features__lib.diff patch; it caused multiple regressions, and the hanging bug it fixed is now fixed correctly. See <https://github.com/ksh93/ksh/issues/118>.)
+#else
+#error The socket_peek feature is required.
 #endif
 #endif

@ -71,52 +80,31 @@ int	action;	/* >0: peeking, if rc>=0, get action records,
 		r = -1;
 #if _stream_peek
 		if((t&STREAM_PEEK) && (ntry == 1 || tm < 0) )
-		{
-#ifdef __sun
-			/*
-			 * I_PEEK on stdin can hang rsh+ksh on solaris
-			 * this kludge will have to do until sun^H^H^Horacle fixes I_PEEK/rsh
-			 */
-			static int	stream_peek;
-			if (stream_peek == 0) /* this will be done just once */
-			{	char	*e;
-				stream_peek = (
-					getenv("LOGNAME") == 0 &&
-					getenv("MAIL") == 0 &&
-					((e = getenv("LANG")) == 0 || strcmp(e, "C") == 0) &&
-					((e = getenv("PATH")) == 0 || strncmp(e, "/usr/bin:", 9) == 0)
-					) ? -1 : 1;
-			}
-			if(stream_peek < 0)
-				t &= ~STREAM_PEEK;
-			else
-#endif
-			{	struct strpeek	pbuf;
-				pbuf.flags = 0;
-				pbuf.ctlbuf.maxlen = -1;
-				pbuf.ctlbuf.len = 0;
-				pbuf.ctlbuf.buf = NIL(char*);
-				pbuf.databuf.maxlen = n;
-				pbuf.databuf.buf = buf;
-				pbuf.databuf.len = 0;
+		{	struct strpeek	pbuf;
+			pbuf.flags = 0;
+			pbuf.ctlbuf.maxlen = -1;
+			pbuf.ctlbuf.len = 0;
+			pbuf.ctlbuf.buf = NIL(char*);
+			pbuf.databuf.maxlen = n;
+			pbuf.databuf.buf = buf;
+			pbuf.databuf.len = 0;

-				if((r = ioctl(fd,I_PEEK,&pbuf)) < 0)
-				{	if(errno == EINTR)
-						return -1;
-					t &= ~STREAM_PEEK;
-				}
-				else
-				{	t &= ~SOCKET_PEEK;
-					if(r > 0 && (r = pbuf.databuf.len) <= 0)
-					{	if(action <= 0)	/* read past eof */
-							r = sysreadf(fd,buf,1);
-						return r;
-					}
-					if(r == 0)
-						r = -1;
-					else if(r > 0)
-						break;
+			if((r = ioctl(fd,I_PEEK,&pbuf)) < 0)
+			{	if(errno == EINTR)
+					return -1;
+				t &= ~STREAM_PEEK;
+			}
+			else
+			{	t &= ~SOCKET_PEEK;
+				if(r > 0 && (r = pbuf.databuf.len) <= 0)
+				{	if(action <= 0)	/* read past eof */
+						r = sysreadf(fd,buf,1);
+					return r;
 				}
+				if(r == 0)
+					r = -1;
+				else if(r > 0)
+					break;
 			}
 		}
 #endif /* stream_peek */
@ -131,33 +119,9 @@ int	action;	/* >0: peeking, if rc>=0, get action records,
 			/* let select be interrupted instead of recv which autoresumes */
 			(t&SOCKET_PEEK) )
 		{	r = -2;
-#if _lib_poll
-			if(r == -2)
-			{
-				struct pollfd	po;
-				po.fd = fd;
-				po.events = POLLIN;
-				po.revents = 0;
-
-				if((r = SFPOLL(&po,1,tm)) < 0)
-				{	if(errno == EINTR)
-						return -1;
-					else if(errno == EAGAIN)
-					{	errno = 0;
-						continue;
-					}
-					else	r = -2;
-				}
-				else	r = (po.revents&POLLIN) ? 1 : -1;
-			}
-#endif /*_lib_poll*/
 #if _lib_select
 			if(r == -2)
-			{
-#if _hpux_threads && vt_threaded
-#define fd_set	int
-#endif
-				fd_set		rd;
+			{	fd_set		rd;
 				struct timeval	tmb, *tmp;
 				FD_ZERO(&rd);
 				FD_SET(fd,&rd);
@ -183,30 +147,6 @@ int	action;	/* >0: peeking, if rc>=0, get action records,
 #endif /*_lib_select*/
 			if(r == -2)
 			{
-#if !_lib_poll && !_lib_select	/* both poll and select can't be used */
-#ifdef FIONREAD			/* quick and dirty check for availability */
-				long	nsec = tm < 0 ? 0 : (tm+999)/1000;
-				while(nsec > 0 && r < 0)
-				{	long	avail = -1;
-					if((r = ioctl(fd,FIONREAD,&avail)) < 0)
-					{	if(errno == EINTR)
-							return -1;
-						else if(errno == EAGAIN)
-						{	errno = 0;
-							continue;
-						}
-						else	/* ioctl failed completely */
-						{	r = -2;
-							break;
-						}
-					}
-					else	r = avail <= 0 ? -1 : (ssize_t)avail;
-
-					if(r < 0 && nsec-- > 0)
-						sleep(1);
-				}
-#endif
-#endif
 			}

 			if(r > 0)		/* there is data now */
@ -223,47 +163,6 @@ int	action;	/* >0: peeking, if rc>=0, get action records,
 #if _socket_peek
 		if(t&SOCKET_PEEK)
 		{
-#if __MACH__ && __APPLE__ /* check 10.4 recv(MSG_PEEK) bug that consumes pipe data */
-			static int	recv_peek_pipe;
-			if (recv_peek_pipe == 0) /* this will be done just once */
-			{	int	fds[2], r;
-				char	tst[2];
-
-				tst[0] = 'a'; tst[1] = 'z';
-
-				/* open a pipe and write to it */
-				recv_peek_pipe = 1;
-				if(recv_peek_pipe == 1 && pipe(fds) < 0)
-					recv_peek_pipe = -1;
-				if(recv_peek_pipe == 1 && write(fds[1], tst, 2) != 2)
-					recv_peek_pipe = -1;
-
-				/* try recv() to see if it gets anything */
-				tst[0] = tst[1] = 0;
-				if(recv_peek_pipe == 1 && (r = recv(fds[0], tst, 1, MSG_PEEK)) != 1)
-					recv_peek_pipe = -1;
-				if(recv_peek_pipe == 1 && tst[0] != 'a')
-					recv_peek_pipe = -1;
-
-				/* make sure that recv() did not consume data */
-				tst[0] = tst[1] = 0;
-				if(recv_peek_pipe == 1 && (r = recv(fds[0], tst, 2, MSG_PEEK)) != 2)
-					recv_peek_pipe = -1;
-				if(recv_peek_pipe == 1 && (tst[0] != 'a' || tst[1] != 'z') )
-					recv_peek_pipe = -1;
-
-				close(fds[0]);
-				close(fds[1]);
-			}
-
-			if(recv_peek_pipe < 0)
-			{	struct stat st; /* recv should work on sockets */
-				if(fstat(fd, &st) < 0 || !S_ISSOCK(st.st_mode) )
-				{	r = -1;
-					t &= ~SOCKET_PEEK;
-				}
-			}
-#endif
 			while((t&SOCKET_PEEK) && (r = recv(fd,(char*)buf,n,MSG_PEEK)) < 0)
 			{	if(errno == EINTR)
 					return -1;