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

[Ian Jackson]

Eli Zaretskii writes ("Re: bug#53432: [PATCH] Avoid losing keyboard input when 
inotify is too busy"):
> Thanks.  I doubt that we want to complicate our input handling this
> way on behalf of "abusing" inotify.  File notifications are known not
> to be scalable enough to support global-auto-revert-mode well.

The problem is that keystrokes (and file notifications) get lost.  I
don't think we should be shipping code that can do that.  Even if it
only does it if you are working on a program whose build system writes
a lot of files, and/or have a slow computer, and/or are unlucky.  Note
that magit turns on global-auto-revert-mode so this is a very common
configuration.  Loss of file updates is also a real problem, becuase
it can lead to the user editing an old version of a file.

The fact that I have to "abuse" things to demonstrate the race, does
not mean that it's not a real bug.  That's in the nature of race bugs.

It's probably happening to the occasional user right now, albeit very
rarely, and they probably think they just didn't hit the key properly
or something.  I really dislike the idea that emacs is making our
users doubt themselves, by pretending the user didn't hit a key.  I
don't think it's good enough for this to be "rare" or "only in unusual
situations" or "only when the computer is overloaded".  We expect
emacs to be reliable and we need to make it so.

I'm obviously open to other suggestions for how to deal with this to
make sure that both keystrokes and file updates are not *lost*, only
possibly *delayed*.  I don't really know the code so it is entirely
plausible that my approach is wrong.


Po Lu writes ("Re: bug#53432: [PATCH] Avoid losing keyboard input when inotify 
is too busy"):
> I would rather not allocate anything on the heap to hold inotify events.

I'm not sure what your underlying objection is to this, so I will
guess: I think you are concerned at the performance implications of
allocating each time we handle inotify events.  But my proposed code
does not allocate each time.  It only (re)allocates when it needs a
bigger buffer than it already has.

Of course the unbounded memory consumption is a problem, as I write in
my FIXME.  Allocating an undounded amount of memory on the stack each
time, as the current code does, is really very brave and seems to me
clearly worse than doing it amortised-approximately-once on the heap.
(But maybe SAFE_ALLOCA falls back to malloc for big requests anyway.)

> The best thing to do, IMO, would be to simply not store any
> FILE_NOTIFY_EVENTS if the keyboard buffer is full, which shouldn't
> affect normal use of file notifications, and would result in less change
> to the keyboard code.

Wouldn't that lead to file notifications getting lost ?  I think that
might result in buffers not being reverted, even though the user has
global-auto-revert-mode enabled.  As I say that would be a problem,
because it can end up with the user editing the wrong version of a
file.  I don't think it is OK.

If it is OK for file notifications to get lost, because the need for a
buffer revert will be picked up another way somehow, then your
suggestion makes perfect sense to me.  But in that case, I don't
understand why this code doesn't use a buffer of fixed size (or, at
least, limited size).

Or to put it another way, the current code does a hair-raising thing
for which the only explanation I could think of was that it is aiming
never to lose notifications; and if there's a comment somewhere saying
that the whole inotify system is best-effort, and it's OK for it to
drop things when stressed, I have missed it.

If it is OK for it to be best-effort, then I think this needs to be in
the documentation for inotify-add-watch.  Currently it says

  The watch will look for ASPECT events and
  invoke CALLBACK when an event occurs.

and there's nothing saying "it might not happen, so you must arrangee
that this is not the sole way you are looking for changes".  I think
that'd be an API which would invite programmers to write and ship lost
event bugs, especially since usually it would work just fine.  But
maybe it would e OK if it is almost always used only with expert
reivew (eg, by code in other parts of emacs).


Eli:
> In any case, installing this on the emacs-28 branch is out of the
> question: it's too late for that.  We may install some variant of this
> on master, after discussing how best to handle this.  Po Lu's
> suggestion to stop processing inotify events sounds better to me.

I was advised by a friend that I ought to send patches against
the emacs-28 branch, so I did that.  Indeed, CONTRIBUTING says

  If you are fixing a bug that exists in the current release, you should
  generally commit it to the release branch;

Anyway my current patches rebase cleanly from 27 all the way to
master.  It seems very likely that'll be true for whatever fix we come
up with.  So the fix can be forward- or back-ported as people like.


> [Ian:]
> > I hope the commit messages are in the expected format.  I think I
> > probably have GNU copyright paperwork on file already, perhaps under a
> > different email address.
> 
> I don't see your name or email address on file, so maybe they are both
> different?

My name hasn't changed.  But I'm happy to do the paperwork (even if it
would be "again").  If it did happen, it will have been a long long
time ago.

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.