[PATCH] Re-enable SIGIO when waiting for events

[PATCH] Re-enable SIGIO when waiting for events

[Mike Crowe]

Pasting a large string into an Emacs tty frame causes
keyboard.c:kbd_buffer_store_buffered_event to call ignore_sigio()
which sets SIGIO to SIG_IGN.

Unfortunately no-one seems to ever re-enable the SIGIO handler.

This leads to timeouts when attempting to paste into an Emacs X11
frame as reported at
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16737 .

It appears that keyboard.c:kbd_buffer_get_event used to re-enable the
signal handler but that was removed as part of a large signal-handling
clean up in 4d7e6e51dd4acecff466a28d958c50f34fc130b8.

Reinstating the re-enabling of SIGIO in kbd_buffer_get_event solves
the problems reported in bug 16737 for me.

* src/keyboard.c (kbd_buffer_get_event): Re-enable SIGIO when waiting
  for event.
---
 src/keyboard.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/keyboard.c b/src/keyboard.c
index c5a392f..e710d5a 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -316,6 +316,8 @@ static Lisp_Object command_loop (void);
 static void echo_now (void);
 static ptrdiff_t echo_length (void);
 
+static void deliver_input_available_signal (int sig);
+
 /* Incremented whenever a timer is run.  */
 unsigned timers_run;
 
@@ -3849,6 +3851,14 @@ kbd_buffer_get_event (KBOARD **kbp,
       /* Start reading input again because we have processed enough to
          be able to accept new events again.  */
       unhold_keyboard_input ();
+#ifdef USABLE_SIGIO
+      if (!noninteractive)
+       {
+         struct sigaction action;
+         emacs_sigaction_init (&action, deliver_input_available_signal);
+         sigaction (SIGIO, &action, 0);
+       }
+#endif
       start_polling ();
     }
 #endif	/* subprocesses */
-- 
2.1.4

Re: [PATCH] Re-enable SIGIO when waiting for events

[Alex Bennée]

Mike Crowe <mac@mcrowe.com> writes:

> Pasting a large string into an Emacs tty frame causes
> keyboard.c:kbd_buffer_store_buffered_event to call ignore_sigio()
> which sets SIGIO to SIG_IGN.
>
> Unfortunately no-one seems to ever re-enable the SIGIO handler.
>
> This leads to timeouts when attempting to paste into an Emacs X11
> frame as reported at
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=16737 .
>
> It appears that keyboard.c:kbd_buffer_get_event used to re-enable the
> signal handler but that was removed as part of a large signal-handling
> clean up in 4d7e6e51dd4acecff466a28d958c50f34fc130b8.
>
> Reinstating the re-enabling of SIGIO in kbd_buffer_get_event solves
> the problems reported in bug 16737 for me.

I've patched my local emacs-24 branch and will give it a spin to see if
the problem still occurs. Thanks!

-- 
Alex Bennée

Re: [PATCH] Re-enable SIGIO when waiting for events

[Tassilo Horn]

Alex Bennée <alex.bennee@linaro.org> writes:

>> Reinstating the re-enabling of SIGIO in kbd_buffer_get_event solves
>> the problems reported in bug 16737 for me.
>
> I've patched my local emacs-24 branch and will give it a spin to see
> if the problem still occurs. Thanks!

I'm using it since yesterday and didn't see that issue anymore although
I've done some heavy c&p-ing between my browser and emacs which used to
trigger the problem with a very high likelyhood.

Bye,
Tassilo

Re: [PATCH] Re-enable SIGIO when waiting for events

[Paul Eggert]

Tassilo Horn wrote:

> I'm using it since yesterday and didn't see that issue anymore although
> I've done some heavy c&p-ing between my browser and emacs which used to
> trigger the problem with a very high likelyhood.

Thanks for checking.  I installed a similar patch as Emacs master commit
0592cefd03f1de2f04b721d07a16e6e0a9e48f73; could you please give it a try?  It is 
like Mike Crowe's patch, except it avoids a race that could lose SIGIOs.  I'll 
also send a heads-up email to the relevant bug reports.

Re: [PATCH] Re-enable SIGIO when waiting for events

[Mike Crowe]

On Friday 17 July 2015 at 12:00:17 -0700, Paul Eggert wrote:
> Tassilo Horn wrote:
> 
> >I'm using it since yesterday and didn't see that issue anymore although
> >I've done some heavy c&p-ing between my browser and emacs which used to
> >trigger the problem with a very high likelyhood.
> 
> Thanks for checking.  I installed a similar patch as Emacs master commit
> 0592cefd03f1de2f04b721d07a16e6e0a9e48f73; could you please give it a try?
> It is like Mike Crowe's patch, except it avoids a race that could lose
> SIGIOs.  I'll also send a heads-up email to the relevant bug reports.

I prefer your version and will switch to testing it instead.

Thanks.

Mike.

Re: [PATCH] Re-enable SIGIO when waiting for events

[Mike Crowe]

