Re: precmd: write error: interrupted

Re: precmd: write error: interrupted

[Bart Schaefer]

(Redirecting this to -workers, so Yuri Cc'd in case he's only on -users)

On Apr 25,  9:38pm, Yuri D'Elia wrote:
}
} precmd() { { print x } 2>/dev/null }
} 
} still doesn't suppress the error.

Interesting!  Something is restoring the stderr descriptors before the
error message in bin_print is written.  Here is a bit of strace from
'zsh -f' with { print -n $'x\r' } 2>/devnull running in a loop on the
command line (not from a precmd):

open("/dev/null", O_WRONLY|O_CREAT|O_NOCTTY|O_TRUNC|O_LARGEFILE, 0666) = 3
dup2(3, 2)                              = 2
write(1, "x\r", 2)                      = ? ERESTARTSYS (To be restarted)
--- SIGWINCH (Window changed) @ 0 (0) ---
dup2(12, 2)                             = 2
dup2(11, 2)                             = 2
write(10, "\33[1m\33[7m%\33[27m\33[1m\33[0m                                     
                                           \r \r", 105) = 105
write(2, "print: write error: interrupt\n", 30) = 30

Those dup2 calls should happen only at the end of execcmd after bin_print
has already returned, but somehow they appear to be happening after the
signal handler is called but before the error message.

Attaching with a debugger blocks the interrupt so I haven't been able to
stack-trace the source of the dup2 calls.

Re: precmd: write error: interrupted

[Frank Terbeck]

Bart Schaefer wrote:
> On Sun, May 5, 2013 at 2:38 AM, Frank Terbeck <ft@bewatermyfriend.org>wrote:
>> I've pushed this, because it's definitely an improvement. I tried
>> looking for the right position to unblock SIGWINCH for the `exec' case
>> in exec.c, but couldn't find it yet. Maybe someone has a more specific
>> idea than I do?
>
>  As far as I can tell, there are six calls to execve() in Src/exec.c.  If a
> winch_unblock() were placed just before each of those six calls, the
> unblock in zfork() from that last patch could probably be reverted.

That sounds like a plan. And my test cases seem to work fine, so I just
pushed those two changes.


Regards, Frank

Re: precmd: write error: interrupted

[Bart Schaefer]

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

On Sun, May 5, 2013 at 2:38 AM, Frank Terbeck <ft@bewatermyfriend.org>wrote:

> I've pushed this, because it's definitely an improvement. I tried
> looking for the right position to unblock SIGWINCH for the `exec' case
> in exec.c, but couldn't find it yet. Maybe someone has a more specific
> idea than I do?
>

 As far as I can tell, there are six calls to execve() in Src/exec.c.  If a
winch_unblock() were placed just before each of those six calls, the
unblock in zfork() from that last patch could probably be reverted.

Re: precmd: write error: interrupted

[Frank Terbeck]

Bart Schaefer wrote:
> On Sat, May 4, 2013 at 5:01 PM, Frank Terbeck <ft@bewatermyfriend.org>wrote:
>> I'm afraid these changes break SIGWINCH handling of child processes.
>
> Not entirely surprising.  I wondered about that but tried it with a couple
> of apps before posting and they seemed to be responding to the change in
> size.  I didn't actually test whether they were receiving WINCH, though.
>
> The below should help, but someone will have to commit it for me, because

It does help with normal children (ie. the ones the shell fork()s for).
But it doesn't fix cases, in which you use `exec' to replace the shell
process with the child process.

> the computer with my sourceforge ssh keys suffered a video chip meltdown
> yesterday and I haven't had a chance to repair it or pull the key file from
> backup yet.  For the same reason, I'm composing this in gmail, so I hope
> the patch has not been mangled.

It was mangled, but is was short enough to apply manually.

I've pushed this, because it's definitely an improvement. I tried
looking for the right position to unblock SIGWINCH for the `exec' case
in exec.c, but couldn't find it yet. Maybe someone has a more specific
idea than I do?


Regards, Frank

Re: precmd: write error: interrupted

[Bart Schaefer]

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

On Sat, May 4, 2013 at 5:01 PM, Frank Terbeck <ft@bewatermyfriend.org>wrote:

> Hi Bart,
>
> I'm afraid these changes break SIGWINCH handling of child processes.
>
>
Not entirely surprising.  I wondered about that but tried it with a couple
of apps before posting and they seemed to be responding to the change in
size.  I didn't actually test whether they were receiving WINCH, though.

The below should help, but someone will have to commit it for me, because
the computer with my sourceforge ssh keys suffered a video chip meltdown
yesterday and I haven't had a chance to repair it or pull the key file from
backup yet.  For the same reason, I'm composing this in gmail, so I hope
the patch has not been mangled.

Something similar may also be needed in the zpty and clone modules.  (Has
anyone ever used the clone module for anything?)

diff --git a/Src/exec.c b/Src/exec.c
index fa14875..c71f3f3 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -303,11 +303,13 @@ zfork(struct timeval *tv)
  zerr("fork failed: %e", errno);
  return -1;
     }
+    if (!pid) {
+ winch_unblock();
 #ifdef HAVE_GETRLIMIT
-    if (!pid)
  /* set resource limits for the child process */
  setlimits(NULL);
 #endif
+    }
     return pid;
 }

Re: precmd: write error: interrupted

[Frank Terbeck]

Hi Bart,

Bart Schaefer wrote:
[...]
> Here's a stab at a more comprehensive patch.  This follows the pattern of
> child_block()/child_unblock() which we use elsewhere to reap children only
> when safe to do so.
>
> I'm not 100% confident that this does the right thing for non-interactive
> shells/sourcing of scripts, but I *think* the change in shingetline()
> should cover those cases.

I'm afraid these changes break SIGWINCH handling of child processes.

If you start an editor or mail-reader or terminal-multiplexer (or
whatever really), that child will not react to any terminal-resizes
anymore.

It took a while to find out, why this:

  TRAPWINCH() { print $COLUMNS $LINES; }

didn't trigger in shells inside the terminal-multiplexer I use, but does
trigger in a naked xterm without multiplexer. The fact that the terminal
multiplexer itself didn't react to SIGWINCHes anymore steered me into
zsh's direction (well, not directly but eventually).

Building a version of the shell before these changes¹ makes everything
work again.  Sorry to be the bearer of bad news. ;)


Regards, Frank

¹ * cbf6f14 <Jun T> 31357: _cp: add support for Mac OS X (5 days ago)

Re: precmd: write error: interrupted

[Bart Schaefer]

On Apr 28,  6:03pm, Bart Schaefer wrote:
}
} Here's a stab at a more comprehensive patch.  This follows the pattern of
} child_block()/child_unblock() which we use elsewhere to reap children only
} when safe to do so.

Incidentally if you're simultaneously doing something like scrolling
around with arrow keys with your left hand while resizing the window
with your right hand, the shell is probably going to misinterpret some 
of your keystrokes, even with this patch.

Re: precmd: write error: interrupted

[Bart Schaefer]

On Apr 28,  5:30pm, Yuri D'Elia wrote:
} Subject: Re: precmd: write error: interrupted
}
} > +    signal_block(signal_mask(SIGWINCH));  /* See zleread() */
} >       callhookfunc("precmd", NULL, 1, NULL);
} > +    signal_unblock(signal_mask(SIGWINCH));
} 
} This is going to solve the issue for "precmd" only though, not the 
} SIGWHINCH issue as a whole.

True; I was trying to creep up on it incrementally.

Incidentally even queue_signals() doesn't actually block the signals, it
just doesn't call anything below the top-level handler, so I suspect that
replacing the above with queue_signals() would still show the bug.

Here's a stab at a more comprehensive patch.  This follows the pattern of
child_block()/child_unblock() which we use elsewhere to reap children only
when safe to do so.

I'm not 100% confident that this does the right thing for non-interactive
shells/sourcing of scripts, but I *think* the change in shingetline()
should cover those cases.

diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 9a4265f..569ad5f 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -567,7 +567,9 @@ raw_getbyte(long do_keytmout, char *cptr)
 	gettyinfo(&ti);
 	ti.tio.c_cc[VMIN] = 0;
 	settyinfo(&ti);
+	winch_unblock();
 	ret = read(SHTTY, cptr, 1);
+	winch_block();
 	ti.tio.c_cc[VMIN] = 1;
 	settyinfo(&ti);
 	if (ret > 0)
@@ -597,7 +599,9 @@ raw_getbyte(long do_keytmout, char *cptr)
 	    else
 		poll_timeout = -1;
 
+	    winch_unblock();
 	    selret = poll(fds, errtry ? 1 : nfds, poll_timeout);
+	    winch_block();
 # else
 	    int fdmax = SHTTY;
 	    struct timeval *tvptr;
@@ -622,8 +626,10 @@ raw_getbyte(long do_keytmout, char *cptr)
 	    else
 		tvptr = NULL;
 
+	    winch_unblock();
 	    selret = select(fdmax+1, (SELECT_ARG_2_T) & foofd,
 			    NULL, NULL, tvptr);
+	    winch_block();
 # endif
 	    /*
 	     * Make sure a user interrupt gets passed on straight away.
@@ -788,7 +794,9 @@ raw_getbyte(long do_keytmout, char *cptr)
 #  else
 	ioctl(SHTTY, TCSETA, &ti.tio);
 #  endif
+	winch_unblock();
 	ret = read(SHTTY, cptr, 1);
+	winch_block();
 #  ifdef HAVE_TERMIOS_H
 	tcsetattr(SHTTY, TCSANOW, &shttyinfo.tio);
 #  else
@@ -799,7 +807,9 @@ raw_getbyte(long do_keytmout, char *cptr)
 #endif
     }
 
+    winch_unblock();
     ret = read(SHTTY, cptr, 1);
+    winch_block();
 
     return ret;
 }
diff --git a/Src/init.c b/Src/init.c
index 8467a73..f4fb3be 100644
--- a/Src/init.c
+++ b/Src/init.c
@@ -1113,6 +1113,7 @@ init_signals(void)
     install_handler(SIGCHLD);
 #ifdef SIGWINCH
     install_handler(SIGWINCH);
+    winch_block();	/* See utils.c:preprompt() */
 #endif
     if (interact) {
 	install_handler(SIGALRM);
diff --git a/Src/input.c b/Src/input.c
index 5cff22d..9bd9663 100644
--- a/Src/input.c
+++ b/Src/input.c
@@ -143,10 +143,12 @@ shingetline(void)
 
     p = buf;
     for (;;) {
+	winch_unblock();
 	do {
 	    errno = 0;
 	    c = fgetc(bshin);
 	} while (c < 0 && errno == EINTR);
+	winch_block();
 	if (c < 0 || c == '\n') {
 	    if (c == '\n')
 		*p++ = '\n';
diff --git a/Src/signals.h b/Src/signals.h
index 9541a1a..d680968 100644
--- a/Src/signals.h
+++ b/Src/signals.h
@@ -59,6 +59,14 @@
 #define child_block()      signal_block(sigchld_mask)
 #define child_unblock()    signal_unblock(sigchld_mask)
 
+#ifdef SIGWINCH
+# define winch_block()      signal_block(signal_mask(SIGWINCH))
+# define winch_unblock()    signal_unblock(signal_mask(SIGWINCH))
+#else
+# define winch_block()      0
+# define winch_unblock()    0
+#endif
+
 /* ignore a signal */
 #define signal_ignore(S)   signal(S, SIG_IGN)
 
diff --git a/Src/utils.c b/Src/utils.c
index 26e2a5c..94ae522 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -1291,6 +1291,13 @@ preprompt(void)
     int period = getiparam("PERIOD");
     int mailcheck = getiparam("MAILCHECK");
 
+    /*
+     * Handle any pending window size changes before we compute prompts,
+     * then block them again to avoid interrupts during prompt display.
+     */
+    winch_unblock();
+    winch_block();
+
     if (isset(PROMPTSP) && isset(PROMPTCR) && !use_exit_printed && shout) {
 	/* The PROMPT_SP heuristic will move the prompt down to a new line
 	 * if there was any dangling output on the line (assuming the terminal
-- 
1.7.9.6 (Apple Git-31.1)

Re: precmd: write error: interrupted

[Yuri D'Elia]

On 04/28/2013 12:31 AM, Bart Schaefer wrote:
> What you've been reporting is almost certainly another variation of the
> same bug.  So maybe all we really need is this (plus some #ifdef):
>
> diff --git a/Src/utils.c b/Src/utils.c
> index 26e2a5c..a20a5e1 100644
> --- a/Src/utils.c
> +++ b/Src/utils.c
> @@ -1323,7 +1323,9 @@ preprompt(void)
>
>       /* If a shell function named "precmd" exists, *
>        * then execute it.                           */
> +    signal_block(signal_mask(SIGWINCH));  /* See zleread() */
>       callhookfunc("precmd", NULL, 1, NULL);
> +    signal_unblock(signal_mask(SIGWINCH));
>       if (errflag)
>          return;

This is going to solve the issue for "precmd" only though, not the 
SIGWHINCH issue as a whole.

You can still arbitrarily stop the example loop I shown before by 
resizing the terminal window.

Re: precmd: write error: interrupted

[Bart Schaefer]

[Cc'ing -workers]

On Apr 27,  7:42pm, Yuri D'Elia wrote:
} Subject: Re: precmd: write error: interrupted
}
} Just for my information, is there something I should do for these 2 
} issues (redirection and SIGWINCH); ie: file a bug report somewhere so it 
} doesn't get forgotten?

The mailing list is the place to file bug reports, so for the moment you
have done as much as you can ...

} I've seen this error for too long to let it go :)

The WINCH bug was tickling a memory so I went looking through the source
for mentions of SIGWINCH and found this in zle_main.c:zlread():

    /*
     * On some windowing systems we may enter this function before the
     * terminal is fully opened and sized, resulting in an infinite
     * series of SIGWINCH when the handler prints the prompt before we
     * have done so here.  Therefore, hold any such signal until the
     * first full refresh has completed.  The important bit is that the
     * handler must not see zleactive = 1 until ZLE really is active.
     * See the end of adjustwinsize() in Src/utils.c
     */
    queue_signals();

What you've been reporting is almost certainly another variation of the
same bug.  So maybe all we really need is this (plus some #ifdef):

diff --git a/Src/utils.c b/Src/utils.c
index 26e2a5c..a20a5e1 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -1323,7 +1323,9 @@ preprompt(void)
 
     /* If a shell function named "precmd" exists, *
      * then execute it.                           */
+    signal_block(signal_mask(SIGWINCH));  /* See zleread() */
     callhookfunc("precmd", NULL, 1, NULL);
+    signal_unblock(signal_mask(SIGWINCH));
     if (errflag)
        return;
 
There are a couple of other things in preprompt() that might want this
kind of wrapper too, e.g. periodic and the prepromptfns array, but we
can start with checking if the above is helpful.

Re: precmd: write error: interrupted

[Yuri D'Elia]

On 04/26/2013 05:08 PM, Bart Schaefer wrote:
> } What other syscalls would be interrupted by SIGWINCH that shouldn't be
> } restarted? Right now I cannot think of anything that SIGWINCH should
> } interrupt.
>
> I've been thinking about this, and the problem with using SA_RESTART is
> twofold:
>
> (1) [Minor] Some platforms don't have restartable syscalls, so this won't
> work everywhere.  But perhaps the intersection of non-restarable syscalls
> and support for SIGWINCH is empty.

(I'm somehow curious of which systems don't support SIGWINCH, must be 
particularly old).

> (2) [Potentially major] A user-defined trap can be installed for the
> SIGWINCH signal.  That means arbitrary shell code might execute during
> handling of the signal, so all sorts of things might happen mid-write,
> not just the default ioctls.

I see, and now I also see your reasoning about queuing SIGWINCH 
everywhere except when waiting in zleread. At any rate, that's the only 
point where updating the terminal for the upcoming output makes sense. 
Updating the column count (for example) while in the middle of a widget 
expansion for instance won't likely help (or maybe even break some 
invariant).

Re: precmd: write error: interrupted

[Peter Stephenson]

On Thu, 25 Apr 2013 15:18:39 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Consider something like:
> 
>     x=({1..1000000}
>     print $x
> 
> If you can't ^C that print, you're potentially in a world of pain.  (It's
> already enough of a problem that you can't ^C the assignment itself.)

(I moved this bit to zsh-workers.)

OK, so what I'm missing is that while the shell code might whizz through
there could be long delays in the backend outputting it to the terminal,
during which the shell is still executing bin_print() (and that's still
true even given your later remarks about queueing).

> I'm pretty sure SIGWINCH is an outlier case here and we should focus on
> the question of when the shell SHOULD react to window size changes,
> rather trying to identify all the builtins that should NOT react.
> 
> For example, we might *always* queue the SIGWINCH signal except when the
> shell is blocked in zleread (or is about to, but hasn't yet, displayed
> the prompt if ZLE is not active).  Those probably don't cover all the
> cases, but you get the idea.

I can see that it's an infrequent operation that's there to fix things
up for the user, i.e. needs handling on the timescale of human reactions
rather than, say, the timescale of Unix processes.  So yes, it looks
plausible it could be handled differently.

I suppose if a foreground process is running, it has to be done as at
present: the process gets the signal and zsh handles it when it gets
back control (but my knowledge of process groups is a bit basic).

pws