sit-for and idle timers

sit-for and idle timers

[Noah Friedman]

The change to sit-for of 2006-07-26 ("Use new SECONDS arg of read-event
instead of a timer") seems to cause problems with idle-timers
which call sit-for.

The problem is that read-event's call tree ultimately results in calling
keyboard.c:read_char, which calls timer_start_idle.  This resets the
activation time for all the current idle timer events, which means that any
function on an idle timer which calls sit-for is now getting scheduled to
be run recursively if another interval of the appropriate length ensues.  I
imagine this can continue until max-lisp-eval-depth is reached.

Here's a small test case which demonstrates the problem:

    (defvar itimer-test-wait 0.5)
    (defvar itimer-test-depth 0)

    (defun itimer-test ()
      (setq itimer-test-depth (1+ itimer-test-depth))
      (unwind-protect
          (let ((flag nil))
            (while (sit-for itimer-test-wait)
              (setq flag (not flag))
              (message "itimer-test-depth: %-3d%s"
                       itimer-test-depth
                       (if flag " (blink)" ""))))
        (setq itimer-test-depth (1- itimer-test-depth))
        (message "itimer-test-depth: %d" itimer-test-depth)))

    (run-with-idle-timer 0.25 t 'itimer-test)

In the pre-7/26 implementation, this timer should never print a depth
greater than 1, and the "(blink)" text should blink on and off with a
regular rhythm.

In the post-7/26 implementation, itimer-test-depth increments indefinitely
until an event is read.

One solution to this problem is to bind timer-idle-list to nil while
calling read-event.  I tested this trivially with a small defadvice:

    (defadvice read-event (around idlefrob activate)
      (let ((timer-idle-list nil))
        ad-do-it))

I'm not sure if this (binding the variable in sit-for that is, not the use
of defadvice as a temporary kludge) is the correct solution since it will
preempt the activation of other idle timers.  But it's simple.  The less
simple solution is probably for sit-for to go back to using a timer itself.

Thoughts?

Re: sit-for and idle timers

[Chong Yidong]

Noah Friedman <friedman@splode.com> writes:

> The less simple solution is probably for sit-for to go back to using
> a timer itself.

The reason for not using a timer is so that when a process filter runs
during the sit-for, the expiry of the sit-for won't interrupt the
filter (which might be waiting for keyboard input).  Otherwise,
process filters can't reliably call input reading functions.  See the
thread labelled "a bug of read-passwd" from July.

> One solution to this problem is to bind timer-idle-list to nil while
> calling read-event.

This would inhibit all other idle timers from running during the
sit-for.  But jit-lock-stealth-fontify, for example, sits for 30
seconds, and I don't think we want to inhibit other idle timers during
this entire period.

> The problem is that read-event's call tree ultimately results in
> calling keyboard.c:read_char, which calls timer_start_idle.  This
> resets the activation time for all the current idle timer events,
> which means that any function on an idle timer which calls sit-for
> is now getting scheduled to be run recursively if another interval
> of the appropriate length ensues.

I think the solution is to avoid calling timer_start_idle when
read-event is given a non-nil SECONDS argument.  What do people think?

Re: sit-for and idle timers

[Chong Yidong]

Noah Friedman <friedman@splode.com> writes:

> The change to sit-for of 2006-07-26 ("Use new SECONDS arg of read-event
> instead of a timer") seems to cause problems with idle-timers
> which call sit-for.

This should now be fixed.

> Here's a small test case which demonstrates the problem:
>
>     (defvar itimer-test-wait 0.5)
>     (defvar itimer-test-depth 0)
>
>     (defun itimer-test ()
>       (setq itimer-test-depth (1+ itimer-test-depth))
>       (unwind-protect
>           (let ((flag nil))
>             (while (sit-for itimer-test-wait)
>               (setq flag (not flag))
>               (message "itimer-test-depth: %-3d%s"
>                        itimer-test-depth
>                        (if flag " (blink)" ""))))
>         (setq itimer-test-depth (1- itimer-test-depth))
>         (message "itimer-test-depth: %d" itimer-test-depth)))
>
>     (run-with-idle-timer 0.25 t 'itimer-test)
>
> In the pre-7/26 implementation, this timer should never print a depth
> greater than 1, and the "(blink)" text should blink on and off with a
> regular rhythm.

Re: sit-for and idle timers

[Richard Stallman]

    The change to sit-for of 2006-07-26 ("Use new SECONDS arg of read-event
    instead of a timer") seems to cause problems with idle-timers
    which call sit-for.


Why do these timer functions call sit-for?  It is a strange thing for
a timer to wait.  It should reschedule itself instead.  What are they
really trying to do?

    > The problem is that read-event's call tree ultimately results in
    > calling keyboard.c:read_char, which calls timer_start_idle.  This
    > resets the activation time for all the current idle timer events,
    > which means that any function on an idle timer which calls sit-for
    > is now getting scheduled to be run recursively if another interval
    > of the appropriate length ensues.

    I think the solution is to avoid calling timer_start_idle when
    read-event is given a non-nil SECONDS argument.  What do people think?

That is definitely not right.  Emacs really is idle when it reads an event,
even if there is a timeout.

The way to find the right fix is to ask WHY the current behavior is
wrong.  Calling timer_start_idle is appropriate when Emacs changes
from non-idle to idle.  It is wrong to call that function when Emacs
is already idle.

So my conclusion is that when read-event is called from an idle timer,
it should not change the state to idle at the beginning, and it should
not change the state away from idle at the end.


Does this change fix it?


*** keyboard.c	12 Aug 2006 17:31:29 -0400	1.866
--- keyboard.c	14 Aug 2006 03:48:20 -0400	
***************
*** 2416,2421 ****
--- 2416,2424 ----
    volatile int reread;
    struct gcpro gcpro1, gcpro2;
    int polling_stopped_here = 0;
+   /* If we are already in the idle state, for instance if we
+      are calling from an idle timer, stay idle when we exit.  */
+   int already_idle = ! EMACS_TIME_NEG_P (timer_idleness_start_time);
  
    also_record = Qnil;
  
***************
*** 2679,2685 ****
        goto non_reread;
      }
  
!   timer_start_idle ();
  
    /* If in middle of key sequence and minibuffer not active,
       start echoing if enough time elapses.  */
--- 2682,2689 ----
        goto non_reread;
      }
  
!   if (! already_idle)
!     timer_start_idle ();
  
    /* If in middle of key sequence and minibuffer not active,
       start echoing if enough time elapses.  */
***************
*** 2749,2755 ****
        c = read_char_x_menu_prompt (nmaps, maps, prev_event, used_mouse_menu);
  
        /* Now that we have read an event, Emacs is not idle.  */
!       timer_stop_idle ();
  
        goto exit;
      }
--- 2753,2760 ----
        c = read_char_x_menu_prompt (nmaps, maps, prev_event, used_mouse_menu);
  
        /* Now that we have read an event, Emacs is not idle.  */
!       if (!already_idle)
! 	timer_stop_idle ();
  
        goto exit;
      }
