bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Michael Heerdegen]

Hello,

in emacs -nw -Q eval in any way

  (while-no-input (sit-for 20))

and hit any key before the sit-for terminates.  This always gives me a
Quit.  That should not happen.

FWIW I think I got rare quits in a similar situation also in graphical
Emacs, but I'm not sure if it's the same issue.


TIA,

Michael.


In GNU Emacs 27.0.50 (build 59, x86_64-pc-linux-gnu, GTK+ Version 3.24.5)
 of 2019-02-16 built on drachen
Repository revision: 5cae45273f019ed93c59c1b9b4655328ce2a0571
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
System Description: Debian GNU/Linux buster/sid

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Mon, 18 Feb 2019 23:29:04 +0100
> 
> in emacs -nw -Q eval in any way
> 
>   (while-no-input (sit-for 20))
> 
> and hit any key before the sit-for terminates.  This always gives me a
> Quit.  That should not happen.

On the emacs-26 branch, we have bug#31692, so this cannot work
correctly there.  That bug is fixed on master, though.

What happens on master is this: when sit-for is called inside
while-no-input on a TTY, it returns the cons cell (t . ?\C-g), which
then causes keyboard quit right after while-no-input returns.

The below seems to fix the problem, but I hope you also have a more
complex real-life use case, because I'm not really sure this solution
has no adverse effects elsewhere.

Stefan, any thoughts?