On Tuesday 21 July 2015 at 13:42:20 +0100, Mike Crowe wrote:
> On Friday 17 July 2015 at 12:00:17 -0700, Paul Eggert wrote:
> > Tassilo Horn wrote:
> > 
> > >I'm using it since yesterday and didn't see that issue anymore although
> > >I've done some heavy c&p-ing between my browser and emacs which used to
> > >trigger the problem with a very high likelyhood.
> > 
> > Thanks for checking.  I installed a similar patch as Emacs master commit
> > 0592cefd03f1de2f04b721d07a16e6e0a9e48f73; could you please give it a try?
> > It is like Mike Crowe's patch, except it avoids a race that could lose
> > SIGIOs.  I'll also send a heads-up email to the relevant bug reports.
> 
> I prefer your version and will switch to testing it instead.

I've been testing with 0592cefd03f1de2f04b721d07a16e6e0a9e48f73 applied to
Debian Jessie's emacs24 (24.4+1-5) for over a week now without seeing any
problems so I think we can call the bug fixed.

Thanks Paul!

Mike.

Re: [PATCH] Re-enable SIGIO when waiting for events

[Tassilo Horn]

Mike Crowe <mac@mcrowe.com> writes:

>> > Thanks for checking.  I installed a similar patch as Emacs master
>> > commit 0592cefd03f1de2f04b721d07a16e6e0a9e48f73; could you please
>> > give it a try?  It is like Mike Crowe's patch, except it avoids a
>> > race that could lose SIGIOs.  I'll also send a heads-up email to
>> > the relevant bug reports.
>> 
>> I prefer your version and will switch to testing it instead.
>
> I've been testing with 0592cefd03f1de2f04b721d07a16e6e0a9e48f73
> applied to Debian Jessie's emacs24 (24.4+1-5) for over a week now
> without seeing any problems so I think we can call the bug fixed.

And I run the git head updating daily, and the issue didn't occur to me
since that commit whereas it bit me several times a day before.  So I
can confirm that it's fixed (as multiple other already did in the
debbugs issue), so I close it with this mail.

Thanks!
Tassilo

Re: [PATCH] Re-enable SIGIO when waiting for events

[Alex Bennée]

Tassilo Horn <tsdh@gnu.org> writes:

> Mike Crowe <mac@mcrowe.com> writes:
>
>>> > Thanks for checking.  I installed a similar patch as Emacs master
>>> > commit 0592cefd03f1de2f04b721d07a16e6e0a9e48f73; could you please
>>> > give it a try?  It is like Mike Crowe's patch, except it avoids a
>>> > race that could lose SIGIOs.  I'll also send a heads-up email to
>>> > the relevant bug reports.
>>> 
>>> I prefer your version and will switch to testing it instead.
>>
>> I've been testing with 0592cefd03f1de2f04b721d07a16e6e0a9e48f73
>> applied to Debian Jessie's emacs24 (24.4+1-5) for over a week now
>> without seeing any problems so I think we can call the bug fixed.
>
> And I run the git head updating daily, and the issue didn't occur to me
> since that commit whereas it bit me several times a day before.  So I
> can confirm that it's fixed (as multiple other already did in the
> debbugs issue), so I close it with this mail.

Will it get applied to the emacs-24 branch or is that considered dead now?

>
> Thanks!
> Tassilo

-- 
Alex Bennée

Re: [PATCH] Re-enable SIGIO when waiting for events

[Mike Crowe]

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

Paul Eggert wrote:
>>>>> Thanks for checking.  I installed a similar patch as Emacs master
>>>>> commit 0592cefd03f1de2f04b721d07a16e6e0a9e48f73; could you please
>>>>> give it a try?  It is like Mike Crowe's patch, except it avoids a
>>>>> race that could lose SIGIOs.  I'll also send a heads-up email to the
>>>>> relevant bug reports.

I wrote:
>>> I've been testing with 0592cefd03f1de2f04b721d07a16e6e0a9e48f73
>>> applied to Debian Jessie's emacs24 (24.4+1-5) for over a week now
>>> without seeing any problems so I think we can call the bug fixed.
 
On Friday 31 July 2015 at 07:03:39 +0100, Alex Bennée wrote:
> Will it get applied to the emacs-24 branch or is that considered dead now?

I'm sort of hoping that I can get this fix into a Debian Jessie stable
update. It being in upstream emacs-24 would make this more rather more
likely. (Debian doesn't allow stuff to be fixed in stable that isn't
already fixed in unstable.)

Is there any chance of committing the fix to emacs-24? It doesn't
cherry-pick cleanly but only requires minor fixing (attached.)

Thanks.

Mike.