***************
*** 2931,2937 ****
  
   non_reread:
  
!   timer_stop_idle ();
    RESUME_POLLING;
  
    if (NILP (c))
--- 2936,2943 ----
  
   non_reread:
  
!   if (!already_idle)
!     timer_stop_idle ();
    RESUME_POLLING;
  
    if (NILP (c))
***************
*** 2970,2976 ****
  	   prevents automatic window selection (under
  	   mouse_autoselect_window from acting as a real input event, for
  	   example banishing the mouse under mouse-avoidance-mode.  */
! 	timer_resume_idle ();
  
        /* Resume allowing input from any kboard, if that was true before.  */
        if (!was_locked)
--- 2976,2983 ----
  	   prevents automatic window selection (under
  	   mouse_autoselect_window from acting as a real input event, for
  	   example banishing the mouse under mouse-avoidance-mode.  */
! 	if (!already_idle)
! 	  timer_resume_idle ();
  
        /* Resume allowing input from any kboard, if that was true before.  */
        if (!was_locked)
***************
*** 3170,3177 ****
  
        show_help_echo (help, window, object, position, 0);
  
!       /* We stopped being idle for this event; undo that.  */
!       timer_resume_idle ();
        goto retry;
      }
  
--- 3177,3185 ----
  
        show_help_echo (help, window, object, position, 0);
  
!       /* If we stopped being idle for this event, undo that.  */
!       if (!already_idle)
! 	timer_resume_idle ();
        goto retry;
      }

Re: sit-for and idle timers

[Chong Yidong]

