read_char() does not detect, handle special-event-map buffer changes

read_char() does not detect, handle special-event-map buffer changes

[Derek Upham]

There is an Emacs package that uses process buffers to communicate with a
spawned worker process.  Due to locking on underlying files, this limits
me to one worker, and hence one Emacs.  I'm trying to add D-Bus support
to both sides, and have come across a bug in Emacs' support for special
events.

The package includes a chunk of code that:

1. Brings up a temporary buffer in a visible window.
2. Queries the worker through the process buffer channel.
3. Handles the worker's response in a process filter handler, putting
   the text into a new buffer and replacing the temporary buffer in the
   same window.

The D-Bus code attempts to do the exact same steps, and appears to
succeed.  But the first event after displaying the response (usually a
keystroke) goes to the /temporary buffer/, generating a "buffer
read-only" error.  After the error, the command loop resyncs the current
buffer to the visible buffer and later events work normally.

I ran Emacs 24.2.2 in GDB and found that the read_char() function has
code that is supposed to detect this case:

      if (current_buffer != prev_buffer)
        {
          /* The command may have changed the keymaps.  Pretend there
             is input in another keyboard and return.  This will
             recalculate keymaps.  */
          c = make_number (-2);
          goto exit;
        }
      else
        goto retry;

However, `current_buffer' and `prev_buffer' are showing up as the same
in the debugger.  I think this is because we haven't gone through a
display refresh at this point in the code; the Emacs window still shows
the temporary buffer, for example.  This specific error case affects
D-Bus, but any similar activity by a special event handler should show
the same bug.

I have a fix that seems to work: remove the test and assume that any
special event handler could have changed the keymaps.

      /* The command may have changed the keymaps.  Pretend there
         is input in another keyboard and return.  This will
         recalculate keymaps.  */
      c = make_number (-2);
      goto exit;

This removes a potential optimization, as the code goes up to a higher
level before restarting `read_char'.  But looking at the list of
special events (in `special-event-map'), those special events should be
infrequent enough that this change won't cause a performance impact.
Does anyone know of a reason not to make this change?

Thanks,

Derek

-- 
Derek Upham
sand@blarg.net

Re: read_char() does not detect, handle special-event-map buffer changes

[Derek Upham]

Here's a small repro case for the problem.  Start up two instances of
Emacs, and load this file into both.  In the first, run
`bad-keymap-startup-server'.  In the second, run `bad-keymap-send-signal',
and immediately switch back to the first Emacs.

The first Emacs will switch to a "*foo*" buffer with some dummy text.
That buffer will be in view mode.  If you hit `q' after the buffer pops
up, Emacs will complain about "*foo*" being read-only.  Hit `q' again and
the buffer disappears.

Kill the "*foo*" buffer and run the test again.  This time, instead of
hitting `q', use `C-n' to move down a line.  After that, when you hit
`q' you exit view mode immediately.

In the original code, I had theorized that the event was going to the
read-only temporary buffer, which had no binding for `q'.  But if that
were the case, we would see different behavior when the "old" buffer is
the writable "bad-keymap.el".  The "*foo*" buffer is getting the `q'
event, but it looks like read_char() has not started using the buffer's
new `view-mode' keymap, due to the way we are jumping to "retry:".  The
proposed fix to exit read_char() after every special event gives the I/O
system a chance to see the new keymap.

Derek

------------------------- bad-keymap.el -------------------------