diff --git a/src/keyboard.c b/src/keyboard.c
index 1d67c3e..4448e2b 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2551,7 +2551,10 @@ read_char (int commandflag, Lisp_Object map,
       restore_getcjmp (save_jump);
       pthread_sigmask (SIG_SETMASK, &empty_mask, 0);
       unbind_to (jmpcount, Qnil);
-      XSETINT (c, quit_char);
+      /* If we are in while-no-input, don't trigger C-g, as that will
+	 quit instead of letting while-no-input do its thing.  */
+      if (!EQ (Vquit_flag, Vthrow_on_input))
+	XSETINT (c, quit_char);
       internal_last_event_frame = selected_frame;
       Vlast_event_frame = internal_last_event_frame;
       /* If we report the quit char as an event,

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Eli Zaretskii]

> Date: Fri, 22 Feb 2019 18:08:58 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 34535@debbugs.gnu.org
> 
> What happens on master is this: when sit-for is called inside
> while-no-input on a TTY, it returns the cons cell (t . ?\C-g), which
> then causes keyboard quit right after while-no-input returns.

I forgot to tell that sit-fore returns that cons cell because
read-event returns ?\C-g, regardless of what you type, when it is
called inside while-no-input.

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Eli Zaretskii]

> Date: Fri, 22 Feb 2019 22:09:29 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 34535@debbugs.gnu.org
> 
> > Date: Fri, 22 Feb 2019 18:08:58 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: 34535@debbugs.gnu.org
> > 
> > What happens on master is this: when sit-for is called inside
> > while-no-input on a TTY, it returns the cons cell (t . ?\C-g), which
> > then causes keyboard quit right after while-no-input returns.
> 
> I forgot to tell that sit-fore returns that cons cell because
> read-event returns ?\C-g, regardless of what you type, when it is
> called inside while-no-input.

To be even more accurate: sit-for pushes that cons cell onto
unread-command-events, and then the command loop reads it and triggers
keyboard quit.

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Michael Heerdegen]

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

Eli Zaretskii <eliz@gnu.org> writes:

> The below seems to fix the problem, but I hope you also have a more
> complex real-life use case, because I'm not really sure this solution
> has no adverse effects elsewhere.

My use case was like this:


[-- Attachment #2: test.el --]
[-- Type: application/emacs-lisp, Size: 1705 bytes --]

[-- Attachment #3: Type: text/plain, Size: 418 bytes --]


This seems fixed for me with your patch.

In real real life, I made it so that while-no-input only covers the
computation needed for the minibuffer hint (counting matches) which must
be interruptible, but not the sit-for used to delay the minibuffer hint
being removed.  That sit-for was inside while-no-input originally was a
matter of laziness, I was able to rewrite it to circumvent the issue.


Thanks,

Michael.

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  34535@debbugs.gnu.org
> Date: Sat, 23 Feb 2019 05:27:36 +0100
> 
> This seems fixed for me with your patch.

Thanks for testing.  I'll push the change in a few days, if nothing
pops up to the contrary.

> In real real life, I made it so that while-no-input only covers the
> computation needed for the minibuffer hint (counting matches) which must
> be interruptible, but not the sit-for used to delay the minibuffer hint
> being removed.  That sit-for was inside while-no-input originally was a
> matter of laziness, I was able to rewrite it to circumvent the issue.

Please note that using while-no-input with BODY that reads some input
in a way causes indeterminate results, because whether the arriving
input will be read and consumed by BODY and/or used to throw control
flow out of BODY, depends on seemingly random factors, like whether
Emacs checks quit-flag before or after BODY consumes the input and
acts upon it.

For example, in your test case:

  (while-no-input (sit-for 20))

there's a conceptual difficulty in deciding whether pressing a key
during the 20-sec wait should return nil or t.  On the one hand,
arriving input should end BODY and return t, but OTOH that same input
causes sit-for to stop waiting, so BODY finishes, and while-no-input
should return what sit-for returned, i.e. nil.  What actually happens
depends on the underlying implementation, which is sometimes
platform-dependent, and on what a more complex BODY does.

IOW, I think while-no-input does not (and cannot) reliably produce
deterministic output in the case where BODY itself reads input.  We
try to DTRT, but there can be no general guarantees in these cases.

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Stefan Monnier]

> IOW, I think while-no-input does not (and cannot) reliably produce
> deterministic output in the case where BODY itself reads input.  We
> try to DTRT, but there can be no general guarantees in these cases.

Thanks, Eli.  Indeed, "TRT" is not even clear here.
Some things are clearly not right, but if we look a bit further it
quickly gets murky.

Regarding your patch.  I'm not sure it's "TRT" but given the murky/messy
context it looks good to me.  Hopefully at some point someone can sit
down and think through the whole way we handle "quit" and try to clean
it up.


        Stefan

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Michael Heerdegen <michael_heerdegen@web.de>, 34535@debbugs.gnu.org
> Date: Sat, 23 Feb 2019 10:13:21 -0500
> 
> Regarding your patch.  I'm not sure it's "TRT" but given the murky/messy
> context it looks good to me.  Hopefully at some point someone can sit
> down and think through the whole way we handle "quit" and try to clean
> it up.

OK, thanks for the feedback.  I will wait a few days before pushing.

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Please note that using while-no-input with BODY that reads some input
> in a way causes indeterminate results, because whether the arriving
> input will be read and consumed by BODY and/or used to throw control
> flow out of BODY, depends on seemingly random factors, like whether
> Emacs checks quit-flag before or after BODY consumes the input and
> acts upon it.

I hope this is not the case in my code.  In the w-n-i BODY I perform a
computation, but as in the skeleton I posted, the BODY itself doesn't
read input.  The w-n-i with the computation is done in a function in
'minibuffer-setup-hook'.  Is that allowed?


Thanks,

Michael.

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: monnier@iro.umontreal.ca,  34535@debbugs.gnu.org
> Date: Sun, 24 Feb 2019 08:16:51 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Please note that using while-no-input with BODY that reads some input
> > in a way causes indeterminate results, because whether the arriving
> > input will be read and consumed by BODY and/or used to throw control
> > flow out of BODY, depends on seemingly random factors, like whether
> > Emacs checks quit-flag before or after BODY consumes the input and
> > acts upon it.
> 
> I hope this is not the case in my code.  In the w-n-i BODY I perform a
> computation, but as in the skeleton I posted, the BODY itself doesn't
> read input.  The w-n-i with the computation is done in a function in
> 'minibuffer-setup-hook'.  Is that allowed?

Your description indicates that in your case BODY doesn't by itself
read input, so you should be OK, I think.

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks for testing.  I'll push the change in a few days, if nothing
> pops up to the contrary.

Dunno if this is related, but I think it started happening at the time I
installed your patch: When I wake up my Laptop from suspend-to-ram,
Emacs uses 100% CPU and it's hard to interrupt it (C-g doesn't help).  I
sent a SIGUSR2 and got this backtrace:

Debugger entered--entering a function:
* command-error-default-function((quit) "" nil)

Michael.

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: monnier@iro.umontreal.ca,  34535@debbugs.gnu.org
> Date: Mon, 25 Feb 2019 08:09:27 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks for testing.  I'll push the change in a few days, if nothing
> > pops up to the contrary.
> 
> Dunno if this is related, but I think it started happening at the time I
> installed your patch: When I wake up my Laptop from suspend-to-ram,
> Emacs uses 100% CPU and it's hard to interrupt it (C-g doesn't
> help).

The only way to be sure is to back out the patch and see if these
problems go away.

> I sent a SIGUSR2 and got this backtrace:
> 
> Debugger entered--entering a function:
> * command-error-default-function((quit) "" nil)

This isn't informative enough, I need a C-level backtrace plus a full
Lisp backtrace.  Please next time attach a debugger when this happens,
source Emacs's src/.gdbinit, and type "thread apply all bt" at GDB
prompt.

Also, I don't think I understand how you got the above Lisp backtrace:
by default SIGUSR2 does nothing.  Do you have some handler for it? if
so, what does it do?

Thanks.

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> This isn't informative enough, I need a C-level backtrace plus a full
> Lisp backtrace.  Please next time attach a debugger when this happens,
> source Emacs's src/.gdbinit, and type "thread apply all bt" at GDB
> prompt.

Ok, I'll do that.

> Also, I don't think I understand how you got the above Lisp backtrace:
> by default SIGUSR2 does nothing.  Do you have some handler for it? if
> so, what does it do?

No, I only use the default behavior:

 -- User Option: debug-on-event
     If you set ‘debug-on-event’ to a special event (*note Special
     Events::), Emacs will try to enter the debugger as soon as it
     receives this event, bypassing ‘special-event-map’.  At present,
     the only supported values correspond to the signals ‘SIGUSR1’ and
     ‘SIGUSR2’ (this is the default).  This can be helpful when
     ‘inhibit-quit’ is set and Emacs is not otherwise responding.

(Why do you think SIGUSR2 would do nothing?)


Michael.

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: monnier@iro.umontreal.ca,  34535@debbugs.gnu.org
> Date: Tue, 26 Feb 2019 10:56:06 +0100
> 
> (Why do you think SIGUSR2 would do nothing?)

Sorry, my bad: I looked in the wrong place to see the default binding
of the event.

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> > This isn't informative enough, I need a C-level backtrace plus a full
> > Lisp backtrace.  Please next time attach a debugger when this happens,
> > source Emacs's src/.gdbinit, and type "thread apply all bt" at GDB
> > prompt.
>
> Ok, I'll do that.

This seems only to happen after several hours of suspended ram sleep.  I
just waited this time to see if it ends, and after ~10 seconds, CPU
usage went to nearly zero and everything was good.  I'm not sure if
Emacs is to blame for anything, though I didn't see this before.  When
it happens the next time, I'll have a look if other programs are
responding.

Michael.

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: monnier@iro.umontreal.ca,  34535@debbugs.gnu.org
> Date: Tue, 26 Feb 2019 18:27:33 +0100
> 
> This seems only to happen after several hours of suspended ram sleep.  I
> just waited this time to see if it ends, and after ~10 seconds, CPU
> usage went to nearly zero and everything was good.  I'm not sure if
> Emacs is to blame for anything, though I didn't see this before.  When
> it happens the next time, I'll have a look if other programs are
> responding.

OK, thanks.  I guess I will soon push the change after all.

bug#34535: 27.0.50; emacs -nw: while-no-input + sit-for + <KEY> => Quit

[Eli Zaretskii]

> Date: Tue, 26 Feb 2019 20:27:55 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: monnier@iro.umontreal.ca, 34535@debbugs.gnu.org
> 
> OK, thanks.  I guess I will soon push the change after all.

Done on the master branch.