bug#53432: [PATCH] Avoid losing keyboard input when inotify is too busy [and 1 more messages]

[Michael Albinus <michael.albinus@gmx.de>]

Ian Jackson <ijackson@chiark.greenend.org.uk> writes:

> Hi.  Thanks for your really helpful reply!

Hi Ian,

> I guess we probably want a "file notifications might have been missed"
> callback.  Having it be a callback like a normal file event callback
> will probably make it easiest to deal with.

Yep.

> Another possibility would be simply to fire every file notification
> event with a dummy set of information.  But that would probably
> involve changing a lot more code.

Yes, and it might also be a performance degradation.

>> For this, I have no opinion. However, I remember vaguely that Stefan
>> said once that we shall not (mis-)use the keyboard buffer for incoming
>> events, like from file notification or from D-Bus. Another mechanism is
>> needed. This idea hasn't been implemented.
>
> Hrm.  I don't think I have an opinion about this.  But I think the
> design with a fixed size buffer, implying that input to the buffer
> needs to be suspended and resumed, means that *every* piece of code
> that might write into this buffer must also participate in the flow
> control mechanism.
>
> I don't think separating out the input buffers would remove that
> requirement.  Ie, the ability to cope correctly with a flood of events
> (of whatever kind) will remain necessary.

Yes. But if we divide the keyboard buffer (also good for mouse events
and other events which do not harm) from the D-Bus and file notification
events, such a check is needed at fewer places.

>> The following packages inject events via the keyboard buffer:
>> dbusbind.c, gfilenotify.c, inotify.c, kqueue.c, w32notify.c. All of them
>> could see a burst of incoming events. It isn't just an inotify problem.
>
> Yes.  How alarming.  Well, this is a bigger problem.
>
> From my personal point of view I have effort available to fix inotify
> and kqueue, provided we can agree a way forward.  I looked at kqueue.c
> and it writes only file notification events to the buffer, so the same
> "file notifications possibly missed" callback would suffice.

Yes. Same also for dbusbind.c and gfilenotify.c. For w32notify.c I'm not
sure, but likely also the same situation.

> But I think the keyboard buffer suspend/resume, with
> event-generating-code specific callbacks for suspend
> ("hold_keyboard_input" etc. calls in "kbd_buffer_store_buffered_event")
> and resume (in "kbd_buffer_get_event") is not a terrible design.
>
> I think every one of these other event sources ought to be able to
> partake of this scheme (adding their own hooks like I proposed for
> inotify) without undue difficulty.

Yes. But I believe we don't need to handle lost events. If we have a
mean to inform the upper libraries, filenotify.el and dbus.el, about
this case, it would be sufficient. A simple check whether the event
buffer is full.

> The alternative would seem to be some kind of dynamic registration
> system.  As I say, splitting the event input buffer into "keyboard"
> and "other things" doesn't seem like it would obviate the need for
> this plubming.

The advantage of splitting into "keyboard" and "other things" buffers
would be, that the keyboard buffer doesn't overrun, whatever burst of
D-Bus or file notification events arrives.

> I was imagining a callback hook into the inotify code for "keyboard
> buffer has become empty", which would check for "have we set the flag
> for lost events".  If the flag was set, it would drain the inotify fd,
> make *one* "lost events" callback, and start reading the inotify fd
> again.
>
> The result would be that if any of the buffers overflowed, we would
> stop processing inotify events altogether until emacs has caught up
> with the user's keyboard input, and then (via the file notification
> "maybe lost" callback, do one check on every file which we might want
> to revert).

I don't believe that the native backends, like inotify.c, deserve too
much intelligence. They shall do stupid event receiving and reporting,
with a callback invoked in case of problems. This callback must be
clever then.

>> > I definitely think we want to get (back) to the point where choosing
>> > the keyboard buffer size can be done purely with respect to
>> > performance considerations rather than worrying about lossage.
>>
>> As said above: perhaps it isn't the best idea to handle such events via
>> the keyboard buffer.
>
> Would it be OK to postpone splitting this out ?  I don't think
> splitting this up is necessary to fix these lost notifications.

Not to fix lost events. But it helps to fix lost keyboard strokes.

> I think I will let experts on glib, and the glib code in emacs, handle
> this.
>
> I guess if I'm fixing this for inotify, I should at least add a
> comment next to kbd_buffer_store_event saying "you may not call this
> unless you participate in the flow control protocol" and add FIXMEs to
> the call sites that currently don't.
>
> All of those use cases will have the practical consequences of these
> bugs reduced by using a bigger buffer.
>
> I guess the next thing I should do is go and read the file
> notification lisp code to see how a "please do a one-off poll"
> callback could be implemented.

Hmm. We are still in different camps about the approach ...

> Ian.

Best regards, Michael.