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

[Ian Jackson <ijackson@chiark.greenend.org.uk>]

Hi.  Thanks for your really helpful reply!

Michael Albinus writes ("Re: bug#53432: [PATCH] Avoid losing keyboard input when inotify is too busy [and 1 more messages]"):
> Ian Jackson <ijackson@chiark.greenend.org.uk> writes:
> > I suggest the following approach for stable branches:
> >
> >  1. Have the inotify code drop notifications if kbd_on_hold_p().
> >     This will mean that keystrokes will never be lost.
> 
> I agree. But then, inotify (and the other file notification backends)
> shall inform that this happened. Either by an error signal, or by
> calling a handler.

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.

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.

> 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.

> > If desired, the increased keyboard buffer size could be made conditional on
> > inotify support so that systems which don't use the keyboard buffer
> > for file notifications don't pay a price.
> 
> 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.

I don't think my emacs (--with-toolkit=lucid) participates in dbus,
glib, or w32notify.  Of course whatever scheme we come up with ought
to be structured so as to work for those.

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.

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.

> Yep. When the file notification backend reports a loss of event, the
> autorevert package might fall back to polling. Perhaps we could even
> raise another signal that things are back to normal, and packages like
> autorevert could switch back to file notification.

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 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.

> > I'm not familiar with glib.  Presuambly glib either exposes something
> > with the semantics of IN_Q_OVERFLOW, or internally converts that to a
> > notification to every watcher.  Otherwise programs using glib's API
> > would have to poll, which seems unlikely - I think that the glib
> > authors will be well aware of the desire to avoid polling for battery
> > life reasons.
> 
> glib uses several native backends, like inotify or kqueue. It has also a
> backend which polls. However, I don't know whether it switches the
> backend during runtime.

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.

Ian.

-- 
Ian Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.  

Pronouns: they/he.  If I emailed you from @fyvzl.net or @evade.org.uk,
that is a private address which bypasses my fierce spamfilter.