[-- Attachment #2: 0001-Fix-hang-with-large-yanks.patch --]
[-- Type: text/x-diff, Size: 3407 bytes --]

From d47c5b810123c6cbd14dd38ac461aa01a4eabacf Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 17 Jul 2015 11:54:24 -0700
Subject: [PATCH] Fix hang with large yanks

Backport of emacs-25 0592cefd03f1de2f04b721d07a16e6e0a9e48f73. Upstream
comment follows:

Fix hang with large yanks This should fix the bug fixed by Mike

Crowe's patch in:
https://lists.gnu.org/archive/html/emacs-devel/2015-07/msg00106.html
A problem in this area has been reported by several users; see
Bug#16737, Bug#17101, Bug#17026, Bug#17172, Bug#19320, Bug#20283.
This fix differs from Mike Crowe's patch in that it should avoid a
race condition that could lose SIGIO signals.  ignore_sigio dates
back to the 1980s when some platforms couldn't block signals, and
could only ignore them, which led to races when signals arrived
while being ignored.  We shouldn't have to worry about those old
platforms now.
* src/dispextern.h, src/sysdep.c (ignore_sigio): Remove.
* src/emacs.c (shut_down_emacs):
Don't call ignore_sigio; unrequest_sigio should suffice.
* src/keyboard.c (kbd_buffer_store_buffered_event):
Use unrequest_sigio, not ignore_sigio.
(kbd_buffer_get_event):
Call request_sigio when getting the ball rolling again.

Conflicts:
	src/sysdep.c
---
 src/dispextern.h | 1 -
 src/emacs.c      | 1 -
 src/keyboard.c   | 4 ++--
 src/sysdep.c     | 9 ---------
 4 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 239c442..cf3d1ec 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3349,7 +3349,6 @@ void unrequest_sigio (void);
 bool tabs_safe_p (int);
 void init_baud_rate (int);
 void init_sigio (int);
-void ignore_sigio (void);
 
 /* Defined in xfaces.c.  */
 
diff --git a/src/emacs.c b/src/emacs.c
index 9b78a70..b5d3ab4 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -2028,7 +2028,6 @@ shut_down_emacs (int sig, Lisp_Object stuff)
   /* There is a tendency for a SIGIO signal to arrive within exit,
      and cause a SIGHUP because the input descriptor is already closed.  */
   unrequest_sigio ();
-  ignore_sigio ();
 
   /* Do this only if terminating normally, we want glyph matrices
      etc. in a core dump.  */
diff --git a/src/keyboard.c b/src/keyboard.c
index 945019e..77af44a 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -3663,8 +3663,7 @@ kbd_buffer_store_event_hold (register struct input_event *event,
           /* Don't read keyboard input until we have processed kbd_buffer.
              This happens when pasting text longer than KBD_BUFFER_SIZE/2.  */
           hold_keyboard_input ();
-          if (!noninteractive)
-            ignore_sigio ();
+          unrequest_sigio ();
           stop_polling ();
         }
 #endif	/* subprocesses */
@@ -3829,6 +3828,7 @@ kbd_buffer_get_event (KBOARD **kbp,
       /* Start reading input again because we have processed enough to
          be able to accept new events again.  */
       unhold_keyboard_input ();
+      request_sigio ();
       start_polling ();
     }
 #endif	/* subprocesses */
diff --git a/src/sysdep.c b/src/sysdep.c
index 01692c2..4b4801d 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -649,15 +649,6 @@ unrequest_sigio (void)
   interrupts_deferred = 1;
 #endif
 }
-
-void
-ignore_sigio (void)
-{
-#ifdef USABLE_SIGIO
-  signal (SIGIO, SIG_IGN);
-#endif
-}
-
 \f
 /* Saving and restoring the process group of Emacs's terminal.  */
 
-- 
2.1.4

Re: [PATCH] Re-enable SIGIO when waiting for events

[Paul Eggert]

On 08/05/2015 07:28 AM, Mike Crowe wrote:
> Is there any chance of committing the fix to emacs-24? It doesn't
> cherry-pick cleanly but only requires minor fixing (attached.)

Don't see why not.  I pushed it.  I doubt whether emacs-24 will be used 
for any new release, so this is mostly just a bookkeeping entry.

Re: [PATCH] Re-enable SIGIO when waiting for events (bug# 16737)

[Alan D. Salewski]

On 2015-08-05 10:46:21, Paul Eggert spake thus:
> On 08/05/2015 07:28 AM, Mike Crowe wrote:
> >Is there any chance of committing the fix to emacs-24? It doesn't
> >cherry-pick cleanly but only requires minor fixing (attached.)
> 
> Don't see why not.  I pushed it.  I doubt whether emacs-24 will be
> used for any new release, so this is mostly just a bookkeeping
> entry.

I just want to say thanks to all who have contributed to addressing this
bug, and especially to Mike Crowe for homing in on the problem and Paul
Eggert for the final fix (and its application to both branches[0]).  I
very much appreciate all the time and effort folks have expended on this
issue.

Thank you!

-Al


[0] master   (commit: 0592cefd03f1de2f04b721d07a16e6e0a9e48f73 )
    and
    emacs-24 (commit: a27ae9d7650a1230d4359eaf0a949f827315a6d2 )