From 9ba2c2e0dfadd78b1688161e4c4feb072a6d3447 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Wed, 19 Aug 2020 21:58:46 +0100 Subject: [PATCH] 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 --- NEWS | 5 + src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/tests/io.sh | 14 +++ src/lib/libast/sfio/sfpkrd.c | 175 +++++++------------------------- 4 files changed, 57 insertions(+), 139 deletions(-) diff --git a/NEWS b/NEWS index b6fdf57da..0348b5174 100644 --- a/NEWS +++ b/NEWS @@ -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) diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index 8899cd5ca..9d19bb453 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -17,4 +17,4 @@ * David Korn * * * ***********************************************************************/ -#define SH_RELEASE "93u+m 2020-08-17" +#define SH_RELEASE "93u+m 2020-08-19" diff --git a/src/cmd/ksh93/tests/io.sh b/src/cmd/ksh93/tests/io.sh index 2dfd036e9..cf37d0ce5 100755 --- 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)) diff --git a/src/lib/libast/sfio/sfpkrd.c b/src/lib/libast/sfio/sfpkrd.c index 2572e6dc6..7a950b3b1 100644 --- 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 + +/* 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 .) +#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;