bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Lars Ingebrigtsen]

To reproduce the problem on Debian (at least), edit src/config.h to
this:

/* Define to 1 if SIGIO is usable. */
/* #define USABLE_SIGIO 0 */

Then recompile Emacs and say

(let ((value 'SECONDARY)
      (x-selection-timeout 2000))
  ;;; from org-get-x-clipboard in lisp/org-compat.el
  (gui-get-selection value 'UTF8_STRING)
  (gui-get-selection value 'COMPOUND_TEXT)
  (gui-get-selection value 'STRING)
  (gui-get-selection value 'TEXT))

This will hang for two seconds instead of returning immediately if
USABLE_SIGIO is defined.

Now, we have SIGIO on almost all supported platforms.  The only ones
that have it disabled might be these ones:

  hpux* | nacl | solaris | unixware )
    emacs_broken_SIGIO=yes

Which is probably why this hasn't been reported more.  (See bug#29170.)

So this isn't exactly a high impact problem, but it should be fixed
anyway.



-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 13 Aug 2021 13:56:40 +0200
> 
> 
> To reproduce the problem on Debian (at least), edit src/config.h to
> this:
> 
> /* Define to 1 if SIGIO is usable. */
> /* #define USABLE_SIGIO 0 */
> 
> Then recompile Emacs and say
> 
> (let ((value 'SECONDARY)
>       (x-selection-timeout 2000))
>   ;;; from org-get-x-clipboard in lisp/org-compat.el
>   (gui-get-selection value 'UTF8_STRING)
>   (gui-get-selection value 'COMPOUND_TEXT)
>   (gui-get-selection value 'STRING)
>   (gui-get-selection value 'TEXT))
> 
> This will hang for two seconds instead of returning immediately if
> USABLE_SIGIO is defined.

can you tell where does it hang, and why?  Bonus points for explaining
why the same scenario doesn't hang for USABLE_SIGIO platforms.

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> can you tell where does it hang, and why?  Bonus points for explaining
> why the same scenario doesn't hang for USABLE_SIGIO platforms.

It's hanging here:

x_get_foreign_selection (Lisp_Object selection_symbol, Lisp_Object target_type,
			 Lisp_Object time_stamp, Lisp_Object frame)
{
[...]
  wait_reading_process_output (secs, nsecs, 0, false,
			       reading_selection_reply, NULL, 0);

That is, it's not really hanging hanging, but this sometimes takes a
couple of seconds without SIGIO, while it returns instantaneously with
SIGOI.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 50043@debbugs.gnu.org
> Date: Fri, 13 Aug 2021 16:31:52 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > can you tell where does it hang, and why?  Bonus points for explaining
> > why the same scenario doesn't hang for USABLE_SIGIO platforms.
> 
> It's hanging here:
> 
> x_get_foreign_selection (Lisp_Object selection_symbol, Lisp_Object target_type,
> 			 Lisp_Object time_stamp, Lisp_Object frame)
> {
> [...]
>   wait_reading_process_output (secs, nsecs, 0, false,
> 			       reading_selection_reply, NULL, 0);
> 
> That is, it's not really hanging hanging, but this sometimes takes a
> couple of seconds without SIGIO, while it returns instantaneously with
> SIGOI.

Do you understand which code un-hangs it when USABLE_SIGIO is defined?
Or is it a SIGIO signal that arrives and does that?  If the latter,
any idea what causes that SIGIO?

Thanks.

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Do you understand which code un-hangs it when USABLE_SIGIO is defined?
> Or is it a SIGIO signal that arrives and does that?  If the latter,
> any idea what causes that SIGIO?

Nope; haven't digged any further.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Ken Brown]

On 8/14/2021 7:52 AM, Lars Ingebrigtsen wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>> Do you understand which code un-hangs it when USABLE_SIGIO is defined?
>> Or is it a SIGIO signal that arrives and does that?  If the latter,
>> any idea what causes that SIGIO?
> 
> Nope; haven't digged any further.

I think I found the problem.

x_get_foreign_selection (Lisp_Object selection_symbol, Lisp_Object target_type,
			 Lisp_Object time_stamp, Lisp_Object frame)
{
[...]
   wait_reading_process_output (secs, nsecs, 0, false,
			       reading_selection_reply, NULL, 0);

I think wait_reading_process_output gets stuck for 2 seconds in a call to select 
(actually xg_select because I'm testing a gtk build).  This is independent of 
the fact that x-selection-timeout is 2 seconds; it happens even if 
x-selection-timeout is 0.  select returns after 2 seconds because the poll_timer 
fires.  On systems with SIGIO, select returns as soon as X events occur, because 
SIGIO is signaled.

To test my theory, I applied the following patch:

diff --git a/src/process.c b/src/process.c
index f923aff1cb..136873edbb 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5477,9 +5477,10 @@ wait_reading_process_output (intmax_t time_limit, int 
nsecs, int read_kbd,
          triggered by processing X events).  In the latter case, set
          nfds to 1 to avoid breaking the loop.  */
        no_avail = 0;
-      if ((read_kbd || !NILP (wait_for_cell))
-         && detect_input_pending ())
+      if ((read_kbd || !NILP (wait_for_cell)))
+         /* && detect_input_pending ()) */
         {
+         detect_input_pending ();
           nfds = read_kbd ? 0 : 1;
           no_avail = 1;
           FD_ZERO (&Available);

This forces the select call to be skipped, so wait_reading_process_output just 
keeps looping, checking for input, and checking for a cell change.  With this 
patch, the hang is gone.

I'm not suggesting that this patch is the right fix.  But somehow we have to 
improve how polling for input is done on systems without SIGIO, at least in the 
situation above in which we're waiting for a cell.  There's a comment in 
process.c around line 5304 that says "the wait is supposed to be short" in this 
case.

We certainly don't want to always skip the select call, but would it make sense 
to use a very short timeout for select in that case?  Or maybe someone has a 
better idea.

Ken

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Eli Zaretskii]

> Date: Mon, 15 Nov 2021 10:19:32 -0500
> Cc: 50043@debbugs.gnu.org
> From: Ken Brown <kbrown@cornell.edu>
> 
> x_get_foreign_selection (Lisp_Object selection_symbol, Lisp_Object target_type,
> 			 Lisp_Object time_stamp, Lisp_Object frame)
> {
> [...]
>    wait_reading_process_output (secs, nsecs, 0, false,
> 			       reading_selection_reply, NULL, 0);
> 
> I think wait_reading_process_output gets stuck for 2 seconds in a call to select 
> (actually xg_select because I'm testing a gtk build).  This is independent of 
> the fact that x-selection-timeout is 2 seconds; it happens even if 
> x-selection-timeout is 0.  select returns after 2 seconds because the poll_timer 
> fires.

Sorry, I don't understand: select waits for up to 2 seconds because
that's what we ask it to do, and those 2 sec do come from
x-selection-timeout.  If x-selection-timeout is zero, select is not
supposed to wait at all, so why does it?  What am I missing?

> On systems with SIGIO, select returns as soon as X events occur, because 
> SIGIO is signaled.

Which X event is that? something related to Emacs and selections, or
just a random event which simply happens at that time?

Anyway, AFAIU, the wait is supposed to end because XTread_socket reads
a SelectionNotify event, and that modifies the cell for which we
wait.  What I'm not sure I understand is how are we supposed to call
XTread_socket when we are stuck inside select all the time?

> We certainly don't want to always skip the select call, but would it make sense 
> to use a very short timeout for select in that case?  Or maybe someone has a 
> better idea.

Making timeout shorter might be the solution, but I'd like to
understand the problem better first.

Thanks.

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Ken Brown]

On 11/15/2021 12:24 PM, Eli Zaretskii wrote:
>> Date: Mon, 15 Nov 2021 10:19:32 -0500
>> Cc: 50043@debbugs.gnu.org
>> From: Ken Brown <kbrown@cornell.edu>
>>
>> x_get_foreign_selection (Lisp_Object selection_symbol, Lisp_Object target_type,
>> 			 Lisp_Object time_stamp, Lisp_Object frame)
>> {
>> [...]
>>     wait_reading_process_output (secs, nsecs, 0, false,
>> 			       reading_selection_reply, NULL, 0);
>>
>> I think wait_reading_process_output gets stuck for 2 seconds in a call to select
>> (actually xg_select because I'm testing a gtk build).  This is independent of
>> the fact that x-selection-timeout is 2 seconds; it happens even if
>> x-selection-timeout is 0.  select returns after 2 seconds because the poll_timer
>> fires.
> 
> Sorry, I don't understand: select waits for up to 2 seconds because
> that's what we ask it to do, and those 2 sec do come from
> x-selection-timeout.  If x-selection-timeout is zero, select is not
> supposed to wait at all, so why does it?  What am I missing?

Setting x-selection-timeout to zero actually makes the timeout infinite:

   if (time_limit < 0 || nsecs < 0)
     wait = MINIMUM;
   else if (time_limit > 0 || nsecs > 0)
     {
       wait = TIMEOUT;
       now = current_timespec ();
       end_time = timespec_add (now, make_timespec (time_limit, nsecs));
     }
   else
     wait = FOREVER;         <<<<<<<<<<<<<<<<<<<<<<

If x-selection-timeout is zero and you really want select to use a timeout of 
zero, you have to specify a negative value for nsecs.  [This strikes me as very 
counterintuitive and a poor design decision.]  x_get_foreign_selection should 
probably be changed to account for this, since the default value of 
x-selection-timeout is in fact zero, and clearly the intention was not to have 
an infinite wait in this case.  There's a comment in x_get_foreign_selection 
that says "don't wait forever".

>> On systems with SIGIO, select returns as soon as X events occur, because
>> SIGIO is signaled.
> 
> Which X event is that? something related to Emacs and selections, or
> just a random event which simply happens at that time?

I guess it's whatever X event is supposed to come in reply to the call

   XConvertSelection (display, selection_atom, type_atom, target_property,
		     requestor_window, requestor_time);

in x_get_foreign_selection.  I don't anything about how I/O works under X, so I 
can't be more specific.

> Anyway, AFAIU, the wait is supposed to end because XTread_socket reads
> a SelectionNotify event, and that modifies the cell for which we
> wait.  What I'm not sure I understand is how are we supposed to call
> XTread_socket when we are stuck inside select all the time?

We're never stuck for more than 2 seconds [when there's no SIGIO] because 
poll_timer fires and either sends SIGALRM or makes timerfd read ready.  Either 
way, select returns, and the next iteration of the main loop checks for input 
and checks for a cell change.

>> We certainly don't want to always skip the select call, but would it make sense
>> to use a very short timeout for select in that case?  Or maybe someone has a
>> better idea.
> 
> Making timeout shorter might be the solution, but I'd like to
> understand the problem better first.

Ken

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Eli Zaretskii]

> Date: Mon, 15 Nov 2021 14:26:08 -0500
> Cc: larsi@gnus.org, 50043@debbugs.gnu.org
> From: Ken Brown <kbrown@cornell.edu>
> 
> >> I think wait_reading_process_output gets stuck for 2 seconds in a call to select
> >> (actually xg_select because I'm testing a gtk build).  This is independent of
> >> the fact that x-selection-timeout is 2 seconds; it happens even if
> >> x-selection-timeout is 0.  select returns after 2 seconds because the poll_timer
> >> fires.
> > 
> > Sorry, I don't understand: select waits for up to 2 seconds because
> > that's what we ask it to do, and those 2 sec do come from
> > x-selection-timeout.  If x-selection-timeout is zero, select is not
> > supposed to wait at all, so why does it?  What am I missing?
> 
> Setting x-selection-timeout to zero actually makes the timeout infinite:

Ah, I thought you meant a literally zero timeout, not the "zero means
infinite" one.

> > Anyway, AFAIU, the wait is supposed to end because XTread_socket reads
> > a SelectionNotify event, and that modifies the cell for which we
> > wait.  What I'm not sure I understand is how are we supposed to call
> > XTread_socket when we are stuck inside select all the time?
> 
> We're never stuck for more than 2 seconds [when there's no SIGIO] because 
> poll_timer fires and either sends SIGALRM or makes timerfd read ready.  Either 
> way, select returns, and the next iteration of the main loop checks for input 
> and checks for a cell change.
> 
> >> We certainly don't want to always skip the select call, but would it make sense
> >> to use a very short timeout for select in that case?  Or maybe someone has a
> >> better idea.
> > 
> > Making timeout shorter might be the solution, but I'd like to
> > understand the problem better first.

If the code is based on the premise that we check for selection when
we exit select, then I think on systems without USABLE_SIGIO we should
call wait_reading_process_output with a short timeout but in a loop,
so that the summary wait is still 2 sec, but we exit the loop as soon
as selection arrives because each call to wait_reading_process_output
has a much shorter timeout, say, 25 msec.  WDYT?

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Ken Brown]

On 11/16/2021 12:45 PM, Eli Zaretskii wrote:
>> From: Ken Brown <kbrown@cornell.edu>
>>>> We certainly don't want to always skip the select call, but would it make sense
>>>> to use a very short timeout for select in that case?  Or maybe someone has a
>>>> better idea.
>>>
>>> Making timeout shorter might be the solution, but I'd like to
>>> understand the problem better first.
> 
> If the code is based on the premise that we check for selection when
> we exit select, then I think on systems without USABLE_SIGIO we should
> call wait_reading_process_output with a short timeout but in a loop,
> so that the summary wait is still 2 sec, but we exit the loop as soon
> as selection arrives because each call to wait_reading_process_output
> has a much shorter timeout, say, 25 msec.  WDYT?

Are you talking about having x_get_foreign_selection call 
wait_reading_process_output in a loop?  That would fix this particular bug, but 
I was thinking of trying to solve a more general problem.  Namely, whenever 
wait_reading_process_output is polling for input, avoid getting stuck in select, 
something like this:

diff --git a/src/process.c b/src/process.c
index f923aff1cb..808bf6f1ff 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5588,6 +5588,15 @@ wait_reading_process_output (intmax_t time_limit, int 
nsecs, int read_kbd,
             timeout = make_timespec (0, 0);
  #endif

+#ifndef USABLE_SIGIO
+         /* If we're polling for input, don't get stuck in select for
+            more than 25 msec. */
+         struct timespec short_timeout = make_timespec (0, 25000000);
+         if ((read_kbd || !NILP (wait_for_cell))
+             && timespec_cmp (short_timeout, timeout) < 0)
+           timeout = short_timeout;
+#endif
+
           /* Non-macOS HAVE_GLIB builds call thread_select in xgselect.c.  */
  #if defined HAVE_GLIB && !defined HAVE_NS
           nfds = xg_select (max_desc + 1,

Ken

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Lars Ingebrigtsen]

Ken Brown <kbrown@cornell.edu> writes:

> Namely, whenever wait_reading_process_output is polling for input,
> avoid getting stuck in select, something like this:

[...]

> +#ifndef USABLE_SIGIO
> +         /* If we're polling for input, don't get stuck in select for
> +            more than 25 msec. */
> +         struct timespec short_timeout = make_timespec (0, 25000000);
> +         if ((read_kbd || !NILP (wait_for_cell))
> +             && timespec_cmp (short_timeout, timeout) < 0)
> +           timeout = short_timeout;
> +#endif

Sounds like a good general solution to me (but I guess this will also
affect `accept-process-output'?  So it should probably be documented
there).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Eli Zaretskii]

> Date: Tue, 16 Nov 2021 18:06:51 -0500
> Cc: larsi@gnus.org, 50043@debbugs.gnu.org
> From: Ken Brown <kbrown@cornell.edu>
> 
> Are you talking about having x_get_foreign_selection call 
> wait_reading_process_output in a loop?

Yes.

> That would fix this particular bug, but I was thinking of trying to
> solve a more general problem.

What is that more general problem, and when does it rear its ugly
head?

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Ken Brown]

On 11/17/2021 8:14 AM, Eli Zaretskii wrote:
>> Date: Tue, 16 Nov 2021 18:06:51 -0500
>> Cc: larsi@gnus.org, 50043@debbugs.gnu.org
>> From: Ken Brown <kbrown@cornell.edu>
>>
>> Are you talking about having x_get_foreign_selection call
>> wait_reading_process_output in a loop?
> 
> Yes.
> 
>> That would fix this particular bug, but I was thinking of trying to
>> solve a more general problem.
> 
> What is that more general problem, and when does it rear its ugly
> head?

The problem is that whenever wait_reading_process_output needs to repeatedly 
check for input (i.e., read_kbd || !NILP (wait_for_cell) == true), it can get 
stuck in select for up to 2 seconds on systems without USABLE_SIGIO.  (2 arises 
here because it's the default value of polling-period.)

I don't know offhand when it rears its ugly head aside from the X selection case 
we're discussing.  We would only know about it if someone notices an unexpected 
delay, finds a recipe for reproducing it, and reports it as a bug, as happened 
here with bug#29170, which is what led to the present bug report.  And look how 
long it took before the cause of that delay was discovered.

So I think it's better to fix the general (potential) problem, not just the X 
selection case.

Ken

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Ken Brown]

On 11/17/2021 2:41 AM, Lars Ingebrigtsen wrote:
> Ken Brown <kbrown@cornell.edu> writes:
> 
>> Namely, whenever wait_reading_process_output is polling for input,
>> avoid getting stuck in select, something like this:
> 
> [...]
> 
>> +#ifndef USABLE_SIGIO
>> +         /* If we're polling for input, don't get stuck in select for
>> +            more than 25 msec. */
>> +         struct timespec short_timeout = make_timespec (0, 25000000);
>> +         if ((read_kbd || !NILP (wait_for_cell))
>> +             && timespec_cmp (short_timeout, timeout) < 0)
>> +           timeout = short_timeout;
>> +#endif
> 
> Sounds like a good general solution to me (but I guess this will also
> affect `accept-process-output'?  So it should probably be documented
> there).

I don't think it affects accept-process-output.  When the latter calls 
wait_reading_process_output, read_kbd == 0 and wait_for_cell == Qnil.

Ken

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Eli Zaretskii]

> Date: Wed, 17 Nov 2021 09:19:27 -0500
> Cc: larsi@gnus.org, 50043@debbugs.gnu.org
> From: Ken Brown <kbrown@cornell.edu>
> 
> > What is that more general problem, and when does it rear its ugly
> > head?
> 
> The problem is that whenever wait_reading_process_output needs to repeatedly 
> check for input (i.e., read_kbd || !NILP (wait_for_cell) == true), it can get 
> stuck in select for up to 2 seconds on systems without USABLE_SIGIO.  (2 arises 
> here because it's the default value of polling-period.)

But that can only happen if select is called with a long enough
timeout, right?

Anyway, if the problem is that on these systems select doesn't return
when there are input events waiting, I guess it's okay to do the
change there.  But maybe it will be better to do it the same way we
decrease the timeout when a timer is expected to expire before the
timeout: we decrease the timeout, but still remember its value, and
don't return from waiting before the timeout unless there really was
some input.  This would avoid affecting unrelated features such as
accept-process-output.

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Ken Brown]

On 11/17/2021 9:34 AM, Eli Zaretskii wrote:
>> Date: Wed, 17 Nov 2021 09:19:27 -0500
>> Cc: larsi@gnus.org, 50043@debbugs.gnu.org
>> From: Ken Brown <kbrown@cornell.edu>
>>
>>> What is that more general problem, and when does it rear its ugly
>>> head?
>>
>> The problem is that whenever wait_reading_process_output needs to repeatedly
>> check for input (i.e., read_kbd || !NILP (wait_for_cell) == true), it can get
>> stuck in select for up to 2 seconds on systems without USABLE_SIGIO.  (2 arises
>> here because it's the default value of polling-period.)
> 
> But that can only happen if select is called with a long enough
> timeout, right?

Right.

> Anyway, if the problem is that on these systems select doesn't return
> when there are input events waiting, I guess it's okay to do the
> change there.

Thanks.

> But maybe it will be better to do it the same way we
> decrease the timeout when a timer is expected to expire before the
> timeout: we decrease the timeout, but still remember its value, and
> don't return from waiting before the timeout unless there really was
> some input.  This would avoid affecting unrelated features such as
> accept-process-output.

I think that's already taken care of.  The timeout is recalculated on each 
iteration of the main while loop:

       /* Compute time from now till when time limit is up.  */
       /* Exit if already run out.  */
       if (wait == TIMEOUT)
	{
	  if (!timespec_valid_p (now))
	    now = current_timespec ();
	  if (timespec_cmp (end_time, now) <= 0)
	    break;
	  timeout = timespec_sub (end_time, now);
	}
       else
	timeout = make_timespec (wait < TIMEOUT ? 0 : 100000, 0);

My reduction of timeout to 25 msec occurs after this.

Ken

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Eli Zaretskii]

> Date: Wed, 17 Nov 2021 09:59:35 -0500
> Cc: larsi@gnus.org, 50043@debbugs.gnu.org
> From: Ken Brown <kbrown@cornell.edu>
> 
> > But maybe it will be better to do it the same way we
> > decrease the timeout when a timer is expected to expire before the
> > timeout: we decrease the timeout, but still remember its value, and
> > don't return from waiting before the timeout unless there really was
> > some input.  This would avoid affecting unrelated features such as
> > accept-process-output.
> 
> I think that's already taken care of.  The timeout is recalculated on each 
> iteration of the main while loop:
> 
>        /* Compute time from now till when time limit is up.  */
>        /* Exit if already run out.  */
>        if (wait == TIMEOUT)
> 	{
> 	  if (!timespec_valid_p (now))
> 	    now = current_timespec ();
> 	  if (timespec_cmp (end_time, now) <= 0)
> 	    break;
> 	  timeout = timespec_sub (end_time, now);
> 	}
>        else
> 	timeout = make_timespec (wait < TIMEOUT ? 0 : 100000, 0);
> 
> My reduction of timeout to 25 msec occurs after this.

So the result will be that on systems without USABLE_SIGIO we loop
more times with shorter timeouts for select?  If so, it SGTM.

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Ken Brown]

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

On 11/17/2021 11:57 AM, Eli Zaretskii wrote:
>> Date: Wed, 17 Nov 2021 09:59:35 -0500
>> Cc: larsi@gnus.org, 50043@debbugs.gnu.org
>> From: Ken Brown <kbrown@cornell.edu>
>>
>>> But maybe it will be better to do it the same way we
>>> decrease the timeout when a timer is expected to expire before the
>>> timeout: we decrease the timeout, but still remember its value, and
>>> don't return from waiting before the timeout unless there really was
>>> some input.  This would avoid affecting unrelated features such as
>>> accept-process-output.
>>
>> I think that's already taken care of.  The timeout is recalculated on each
>> iteration of the main while loop:
>>
>>         /* Compute time from now till when time limit is up.  */
>>         /* Exit if already run out.  */
>>         if (wait == TIMEOUT)
>> 	{
>> 	  if (!timespec_valid_p (now))
>> 	    now = current_timespec ();
>> 	  if (timespec_cmp (end_time, now) <= 0)
>> 	    break;
>> 	  timeout = timespec_sub (end_time, now);
>> 	}
>>         else
>> 	timeout = make_timespec (wait < TIMEOUT ? 0 : 100000, 0);
>>
>> My reduction of timeout to 25 msec occurs after this.
> 
> So the result will be that on systems without USABLE_SIGIO we loop
> more times with shorter timeouts for select?

Yes.

> If so, it SGTM.

Thanks, I've pushed that change.

There's still one more issue related to this bug, which I mentioned earlier: If 
x_selection_timeout is zero, x_get_foreign_selection will call 
wait_reading_process output with an infinite timeout rather than a zero timeout, 
which I'm sure is not what was intended.  The attached patch fixes that.

Ken

[-- Attachment #2: 0001-Avoid-an-accidental-use-of-an-infinite-timeout.patch --]
[-- Type: text/plain, Size: 1151 bytes --]

From 5a0afdf1cb2ef276a2b6e4cf26abf18405127db2 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Wed, 17 Nov 2021 12:20:24 -0500
Subject: [PATCH] Avoid an accidental use of an infinite timeout

* src/xselect.c (x_get_foreign_selection): If x-selection-timeout
is zero, call wait_reading_process_output with a zero timeout
rather than an infinite timeout.  (Bug#50043)
---
 src/xselect.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/xselect.c b/src/xselect.c
index cd6d86bdf4..53b41f1ea2 100644
--- a/src/xselect.c
+++ b/src/xselect.c
@@ -1196,6 +1196,10 @@ x_get_foreign_selection (Lisp_Object selection_symbol, Lisp_Object target_type,
   intmax_t timeout = max (0, x_selection_timeout);
   intmax_t secs = timeout / 1000;
   int nsecs = (timeout % 1000) * 1000000;
+  /* If x_selection_timeout == 0, avoid the "zero means infinite"
+     behavior of wait_reading_process_output. */
+  if (secs == 0 && nsecs == 0)
+    nsecs = -1;
   TRACE1 ("  Start waiting %"PRIdMAX" secs for SelectionNotify", secs);
   wait_reading_process_output (secs, nsecs, 0, false,
 			       reading_selection_reply, NULL, 0);
-- 
2.33.0

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Eli Zaretskii]

> Date: Wed, 17 Nov 2021 12:25:38 -0500
> Cc: larsi@gnus.org, 50043@debbugs.gnu.org
> From: Ken Brown <kbrown@cornell.edu>
> 
> There's still one more issue related to this bug, which I mentioned earlier: If 
> x_selection_timeout is zero, x_get_foreign_selection will call 
> wait_reading_process output with an infinite timeout rather than a zero timeout, 
> which I'm sure is not what was intended.  The attached patch fixes that.

Bu that's clearly what was intended.  The doc string of
x-selection-timeout says:

  A value of 0 means wait as long as necessary.

And with the fix in wait_reading_process_output the original problem
is gone, since we will now loop with short select timeout, waiting for
the selection event.  Which is the intent here, AFAIU.

bug#50043: 28.0.50; USABLE_SIGOI undef code paths do not work correctly

[Ken Brown]

On 11/17/2021 12:37 PM, Eli Zaretskii wrote:
>> Date: Wed, 17 Nov 2021 12:25:38 -0500
>> Cc: larsi@gnus.org, 50043@debbugs.gnu.org
>> From: Ken Brown <kbrown@cornell.edu>
>>
>> There's still one more issue related to this bug, which I mentioned earlier: If
>> x_selection_timeout is zero, x_get_foreign_selection will call
>> wait_reading_process output with an infinite timeout rather than a zero timeout,
>> which I'm sure is not what was intended.  The attached patch fixes that.
> 
> Bu that's clearly what was intended.  The doc string of
> x-selection-timeout says:
> 
>    A value of 0 means wait as long as necessary.

Oh, I somehow missed that, sorry.  In that case I'm closing the bug.

Ken