(require 'dbus)

(defun bad-keymap-switch-handler ()
  (let* ((buf (get-buffer-create "*foo*")))
    (with-current-buffer buf
      (let ((inhibit-read-only t))
	(erase-buffer)
	(insert "foo foo foo\n")
	(switch-to-buffer buf)
	(goto-char (point-min)))
      (view-mode))))

(defun bad-keymap-startup-server ()
  (interactive)
  (dbus-register-signal :session "bad-keymap" "/bad/keymap" "bad.keymap"
                        "Switch" 'bad-keymap-switch-handler))

(defun bad-keymap-send-signal ()
  (interactive)
  (sit-for 5)
  (dbus-send-signal :session "bad-keymap" "/bad/keymap" "bad.keymap"
                    "Switch"))

-----------------------------------------------------------------


Derek Upham writes:
> There is an Emacs package that uses process buffers to communicate with a
> spawned worker process.  Due to locking on underlying files, this limits
> me to one worker, and hence one Emacs.  I'm trying to add D-Bus support
> to both sides, and have come across a bug in Emacs' support for special
> events.
>
> The package includes a chunk of code that:
>
> 1. Brings up a temporary buffer in a visible window.
> 2. Queries the worker through the process buffer channel.
> 3. Handles the worker's response in a process filter handler, putting
>    the text into a new buffer and replacing the temporary buffer in the
>    same window.
>
> The D-Bus code attempts to do the exact same steps, and appears to
> succeed.  But the first event after displaying the response (usually a
> keystroke) goes to the /temporary buffer/, generating a "buffer
> read-only" error.  After the error, the command loop resyncs the current
> buffer to the visible buffer and later events work normally.
>
> I ran Emacs 24.2.2 in GDB and found that the read_char() function has
> code that is supposed to detect this case:
>
>       if (current_buffer != prev_buffer)
>         {
>           /* The command may have changed the keymaps.  Pretend there
>              is input in another keyboard and return.  This will
>              recalculate keymaps.  */
>           c = make_number (-2);
>           goto exit;
>         }
>       else
>         goto retry;
>
> However, `current_buffer' and `prev_buffer' are showing up as the same
> in the debugger.  I think this is because we haven't gone through a
> display refresh at this point in the code; the Emacs window still shows
> the temporary buffer, for example.  This specific error case affects
> D-Bus, but any similar activity by a special event handler should show
> the same bug.
>
> I have a fix that seems to work: remove the test and assume that any
> special event handler could have changed the keymaps.
>
>       /* The command may have changed the keymaps.  Pretend there
>          is input in another keyboard and return.  This will
>          recalculate keymaps.  */
>       c = make_number (-2);
>       goto exit;
>
> This removes a potential optimization, as the code goes up to a higher
> level before restarting `read_char'.  But looking at the list of
> special events (in `special-event-map'), those special events should be
> infrequent enough that this change won't cause a performance impact.
> Does anyone know of a reason not to make this change?
>
> Thanks,
>
> Derek


-- 
Derek Upham
sand@blarg.net

Re: read_char() does not detect, handle special-event-map buffer changes

[Derek Upham]

Here's an even smaller repro case.  Eval the following in an Emacs
instance:

    (defun sigusr1-handler ()
      (interactive)
      (let* ((buf (get-buffer-create "*foo*")))
        (with-current-buffer buf
          (let ((inhibit-read-only t))
            (erase-buffer)
            (insert "foo foo foo\n")
            (switch-to-buffer buf)
            (goto-char (point-min)))
          (view-mode))))

    (define-key special-event-map [sigusr1] 'sigusr1-handler)

Then run

    sleep 5; kill -USR1 $EMACS_PID

in a separate terminal.  Move focus back to Emacs before the signal goes
out.  Hit `q' in the new buffer and Emacs will complain about "*foo*"
being read-only.  The second time you hit `q', Emacs will exit view mode
and bury the buffer.

Derek

Derek Upham writes:

> Here's a small repro case for the problem.  Start up two instances of
> Emacs, and load this file into both.  In the first, run
> `bad-keymap-startup-server'.  In the second, run `bad-keymap-send-signal',
> and immediately switch back to the first Emacs.
>
> The first Emacs will switch to a "*foo*" buffer with some dummy text.
> That buffer will be in view mode.  If you hit `q' after the buffer pops
> up, Emacs will complain about "*foo*" being read-only.  Hit `q' again and
> the buffer disappears.
>
> Kill the "*foo*" buffer and run the test again.  This time, instead of
> hitting `q', use `C-n' to move down a line.  After that, when you hit
> `q' you exit view mode immediately.
>
> In the original code, I had theorized that the event was going to the
> read-only temporary buffer, which had no binding for `q'.  But if that
> were the case, we would see different behavior when the "old" buffer is
> the writable "bad-keymap.el".  The "*foo*" buffer is getting the `q'
> event, but it looks like read_char() has not started using the buffer's
> new `view-mode' keymap, due to the way we are jumping to "retry:".  The
> proposed fix to exit read_char() after every special event gives the I/O
> system a chance to see the new keymap.
>
> Derek
>
> ------------------------- bad-keymap.el -------------------------
>
> (require 'dbus)
>
> (defun bad-keymap-switch-handler ()
>   (let* ((buf (get-buffer-create "*foo*")))
>     (with-current-buffer buf
>       (let ((inhibit-read-only t))
> 	(erase-buffer)
> 	(insert "foo foo foo\n")
> 	(switch-to-buffer buf)
> 	(goto-char (point-min)))
>       (view-mode))))
>
> (defun bad-keymap-startup-server ()
>   (interactive)
>   (dbus-register-signal :session "bad-keymap" "/bad/keymap" "bad.keymap"
>                         "Switch" 'bad-keymap-switch-handler))
>
> (defun bad-keymap-send-signal ()
>   (interactive)
>   (sit-for 5)
>   (dbus-send-signal :session "bad-keymap" "/bad/keymap" "bad.keymap"
>                     "Switch"))
>
> -----------------------------------------------------------------
>
>
> Derek Upham writes:
>> There is an Emacs package that uses process buffers to communicate with a
>> spawned worker process.  Due to locking on underlying files, this limits
>> me to one worker, and hence one Emacs.  I'm trying to add D-Bus support
>> to both sides, and have come across a bug in Emacs' support for special
>> events.
>>
>> The package includes a chunk of code that:
>>
>> 1. Brings up a temporary buffer in a visible window.
>> 2. Queries the worker through the process buffer channel.
>> 3. Handles the worker's response in a process filter handler, putting
>>    the text into a new buffer and replacing the temporary buffer in the
>>    same window.
>>
>> The D-Bus code attempts to do the exact same steps, and appears to
>> succeed.  But the first event after displaying the response (usually a
>> keystroke) goes to the /temporary buffer/, generating a "buffer
>> read-only" error.  After the error, the command loop resyncs the current
>> buffer to the visible buffer and later events work normally.
>>
>> I ran Emacs 24.2.2 in GDB and found that the read_char() function has
>> code that is supposed to detect this case:
>>
>>       if (current_buffer != prev_buffer)
>>         {
>>           /* The command may have changed the keymaps.  Pretend there
>>              is input in another keyboard and return.  This will
>>              recalculate keymaps.  */
>>           c = make_number (-2);
>>           goto exit;
>>         }
>>       else
>>         goto retry;
>>
>> However, `current_buffer' and `prev_buffer' are showing up as the same
>> in the debugger.  I think this is because we haven't gone through a
>> display refresh at this point in the code; the Emacs window still shows
>> the temporary buffer, for example.  This specific error case affects
>> D-Bus, but any similar activity by a special event handler should show
>> the same bug.
>>
>> I have a fix that seems to work: remove the test and assume that any
>> special event handler could have changed the keymaps.
>>
>>       /* The command may have changed the keymaps.  Pretend there
>>          is input in another keyboard and return.  This will
>>          recalculate keymaps.  */
>>       c = make_number (-2);
>>       goto exit;
>>
>> This removes a potential optimization, as the code goes up to a higher
>> level before restarting `read_char'.  But looking at the list of
>> special events (in `special-event-map'), those special events should be
>> infrequent enough that this change won't cause a performance impact.
>> Does anyone know of a reason not to make this change?
>>
>> Thanks,
>>
>> Derek


-- 
Derek Upham
sand@blarg.net

Re: read_char() does not detect, handle special-event-map buffer changes

[Stefan Monnier]

> in a separate terminal.  Move focus back to Emacs before the signal goes
> out.  Hit `q' in the new buffer and Emacs will complain about "*foo*"
> being read-only.  The second time you hit `q', Emacs will exit view mode
> and bury the buffer.

I think this is a known problem: the set of active keymaps is determined
at the end of the previous command, so any change performed via
something like a special-event-map binding or a process-filter will bump
into this problem (you don't even need to switch-to-buffer, just
enabling view-mode is sufficient).

It's a bug: we should instead wait until the first key is pressed
before figuring out the active keymaps.
Problem is, this bug is in read_key_sequence, which is a pretty
scary function.

In the mean time, you can work around the bug by adding to
unread-command-event (from your sigusr1-handler) a dummy event that is
bound in global-map to something like `ignore'.


        Stefan

Re: read_char() does not detect, handle special-event-map buffer changes

[Derek Upham]

Stefan Monnier writes:
>> in a separate terminal.  Move focus back to Emacs before the signal goes
>> out.  Hit `q' in the new buffer and Emacs will complain about "*foo*"
>> being read-only.  The second time you hit `q', Emacs will exit view mode
>> and bury the buffer.
>
> I think this is a known problem: the set of active keymaps is determined
> at the end of the previous command, so any change performed via
> something like a special-event-map binding or a process-filter will bump
> into this problem (you don't even need to switch-to-buffer, just
> enabling view-mode is sufficient).
>
> It's a bug: we should instead wait until the first key is pressed
> before figuring out the active keymaps.
> Problem is, this bug is in read_key_sequence, which is a pretty
> scary function.
>
> In the mean time, you can work around the bug by adding to
> unread-command-event (from your sigusr1-handler) a dummy event that is
> bound in global-map to something like `ignore'.
>
>
>         Stefan

Did you see my comment in the original email?  read_char() is already
trying to detect changed keymaps.

      if (current_buffer != prev_buffer)
        {
          /* The command may have changed the keymaps.  Pretend there
             is input in another keyboard and return.  This will
             recalculate keymaps.  */
          c = make_number (-2);
          goto exit;
        }
      else
        goto retry;

The bug is happening because the test is flawed: current_buffer and
prev_buffer are the same, so Emacs doesn't think it needs to recalculate
anything.  It loops back to the top of read_char() and reads another
character with the same keymap.

Removing the retry case and exiting every time fixes the problem:

      /* The command may have changed the keymaps.  Pretend there
         is input in another keyboard and return.  This will
         recalculate keymaps.  */
      c = make_number (-2);
      goto exit;

This removes a flawed optimization and returns a documented value.  It
doesn't touch read_key_sequence, so that risk goes away.  If we were
getting special events at a high rate of speed this /might/ cause a
slowdown, but nothing in the special events table seems to be used that
way---and I expect that the extra time spent popping back up to
read_key_sequence for the retry will still be very fast compared to the
time spent in the Emacs Lisp callback.

Derek

-- 
Derek Upham
sand@blarg.net

Re: read_char() does not detect, handle special-event-map buffer changes

[Michael Albinus]

Derek Upham <sand@blarg.net> writes:

> This removes a flawed optimization and returns a documented value.  It
> doesn't touch read_key_sequence, so that risk goes away.  If we were
> getting special events at a high rate of speed this /might/ cause a
> slowdown, but nothing in the special events table seems to be used that
> way---and I expect that the extra time spent popping back up to
> read_key_sequence for the retry will still be very fast compared to the
> time spent in the Emacs Lisp callback.

I don't know what you mean with "high rate of speed", but special events
are used for D-Bus and for file notifications. There could be a burst of
incoming events, temporarily.

> Derek

Best regards, Michael.

Re: read_char() does not detect, handle special-event-map buffer changes

[Derek Upham]

Michael Albinus writes:

> Derek Upham <sand@blarg.net> writes:
>
>> This removes a flawed optimization and returns a documented value.  It
>> doesn't touch read_key_sequence, so that risk goes away.  If we were
>> getting special events at a high rate of speed this /might/ cause a
>> slowdown, but nothing in the special events table seems to be used that
>> way---and I expect that the extra time spent popping back up to
>> read_key_sequence for the retry will still be very fast compared to the
>> time spent in the Emacs Lisp callback.
>
> I don't know what you mean with "high rate of speed", but special events
> are used for D-Bus and for file notifications. There could be a burst of
> incoming events, temporarily.

A couple of emails back I put in a D-Bus handler for asynchronous
messages.  I'll set it up to do nothing but push a timestamp onto a
list.  Then with a couple of dbus-send instances I can look for rate
differences between the two code paths.  I should have numbers this
weekend.

Derek

-- 
Derek Upham
sand@blarg.net

Re: read_char() does not detect, handle special-event-map buffer changes

[Michael Albinus]

Derek Upham <sand@blarg.net> writes:

> A couple of emails back I put in a D-Bus handler for asynchronous
> messages.  I'll set it up to do nothing but push a timestamp onto a
> list.  Then with a couple of dbus-send instances I can look for rate
> differences between the two code paths.  I should have numbers this
> weekend.

Would be interesting. Alternatively, you could also stress via file
notifications. Eval

(defun my-inotify-handler (&rest args)
  (message (format "Here I am '%s'" args)))
(inotify-add-watch "/tmp" t 'my-inotify-handler)

And in a shell outside Emacs, you call

# while /bin/true; do touch /tmp/123; done

> Derek

Best regards, Michael.

Re: read_char() does not detect, handle special-event-map buffer changes

[Derek Upham]

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


Michael Albinus writes:

> Derek Upham <sand@blarg.net> writes:
>
>> A couple of emails back I put in a D-Bus handler for asynchronous
>> messages.  I'll set it up to do nothing but push a timestamp onto a
>> list.  Then with a couple of dbus-send instances I can look for rate
>> differences between the two code paths.  I should have numbers this
>> weekend.
>
> Would be interesting. Alternatively, you could also stress via file
> notifications. Eval
>
> (defun my-inotify-handler (&rest args)
>   (message (format "Here I am '%s'" args)))
> (inotify-add-watch "/tmp" t 'my-inotify-handler)
>
> And in a shell outside Emacs, you call
>
> # while /bin/true; do touch /tmp/123; done

Neither my stock Emacs or experimental Emacs have inotify support, so I
went with D-Bus.  I fired off four processes in parallel, each spawning
in sequence 1001 dbus-send instances sending one signal.  The processes
ran for just over 5 seconds, so Emacs was handling around 800 signals
per second.

 | Metric                  |   Stock | Experimental |
 |-------------------------+---------+--------------|
 | average interval (usec) | 1249.68 |       1255.1 |
 | sum of intervals (usec) | 5002463 |      5024162 |

A graph shows essentially the same clustering for the two runs.  They
are both pretty steady.


[-- Attachment #2: interval times --]
[-- Type: image/png, Size: 51926 bytes --]

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


Derek

-- 
Derek Upham
sand@blarg.net

Re: read_char() does not detect, handle special-event-map buffer changes

[Stefan Monnier]

> Did you see my comment in the original email?  read_char() is already
> trying to detect changed keymaps.

>       if (current_buffer != prev_buffer)
>         {
>           /* The command may have changed the keymaps.  Pretend there
>              is input in another keyboard and return.  This will
>              recalculate keymaps.  */
>           c = make_number (-2);
>           goto exit;
>         }
>       else
>         goto retry;

> The bug is happening because the test is flawed: current_buffer and
> prev_buffer are the same, so Emacs doesn't think it needs to recalculate
> anything.  It loops back to the top of read_char() and reads another
> character with the same keymap.

I know, but I'm still weary of the possible side-effects.  The change
I was proposing (to delay computation of the set of active keymaps to
after we got the first event) also has the advantage that it is not
specific to special-event-map, so it's desirable in any case.

> This removes a flawed optimization and returns a documented value.

The main issue is that it's not clear what should happen if you switch
buffers in the middle (rather than before the beginning) of
a key sequence.  And I'd rather not have to think about it ;-)

> It doesn't touch read_key_sequence, so that risk goes away.

I've just installed a patch (in trunk) which touches read_key_sequence
intrusively (in case you still had doubts whether I was a masochist).
This should hopefully fix your problem (at least it fixes the
sigusr1-handler test case you sent).  It also cleans up the code a bit.
Hopefully it doesn't introduce too many new problems.


        Stefan

Re: read_char() does not detect, handle special-event-map buffer changes

[Derek Upham]

Stefan Monnier writes:
> I've just installed a patch (in trunk) which touches read_key_sequence
> intrusively (in case you still had doubts whether I was a masochist).
> This should hopefully fix your problem (at least it fixes the
> sigusr1-handler test case you sent).  It also cleans up the code a bit.
> Hopefully it doesn't introduce too many new problems.

Thank you.  The change works for my full app.  What version number will trunk
become?  24.4?

Derek

-- 
Derek Upham
sand@blarg.net

Re: read_char() does not detect, handle special-event-map buffer changes

[Stefan Monnier]

>> I've just installed a patch (in trunk) which touches read_key_sequence
>> intrusively (in case you still had doubts whether I was a masochist).
>> This should hopefully fix your problem (at least it fixes the
>> sigusr1-handler test case you sent).  It also cleans up the code a bit.
>> Hopefully it doesn't introduce too many new problems.
> Thank you.  The change works for my full app.

Thank you for confirming it fixes the problem for you.

> What version number will trunk become?  24.4?

Yes.


        Stefan