> Why do these timer functions call sit-for?  It is a strange thing for
> a timer to wait.  It should reschedule itself instead.  What are they
> really trying to do?

It varies from situation to situation.  For example, jit-lock uses the
idle timer function jit-lock-stealth-fontify, which calls sit-for.
The goal, in that case, is to wait a certain amount of time between
fontifying chunks.

>     I think the solution is to avoid calling timer_start_idle when
>     read-event is given a non-nil SECONDS argument.  What do people think?
>
> That is definitely not right.  Emacs really is idle when it reads an event,
> even if there is a timeout.
>
> So my conclusion is that when read-event is called from an idle timer,
> it should not change the state to idle at the beginning, and it should
> not change the state away from idle at the end.
>
> Does this change fix it?

I already checked in a different change to keyboard.c before your
email came in.  But I think your version makes more sense.

Re: sit-for and idle timers

[Chong Yidong]

>> That is definitely not right.  Emacs really is idle when it reads an event,
>> even if there is a timeout.
>>
>> So my conclusion is that when read-event is called from an idle timer,
>> it should not change the state to idle at the beginning, and it should
>> not change the state away from idle at the end.
>
> I already checked in a different change to keyboard.c before your
> email came in.  But I think your version makes more sense.

I just thought of one inconsistency, though.  Suppose Emacs is not
idle, and is running Lisp code that calls sit-for.  With your change,
this starts idle timers:

!   if (! already_idle)
!     timer_start_idle ();

This is inconsistent with the old (built-in) behavior of sit-for,
which did not activate idle timers while waiting either.  The
reasoning, I think, is that `sit-for' means for the Lisp code to
"spin" for that period of time or until input arrives, which is not
the same as idling.  (This is similar to why `sleep-for' does not run
idle timers.)

If this is the intended behavior, then read-event with a timeout
should not start idle timers, since it only exists to be called by
sit-for.  We could simply document this behavior in the lispref
manual.

Re: sit-for and idle timers

[Richard Stallman]

    This is inconsistent with the old (built-in) behavior of sit-for,
    which did not activate idle timers while waiting either.

Ok, I am convinced.  Could you implement it?

Meanwhile, before the call at line 1494 of keyboard.c, something ought
to make Emacs idle.

Re: sit-for and idle timers

[Chong Yidong]

Richard Stallman <rms@gnu.org> writes:

>     This is inconsistent with the old (built-in) behavior of sit-for,
>     which did not activate idle timers while waiting either.
>
> Ok, I am convinced.  Could you implement it?

Done.  I also updated the lispref manual to clarify this issue.

> Meanwhile, before the call at line 1494 of keyboard.c, something ought
> to make Emacs idle.

You mean something like this?

  /* Bind inhibit-quit to t so that C-g gets read in
     rather than quitting back to the minibuffer.  */
  int count  = SPECPDL_INDEX ();
  specbind (Qinhibit_quit, Qt);

  timer_start_idle ();            
  sit_for (Vminibuffer_message_timeout, 0, 2);
  timer_stop_idle ();

  /* Clear the echo area.  */
  message2 (0, 0, 0);

I don't really see why it's important (it's currently just a two
second pause in which idle timers don't run), and I don't know the
code well enough to know if it will break anything horribly.

Re: sit-for and idle timers

[Richard Stallman]

    You mean something like this?

      /* Bind inhibit-quit to t so that C-g gets read in
	 rather than quitting back to the minibuffer.  */
      int count  = SPECPDL_INDEX ();
      specbind (Qinhibit_quit, Qt);

      timer_start_idle ();            
      sit_for (Vminibuffer_message_timeout, 0, 2);
      timer_stop_idle ();

Yes, except does it need timer_stop_idle?
It is going to read input straightaway after.

    I don't really see why it's important (it's currently just a two
    second pause in which idle timers don't run), and I don't know the
    code well enough to know if it will break anything horribly.

It isn't tremendously important, but it seems correct for Emacs'
idleness to begin when the command finishes.

If you see a risk, I don't insist on doing this now.
How about if you install it in the unicode-2 branch?

to be idle as soon as 

Re: sit-for and idle timers

[Chong Yidong]

Richard Stallman <rms@gnu.org> writes:

> If you see a risk, I don't insist on doing this now.
> How about if you install it in the unicode-2 branch?

OK.