Re: PATCH: improve checkptycmd

[Mikael Magnusson <mikachu@xxxxxxxxx>]

On Mon, Jul 6, 2026 at 8:44 PM Mikael Magnusson <mikachu@xxxxxxxxx> wrote:
>
> On Mon, Jul 6, 2026 at 7:39 PM Jun T <takimoto-j@xxxxxxxxxxxxxxxxx> wrote:
> >
> > With this patch (commit 4b4ebcca) lots of tests fails on macOS
> > (probably all the tests that use zpty). It seems reading from zpty
> > returns nothing.
> >
> > On macOS, HAVE_POLL is not defined because zsh_system.h:374-381 has
> >
> > #if defined(__APPLE__) && defined(HAVE_SELECT)
> > /*
> >  * Prefer select() to poll() on MacOS X since poll() is known
> >  * to be problematic in 10.4
> >  */
> > #undef HAVE_POLL
> > #undef HAVE_POLL_H
> > #endif
> >
> >
> > In function checkptycmd(), at zpty.c:576
> >         ret = select(cmd->fd + 1, (SELECT_ARG_2_T) &fds, NULL, NULL, &tv);
> > returns ret=1 (and select_ret is set to 1), but at line 583
> >     if (ioctl(cmd->fd, FIONREAD, (char *) &val) == 0 && val > 0)
> > returns val = 0, and the pty is closed at line 588.
> >
> > I don't know select()/ioctl() are working as expected or not.
> >
> > But, if I remove the lines 374-381 in zsh_system.h, i.e., if we use poll()
> > also on macOS, then all the test pass, at least on the latest macOS (Sequoia).
> >
> > Version 10.4 mentioned in the comment is Tiger, which is released in 2005,
> > and not supported since 2009, probably.
> >
> > If there are someone who have older macOS, please test by removing the
> > lines 374-381 in zsh_system.h.
>
> Hmm, I didn't look at the code again, but the idea was that even if
> one of these things is not defined, the code should still work at
> least as well as before. I'll take another look at it a bit later.

dana added some debug prints and it seems like FIONREAD doesn't work
on OSX when the other end of the pty is closed, which seems very
unhelpful of it. If we just re-enable HAVE_POLL there, I guess there's
no reason to keep this path anyway. It does work fine on Linux but I'm
not sure how to verify which OSes it does work on. The safest choice
is probably to just remove it. In theory we could add some fallback
that does a non-blocking read but I'm not sure if that's worth it.

-- 
Mikael Magnusson

From 4e1b808c491571f4fb7bfd134652ed5fc53c69a7 Mon Sep 17 00:00:00 2001
From: Mikael Magnusson <mikachu@xxxxxxxxx>
Date: Thu, 9 Jul 2026 01:36:02 +0200
Subject: PATCH: checkptycmd: remove select()+FIONREAD variant, re-enable poll
 on osx

---
 Src/Modules/zpty.c | 33 ---------------------------------
 Src/zsh_system.h   |  9 ---------
 2 files changed, 42 deletions(-)

diff --git a/Src/Modules/zpty.c b/Src/Modules/zpty.c
index 2f9b80f40c..1d381cdc8e 100644
--- a/Src/Modules/zpty.c
+++ b/Src/Modules/zpty.c
@@ -555,39 +555,6 @@ checkptycmd(Ptycmd cmd)
     cmd->fin = 1;
     zclose(cmd->fd);
 }
-#elif defined(FIONREAD)
-static void
-checkptycmd(Ptycmd cmd)
-{
-    int val = 0, select_ret = 0;
-
-    if (cmd->fin)
-        return;
-#ifdef HAVE_SELECT
-    {
-	fd_set fds;
-	struct timeval tv;
-	int ret;
-
-	FD_ZERO(&fds);
-	FD_SET(cmd->fd, &fds);
-	tv.tv_sec = 0;
-	tv.tv_usec = 0;
-	ret = select(cmd->fd + 1, (SELECT_ARG_2_T) &fds, NULL, NULL, &tv);
-	if (ret > 0 && FD_ISSET(cmd->fd, &fds)) {
-	    /* either there are bytes, or the process exited */
-	    select_ret = 1;
-	}
-    }
-#endif
-    if (ioctl(cmd->fd, FIONREAD, (char *) &val) == 0 && val > 0)
-        return;  /* data available, not finished */
-    /* No data (or ioctl failed): check if process is dead */
-    if (select_ret || kill(cmd->pid, 0) < 0) {
-        cmd->fin = 1;
-        zclose(cmd->fd);
-    }
-}
 #else
 static void
 checkptycmd(Ptycmd cmd)
diff --git a/Src/zsh_system.h b/Src/zsh_system.h
index 21446a9b18..d46135e10c 100644
--- a/Src/zsh_system.h
+++ b/Src/zsh_system.h
@@ -371,15 +371,6 @@ struct timespec {
 # endif
 #endif
 
-#if defined(__APPLE__) && defined(HAVE_SELECT)
-/*
- * Prefer select() to poll() on MacOS X since poll() is known
- * to be problematic in 10.4
- */
-#undef HAVE_POLL
-#undef HAVE_POLL_H
-#endif
-
 #ifdef HAVE_SYS_FILIO_H
 # include <sys/filio.h>
 #endif
-- 
2.38.1