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

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

[Ian Jackson]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1342 bytes --]

I experienced a bug where my Emacs (Debian's emacs-lucid) would lose
keystrokes under pathological conditions.  I tracked this down to the
keyboard input buffer filling up with inotify events.

The attached patches have been rebased onto emacs-28 (fbc9b121e062)
and I have tested before-and-after versions in parallel to verify that
the bug appears in the upstream branch and has been fixed with my
patches.

The bug was really quite severe in my (rather abusive) situation, with
Debian's emacs-lucid 26.1.  With the upstream emacs-28 branch it
requires more effort, and loses fewer keys.  I suspect that some more
sophisticated approach to inotify requests means it's harder to get
the buffer to fill up with the newer version.  Unfortunately I am not
able to share my whole repro, but a key part is to visit the output
file from this
  while yes | dd of=t bs=1 count=1000000; do : ;
with global-auto-revert-mode enabled.

With my patches it is still possible to get emacs to perform very
poorly by abusing it this way, but at least all the keys arrive
eventually.  I added some FIXMEs for further work that would be good,
but is not critical to fixing the bug.

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.

Regards,
Ian.


[-- Attachment #2: 0001-inotify.c-Document-hazard-of-unlimited-buffering.patch --]
[-- Type: application/octet-stream, Size: 939 bytes --]

From a90b9c3c54eabe4e9429911417a8f3196b9036c2 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ijackson@chiark.greenend.org.uk>
Date: Fri, 21 Jan 2022 20:28:05 +0000
Subject: [PATCH 1/4] * inotify.c: Document hazard of unlimited buffering

---
 src/inotify.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/inotify.c b/src/inotify.c
index e92ad40abc..b676ea216a 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -316,6 +316,9 @@ inotify_callback (int fd, void *_)
   if (ioctl (fd, FIONREAD, &to_read) < 0)
     report_file_notify_error ("Error while retrieving file system events",
 			      Qnil);
+  // FIXME: feeding the value from FIONREAD to a memory allocator is unwise
+  // Sadly the inotify event buffer parsing algorithm is not capable of
+  // handling the case where we read only part of a kernel event.
   USE_SAFE_ALLOCA;
   char *buffer = SAFE_ALLOCA (to_read);
   ssize_t n = read (fd, buffer, to_read);
-- 
2.20.1


[-- Attachment #3: 0002-inotify.c-Use-a-buffer-from-the-heap.patch --]
[-- Type: application/octet-stream, Size: 2382 bytes --]

From 6a5dd5766a5806258d665cab343a103b5705bf32 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ijackson@chiark.greenend.org.uk>
Date: Fri, 21 Jan 2022 19:42:43 +0000
Subject: [PATCH 2/4] * inotify.c: Use a buffer from the heap

We are going to need this buffer to be persistent, because we may need
to stop processing it halfway through (if the main input event buffer
fills up).

Also, getting a value from FIONREAD and then feeding it to alloca is
even less wise than feeding it to malloc.
---
 src/inotify.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/src/inotify.c b/src/inotify.c
index b676ea216a..ad02164b37 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -43,6 +43,9 @@ Copyright (C) 2012-2022 Free Software Foundation, Inc.
 /* File handle for inotify.  */
 static int inotifyfd = -1;
 
+static size_t inotify_buffer_len = 0;
+static char *inotify_buffer = NULL;
+
 /* Alist of files being watched.  We want the returned descriptor to
    be unique for every watch, but inotify returns the same descriptor
    WD for multiple calls to inotify_add_watch with the same file.
@@ -319,9 +322,13 @@ inotify_callback (int fd, void *_)
   // FIXME: feeding the value from FIONREAD to a memory allocator is unwise
   // Sadly the inotify event buffer parsing algorithm is not capable of
   // handling the case where we read only part of a kernel event.
-  USE_SAFE_ALLOCA;
-  char *buffer = SAFE_ALLOCA (to_read);
-  ssize_t n = read (fd, buffer, to_read);
+  if (to_read > inotify_buffer_len)
+    {
+      inotify_buffer = xrealloc(inotify_buffer, to_read);
+      inotify_buffer_len = to_read;
+    }
+
+  ssize_t n = read (fd, inotify_buffer, to_read);
   if (n < 0)
     report_file_notify_error ("Error while reading file system events", Qnil);
 
@@ -331,7 +338,7 @@ inotify_callback (int fd, void *_)
 
   for (ssize_t i = 0; i < n; )
     {
-      struct inotify_event *ev = (struct inotify_event *) &buffer[i];
+      struct inotify_event *ev = (struct inotify_event *) &inotify_buffer[i];
       Lisp_Object descriptor = INT_TO_INTEGER (ev->wd);
       Lisp_Object prevtail = find_descriptor (descriptor);
 
@@ -351,8 +358,6 @@ inotify_callback (int fd, void *_)
         }
       i += sizeof (*ev) + ev->len;
     }
-
-  SAFE_FREE ();
 }
 
 DEFUN ("inotify-add-watch", Finotify_add_watch, Sinotify_add_watch, 3, 3, 0,
-- 
2.20.1


[-- Attachment #4: 0003-inotify.c-Break-out-inotify_process_buffer.patch --]
[-- Type: application/octet-stream, Size: 1679 bytes --]

From bc46afb34524d97a354a636c476544daedf560cd Mon Sep 17 00:00:00 2001
From: Ian Jackson <ijackson@chiark.greenend.org.uk>
Date: Fri, 21 Jan 2022 19:51:25 +0000
Subject: [PATCH 3/4] * inotify.c: Break out inotify_process_buffer

We pass the buffer fullness in a global variable.  This is weird right
now, but we are going to want this to persist so it is convenient to
do it now.
---
 src/inotify.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/inotify.c b/src/inotify.c
index ad02164b37..8a3685f2f1 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -44,6 +44,7 @@ Copyright (C) 2012-2022 Free Software Foundation, Inc.
 static int inotifyfd = -1;
 
 static size_t inotify_buffer_len = 0;
+static size_t inotify_buffer_full = 0;
 static char *inotify_buffer = NULL;
 
 /* Alist of files being watched.  We want the returned descriptor to
@@ -71,6 +72,9 @@ Copyright (C) 2012-2022 Free Software Foundation, Inc.
 
 static Lisp_Object watch_list;
 
+static void
+inotify_process_buffer (void);
+
 static Lisp_Object
 mask_to_aspects (uint32_t mask)
 {
@@ -332,11 +336,18 @@ inotify_callback (int fd, void *_)
   if (n < 0)
     report_file_notify_error ("Error while reading file system events", Qnil);
 
+  inotify_buffer_full = n;
+  inotify_process_buffer ();
+}
+
+static void
+inotify_process_buffer (void)
+{
   struct input_event event;
   EVENT_INIT (event);
   event.kind = FILE_NOTIFY_EVENT;
 
-  for (ssize_t i = 0; i < n; )
+  for (ssize_t i = 0; i < inotify_buffer_full; )
     {
       struct inotify_event *ev = (struct inotify_event *) &inotify_buffer[i];
       Lisp_Object descriptor = INT_TO_INTEGER (ev->wd);
-- 
2.20.1


[-- Attachment #5: 0004-inotify-Pause-when-keyboard-input-is-suspended.patch --]
[-- Type: application/octet-stream, Size: 7206 bytes --]

From 7b94f182683ae12e3f39a82a9957239d9b35832e Mon Sep 17 00:00:00 2001
From: Ian Jackson <ijackson@chiark.greenend.org.uk>
Date: Fri, 21 Jan 2022 19:56:13 +0000
Subject: [PATCH 4/4] inotify: Pause when keyboard input is suspended

If the emacs main thread cannot process events quickly enough, the
input buffer can become full.  If that happens, events can get lost.

There is a mechanism to detect this in keyboard.c, and prevent people
from putting more stuff into the buffer, but it was not wired up to
the inotify machinery.  So heavy inotify load can cause lost
keystrokes!

Fix this.

* inotify.c: Be able to pause and resume the reading, retaining the
buffered events as necessary.
* keyboard.c: Call stop_inotify_input and resume_inotify_input.

Details of the approach are:

 - The `i` loop variable in `inotify_process_buffer` becomes a global
   variable, so that we can retain our knowledge of where we were.

 - Enabling or disabling reading from the inotify fd is now done in a
   single idempotent function which checks whether (i) input is
   suspended (ii) the buffer of unprocssed inotify events is empty.
   Happily add_read_fd and delete_read_fd are idempotent.

 - New hooks are provided by which keyboard.c tells inotify.c when
   input is being suspended and unsuspended.  Suspending input is
   straightforward.  Unsuspending it may involve processing buffered
   events (and that might cause the input to be suspended again!)

I have checked that this fixes the lost input bug, which I can
reproduce with a crazy test setup.  With my patch, when emacs is
processing too many inotify events, it can become rather unresponsive
- but when it recovers, all the events are there.

Tested-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
---
 src/inotify.c  | 57 ++++++++++++++++++++++++++++++++++++++++++++++----
 src/keyboard.c |  2 ++
 src/keyboard.h |  9 ++++++++
 3 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/src/inotify.c b/src/inotify.c
index 8a3685f2f1..97c0fd9efa 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -43,8 +43,15 @@ Copyright (C) 2012-2022 Free Software Foundation, Inc.
 /* File handle for inotify.  */
 static int inotifyfd = -1;
 
+/* Buffer is like this:
+ *   |oooo|======================|---------|
+ *   0    done                   full      len
+ * "oooo" is events we have processed into the keyboard buffer.
+ * "====" is events we have read from the kernel but not processed.
+ * "----" is spare space that was used on a previous occasion  */
 static size_t inotify_buffer_len = 0;
 static size_t inotify_buffer_full = 0;
+static size_t inotify_buffer_done = 0;
 static char *inotify_buffer = NULL;
 
 /* Alist of files being watched.  We want the returned descriptor to
@@ -74,6 +81,8 @@ Copyright (C) 2012-2022 Free Software Foundation, Inc.
 
 static void
 inotify_process_buffer (void);
+static void
+inotify_add_or_remove_reading_fd (void);
 
 static Lisp_Object
 mask_to_aspects (uint32_t mask)
@@ -319,6 +328,14 @@ remove_watch (Lisp_Object descriptor, Lisp_Object id)
 static void
 inotify_callback (int fd, void *_)
 {
+  if ( inotify_buffer_done != inotify_buffer_full )
+    {
+      inotify_add_or_remove_reading_fd ();
+      return;
+    }
+
+  // Buffer is empty.  We will reset done and full after reading into it.
+
   int to_read;
   if (ioctl (fd, FIONREAD, &to_read) < 0)
     report_file_notify_error ("Error while retrieving file system events",
@@ -337,9 +354,14 @@ inotify_callback (int fd, void *_)
     report_file_notify_error ("Error while reading file system events", Qnil);
 
   inotify_buffer_full = n;
+  inotify_buffer_done = 0;
   inotify_process_buffer ();
+  inotify_add_or_remove_reading_fd ();
 }
 
+/* Idempotently process events from our buffer of things we have read
+ * from the kernel, into the input event buffer.  Call sites should
+ * call inotify_add_or_remove_reading_fd afterwards.  */
 static void
 inotify_process_buffer (void)
 {
@@ -347,9 +369,14 @@ inotify_process_buffer (void)
   EVENT_INIT (event);
   event.kind = FILE_NOTIFY_EVENT;
 
-  for (ssize_t i = 0; i < inotify_buffer_full; )
+  // FIXME: Ideally in the future some arrangement would be made to
+  // prioritise keyboard input over inotify events, which would
+  // involve using a tighter condition than kbd_on_hold_p.
+
+  while( inotify_buffer_done < inotify_buffer_full && !kbd_on_hold_p () )
     {
-      struct inotify_event *ev = (struct inotify_event *) &inotify_buffer[i];
+      struct inotify_event *ev =
+	(struct inotify_event *) &inotify_buffer[inotify_buffer_done];
       Lisp_Object descriptor = INT_TO_INTEGER (ev->wd);
       Lisp_Object prevtail = find_descriptor (descriptor);
 
@@ -367,10 +394,32 @@ inotify_process_buffer (void)
           if (ev->mask & IN_IGNORED)
 	    remove_descriptor (prevtail, true);
         }
-      i += sizeof (*ev) + ev->len;
+      inotify_buffer_done += sizeof (*ev) + ev->len;
     }
 }
 
+/* Idempotently start, or stop, reading from the inotify fd, according
+ * to what is required.  */
+static void inotify_add_or_remove_reading_fd (void) {
+  if (inotifyfd >= 0) {
+    if ( kbd_on_hold_p () || inotify_buffer_done != inotify_buffer_full )
+      delete_read_fd (inotifyfd);
+    else
+      add_read_fd (inotifyfd, &inotify_callback, NULL);
+  }
+}
+
+/* Called when keyboard buffer is getting full, kbd_on_hold_p becomes true. */
+void stop_inotify_input (void) {
+  inotify_add_or_remove_reading_fd ();
+}
+
+/* Called when buffer can take more events, kbd_on_hold_p becomes false */
+void resume_inotify_input (void) {
+  inotify_process_buffer();
+  inotify_add_or_remove_reading_fd ();
+}
+
 DEFUN ("inotify-add-watch", Finotify_add_watch, Sinotify_add_watch, 3, 3, 0,
        doc: /* Add a watch for FILE-NAME to inotify.
 
@@ -447,7 +496,7 @@ renames (moved-from and moved-to).
       if (inotifyfd < 0)
 	report_file_notify_error ("File watching is not available", Qnil);
       watch_list = Qnil;
-      add_read_fd (inotifyfd, &inotify_callback, NULL);
+      inotify_add_or_remove_reading_fd ();
     }
 
   encoded_file_name = ENCODE_FILE (filename);
diff --git a/src/keyboard.c b/src/keyboard.c
index 9865bc9add..171bdcf1d5 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -3657,6 +3657,7 @@ kbd_buffer_store_buffered_event (union buffered_input_event *event,
           hold_keyboard_input ();
           unrequest_sigio ();
           stop_polling ();
+	  stop_inotify_input ();
         }
 #endif	/* subprocesses */
     }
@@ -3849,6 +3850,7 @@ kbd_buffer_get_event (KBOARD **kbp,
       unhold_keyboard_input ();
       request_sigio ();
       start_polling ();
+      resume_inotify_input ();
     }
 #endif	/* subprocesses */
 
diff --git a/src/keyboard.h b/src/keyboard.h
index 03aa96ad4b..a832d08c2b 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -499,4 +499,13 @@ kbd_buffer_store_event_hold (struct input_event *event,
 
 INLINE_HEADER_END
 
+/* Defined in inotify.c */
+#ifdef HAVE_INOTIFY
+extern void stop_inotify_input (void);
+extern void resume_inotify_input (void);
+#else
+INLINE void stop_inotify_input (void) { }
+INLINE void resume_inotify_input (void) { }
+#endif
+
 #endif /* EMACS_KEYBOARD_H */
-- 
2.20.1


[-- Attachment #6: .signature --]
[-- Type: text/plain, Size: 218 bytes --]



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

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> I experienced a bug where my Emacs (Debian's emacs-lucid) would lose
> keystrokes under pathological conditions.  I tracked this down to the
> keyboard input buffer filling up with inotify events.
>
> The attached patches have been rebased onto emacs-28 (fbc9b121e062)
> and I have tested before-and-after versions in parallel to verify that
> the bug appears in the upstream branch and has been fixed with my
> patches.
>
> The bug was really quite severe in my (rather abusive) situation, with
> Debian's emacs-lucid 26.1.  With the upstream emacs-28 branch it
> requires more effort, and loses fewer keys.  I suspect that some more
> sophisticated approach to inotify requests means it's harder to get
> the buffer to fill up with the newer version.  Unfortunately I am not
> able to share my whole repro, but a key part is to visit the output
> file from this
>   while yes | dd of=t bs=1 count=1000000; do : ;
> with global-auto-revert-mode enabled.
>
> With my patches it is still possible to get emacs to perform very
> poorly by abusing it this way, but at least all the keys arrive
> eventually.  I added some FIXMEs for further work that would be good,
> but is not critical to fixing the bug.

I would rather not allocate anything on the heap to hold inotify events.

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.

Thanks.

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.

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

[Eli Zaretskii]

> From: Ian Jackson <ijackson@chiark.greenend.org.uk>
> Date: Sat, 22 Jan 2022 19:34:33 +0000
> Cc: 53432@debbugs.gnu.org
> 
> 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.

Which is why we recommend turning off file-notifications when you turn
on global-auto-revert-mode.

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

What race is that?  This processing is going on in a single thread, so
how can there be any races?

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

Yes, but inotify (and other similar features on other OSes) doesn't
guarantee that all the events be delivered when there are many of
them.  Which is one more reason not to use file notifications to watch
too many files/directories.  This stuff isn't scalable enough.

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

It also says, right after that

						      However, when the
  release branch is for Emacs version NN.2 and later, or when it is for
  Emacs version NN.1 that is in the very last stages of its pretest,
  that branch is considered to be in a feature freeze: only bug fixes
  that are "safe" or are fixing major problems should go to the release
  branch, the rest should be committed to the master branch.  This is so
  to avoid destabilizing the next Emacs release.  If you are unsure
  whether your bug fix is "safe" enough for the release branch, ask on
  the emacs-devel mailing list.

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

Sorry, I don't see your name there.  I just rechecked.

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

[Dmitry Gutov]

On 22.01.2022 21:48, Eli Zaretskii wrote:
> Which is why we recommend turning off file-notifications when you turn
> on global-auto-revert-mode.

If it was really our recommendation, I think we'd set 
auto-revert-use-notify to nil by default.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Dmitry Gutov <dgutov@yandex.ru> writes:

> If it was really our recommendation, I think we'd set
> auto-revert-use-notify to nil by default.

That's for auto-revert-mode, which is enabled more often than its global
variant.

Thanks.

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

[Dmitry Gutov]

On 23.01.2022 04:47, Po Lu wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> If it was really our recommendation, I think we'd set
>> auto-revert-use-notify to nil by default.
> That's for auto-revert-mode, which is enabled more often than its global
> variant.

Why would it be enabled more often?

'global-auto-revert-mode' is so easy to toggle (unlike setting up your 
own auto-revert-mode setup) and it corresponds to the feature of many 
contemporary code editors which is usually on by default.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Why would it be enabled more often?

I don't know, but that (and scalability issues) is why
`auto-revert-use-notify' is set to nil when the global mode is enabled.
See bug#22814.

Thanks.

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

[Dmitry Gutov]

On 23.01.2022 04:53, Po Lu wrote:
> I don't know, but that (and scalability issues) is why
> `auto-revert-use-notify' is set to nil when the global mode is enabled.
> See bug#22814.

Is it? There are only two references to auto-revert-use-notify in 
autorevert.el, and neither changes that var.

Also see:

etc/NEWS.25
1108:*** 'auto-revert-use-notify' is set to nil in 
'global-auto-revert-mode'.

etc/NEWS.26
1070:** 'auto-revert-use-notify' is set back to t in 
'global-auto-revert-mode'.

The latter coming with commit 484967796755051c4045cdcc26b0d3d129cc72ad.

(Which was non-trivial to figure out because vc-annotate doesn't help 
with NEWS files because of rotation.)

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Is it? There are only two references to auto-revert-use-notify in
> autorevert.el, and neither changes that var.
>
> Also see:
>
> etc/NEWS.25
> 1108:*** 'auto-revert-use-notify' is set to nil in
> 'global-auto-revert-mode'.
>
> etc/NEWS.26
> 1070:** 'auto-revert-use-notify' is set back to t in
> 'global-auto-revert-mode'.
>
> The latter coming with commit 484967796755051c4045cdcc26b0d3d129cc72ad.
>
> (Which was non-trivial to figure out because vc-annotate doesn't help
> with NEWS files because of rotation.)

Hmm, maybe we should revert that commit, since file notifications are
definitely not scalable enough to support such functionality.

Eli?

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

[Dmitry Gutov]

On 23.01.2022 05:26, Po Lu via Bug reports for GNU Emacs, the Swiss army 
knife of text editors wrote:
> Hmm, maybe we should revert that commit, since file notifications are
> definitely not scalable enough to support such functionality.

VS Code et al. seem to disagree, though (*).

But it does sound like you want to discuss this issue with Michael, at 
least.

(*) These pages might be of interest:

https://stackoverflow.com/questions/50901127/vsc-unable-to-watch-for-file-changes-in-this-large-workspace-weird
https://github.com/microsoft/vscode-docs/blob/vnext/release-notes/v1_62.md#file-watching-changes

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Dmitry Gutov <dgutov@yandex.ru> writes:

> VS Code et al. seem to disagree, though (*).

Don't they also have problems with abuse of file notifications, such as
when a file is modified repeatedly in rapid succession?

Thanks.

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

[Dmitry Gutov]

On 23.01.2022 05:48, Po Lu wrote:
> Don't they also have problems with abuse of file notifications, such as
> when a file is modified repeatedly in rapid succession?

Apparently not insurmountable ones.

Both https://github.com/parcel-bundler/watcher (which VS Code uses 
according to that change log entry) and 
https://facebook.github.io/watchman/ claim to work reliably and handle 
such issues.

I'm not really a VS Code user to speak from personal experience, but 
given the size of their user base they should have kinks like that 
worked out already.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Ian Jackson
>  <ijackson@chiark.greenend.org.uk>,  53432@debbugs.gnu.org
> Date: Sun, 23 Jan 2022 11:26:22 +0800
> 
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
> > Is it? There are only two references to auto-revert-use-notify in
> > autorevert.el, and neither changes that var.
> >
> > Also see:
> >
> > etc/NEWS.25
> > 1108:*** 'auto-revert-use-notify' is set to nil in
> > 'global-auto-revert-mode'.
> >
> > etc/NEWS.26
> > 1070:** 'auto-revert-use-notify' is set back to t in
> > 'global-auto-revert-mode'.
> >
> > The latter coming with commit 484967796755051c4045cdcc26b0d3d129cc72ad.
> >
> > (Which was non-trivial to figure out because vc-annotate doesn't help
> > with NEWS files because of rotation.)
> 
> Hmm, maybe we should revert that commit, since file notifications are
> definitely not scalable enough to support such functionality.
> 
> Eli?

The nil setting was due to kqueue specific problem.  Michael fixed
that particular problem and therefore reverted the nil setting.

However, my opinion is still the same: file-notifications are not
scalable, so people who use and abuse them globally do that to their
own cost.  Not sure if it's worth forcing the notifications off,
though.  Perhaps we should have a PROBLEMS entry about that.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi,

> The nil setting was due to kqueue specific problem.  Michael fixed
> that particular problem and therefore reverted the nil setting.

Indeed.

> However, my opinion is still the same: file-notifications are not
> scalable, so people who use and abuse them globally do that to their
> own cost.  Not sure if it's worth forcing the notifications off,
> though.  Perhaps we should have a PROBLEMS entry about that.

I believe we should improve event handling. When it is not possible to
handle D-Bus events properly, and the native monitor detects this
situation, it shall signal an error or call a handler. Packages
monitoring files could react in case of.

Best regards, Michael.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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

The underlying objection is that it's IMHO wrong to allocate anything on
the heap to cater to "abuse" of a feature, in this case inotify.

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

It can't end up with the user unknowingly editing the wrong version of a
file, because Emacs will ask him:

  foo.h changed on disk; really edit the buffer? (y, n, r or C-h)

When he first tries to edit the file again (the way this works is not
file notifications, but instead comparing stat data.)

There are also builds without file notifications, which are used on many
systems.

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

Which code?  The keyboard buffer is the "buffer of fixed size" in the
inotify code.

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

All file notification systems are "best effort".  At the very least,
dropping events when the keyboard buffer is full would make the behavior
consistent with GLib file notifications, which drops notifications
whenever its own internal buffer fills up.

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

It's acceptable for there to be some lossage when confronted by (in your
own words) abuse of a feature.  That is taken for granted, whether or
not it is documented.

Thanks.

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

[Ian Jackson]

Po Lu writes ("Re: bug#53432: [PATCH] Avoid losing keyboard input when inotify is too busy [and 1 more messages]"):
> It can't end up with the user unknowingly editing the wrong version of a
> file, because Emacs will ask him:
> 
>   foo.h changed on disk; really edit the buffer? (y, n, r or C-h)

Good point.  That means that losing the file notifications isn't
terrible.  It's still not great, though.  In fact now that you point
this out, I think this (lost file notifications resulting in this
prompt) has happened to me on more than one occasion.  It made me
uneasy; I thought "why does it sometimes do this, is there something I
am doing wrong, or did I miskey?"


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.

 2. At least double the size of the keyboard buffer from 4k to 8k.
    I think this is necessary because in a stable branch we don't want
    to make anyone's experience worse.  Doing (1) will cause emacs to
    start to lose file notifications when the buffer is half full,
    rather than full.  Doubling the buffer (at trivial memory cost) is
    an easy way to fix this.

 3. Consider a significantly larger bump to the keyboard buffer to
    further reduce the incidence of this unreliability.  It's a single
    static buffer; we could easily change it from 4k to 40k (say) with
    negligible cost.

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.


For master, I still think we need to make global-auto-revert-mode
reliable.  We don't want to stop using file notifications for it
(where they are available), because we don't want emacs to be polling
- that is poor for battery life on laptops.

It is true that the kernel's inotify system may sometimes drop events
because the buffer in the kernel filled up - but when that happens, it
sends a IN_Q_OVERFLOW event, with -1 for the associated fd.
I.e. rather than completely losing events, it collapses them into a
single "you missed some stuff" event.

If we handled that notification, we could continue to use inotify and
fall back to a single scan through all the files, when we're told that
inotify was overloaded.  I haven't looked at the auto-revert code,
but, is there a way we could invoke a "one-off" poll ?

IF we did this then the inotify code could use the same mechanism to
handle the case where it dropped events due to the kbd_on_hold_p, and
everything would work correctly.  Complicated buffer management in the
inotify code wouldn't be needed.

We would still probably want to have a significantly larger keyboard
buffer, at leat when inotify is enabled.  This is because we can
reasonably expect a much higher rate of file notifications than
keyboard events.  Even if we have made emacs reliable when the buffer
fills up, we still don't *want* the buffer to fill up because the
non-file-notification based auto revert is a lot less efficient,
especially in a large emacs visiting many files: it will often be the
case that only a handful of files have changed, perhaps very many
times.

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.


> > 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).
> 
> Which code?  The keyboard buffer is the "buffer of fixed size" in the
> inotify code.

The inotify code has its own buffer in inotify_callback.  On each
pass, it calls FIONREAD and passes the return value to SAFE_ALLOCA.
I think this is quite pathological.

If we can have some proper way of handling lost inotify events
(including IN_Q_OVERFLOW) then the inotify code could have a fixed
size buffer or a fixed limit on the amount it owuld read.  For
performance reasons (see above) this ought to be reasonably large
(tens of kbytes I guess).

> All file notification systems are "best effort".  At the very least,
> dropping events when the keyboard buffer is full would make the behavior
> consistent with GLib file notifications, which drops notifications
> whenever its own internal buffer fills up.

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.


Po Lu writes ("Re: bug#53432: [PATCH] Avoid losing keyboard input when inotify is too busy [and 1 more messages]"):
> The underlying objection is that it's IMHO wrong to allocate anything on
> the heap to cater to "abuse" of a feature, in this case inotify.

I really regret (ab)using the word "abuse" in my original message.

I experienced actual lost keystrokes with emacs 26 in in my usual
configuration.  I don't think I was doing anything unreasonable.  The
build system of the project I was working on does do quite a lot of
file writing in a single directory but that is hardly so unusual.

In emacs 27 and 28 it was necessary to be more aggressive to create a
situation where the bug would reproduce.  This is in the nature of
this kind of bug.


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

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

[Michael Albinus]

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

Hi,

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

>  2. At least double the size of the keyboard buffer from 4k to 8k.
>     I think this is necessary because in a stable branch we don't want
>     to make anyone's experience worse.  Doing (1) will cause emacs to
>     start to lose file notifications when the buffer is half full,
>     rather than full.  Doubling the buffer (at trivial memory cost) is
>     an easy way to fix this.
>
>  3. Consider a significantly larger bump to the keyboard buffer to
>     further reduce the incidence of this unreliability.  It's a single
>     static buffer; we could easily change it from 4k to 40k (say) with
>     negligible cost.

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.

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

> For master, I still think we need to make global-auto-revert-mode
> reliable.  We don't want to stop using file notifications for it
> (where they are available), because we don't want emacs to be polling
> - that is poor for battery life on laptops.

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.

> It is true that the kernel's inotify system may sometimes drop events
> because the buffer in the kernel filled up - but when that happens, it
> sends a IN_Q_OVERFLOW event, with -1 for the associated fd.
> I.e. rather than completely losing events, it collapses them into a
> single "you missed some stuff" event.

Yep, this another case (beside of keyboard buffer full) where a "file
notification broken" signal is useful.

> If we handled that notification, we could continue to use inotify and
> fall back to a single scan through all the files, when we're told that
> inotify was overloaded.  I haven't looked at the auto-revert code,
> but, is there a way we could invoke a "one-off" poll ?

Don't know off-hand, but it shall be possible. However, if autorevert
falls back to polling anyway, we get this for free.

> IF we did this then the inotify code could use the same mechanism to
> handle the case where it dropped events due to the kbd_on_hold_p, and
> everything would work correctly.  Complicated buffer management in the
> inotify code wouldn't be needed.

Agreed.

> We would still probably want to have a significantly larger keyboard
> buffer, at leat when inotify is enabled.  This is because we can
> reasonably expect a much higher rate of file notifications than
> keyboard events.  Even if we have made emacs reliable when the buffer
> fills up, we still don't *want* the buffer to fill up because the
> non-file-notification based auto revert is a lot less efficient,
> especially in a large emacs visiting many files: it will often be the
> case that only a handful of files have changed, perhaps very many
> times.
>
> 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.

>> All file notification systems are "best effort".  At the very least,
>> dropping events when the keyboard buffer is full would make the behavior
>> consistent with GLib file notifications, which drops notifications
>> whenever its own internal buffer fills up.
>
> 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.

> Thanks,
> Ian.

Best regards, Michael.

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

[Ian Jackson]

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.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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

Your Emacs is probably built with D-Bus and GLib support.  Choice of
toolkit does not affect that.

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

[Michael Albinus]

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.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Mon, 24 Jan 2022 15:42:33 +0100
> Cc: Po Lu <luangruo@yahoo.com>, 53432@debbugs.gnu.org
> 
> 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.

But the disadvantage is that we will immediately be facing a problem
of priority in handling input from more than one source.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> 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.
>
> But the disadvantage is that we will immediately be facing a problem
> of priority in handling input from more than one source.

"Key strokes first!" :-)

An alternative approach could be to restrict how many burst events are
put into the beyboard buffer. Let's say that D-Bus and file notification
events are allowed to fill that buffer until (KBD_BUFFER_SIZE - 512)
events (arbitrary number). This would let place for key strokes, mouse
events and alike.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: ijackson@chiark.greenend.org.uk,  luangruo@yahoo.com,
>   53432@debbugs.gnu.org
> Date: Mon, 24 Jan 2022 16:35:17 +0100
> 
> >> 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.
> >
> > But the disadvantage is that we will immediately be facing a problem
> > of priority in handling input from more than one source.
> 
> "Key strokes first!" :-)

But it isn't only the key strokes, it's also all the events sent to us
by the window-system.  Now tell me why, say, an expose event should be
more important than a file-notification event, and not the other way
around?

> An alternative approach could be to restrict how many burst events are
> put into the beyboard buffer. Let's say that D-Bus and file notification
> events are allowed to fill that buffer until (KBD_BUFFER_SIZE - 512)
> events (arbitrary number). This would let place for key strokes, mouse
> events and alike.

That's what Po Lu was suggesting, AFAIU: limit the number of queued
file-notification events to not more than some threshold.

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

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> > But the disadvantage is that we will immediately be facing a problem
>> > of priority in handling input from more than one source.
>>
>> "Key strokes first!" :-)
>
> But it isn't only the key strokes, it's also all the events sent to us
> by the window-system.  Now tell me why, say, an expose event should be
> more important than a file-notification event, and not the other way
> around?

After all, Emacs is still a text editor, isn't it? Key strokes and mouse
events are the most important events, I believe.

As rule of thumb, I would discriminate all events, which can aarive as
burst, and which are already known to be lost sometimes. D-Bus and file
notification events. If we classify other events into this category - no
problem.

>> An alternative approach could be to restrict how many burst events are
>> put into the beyboard buffer. Let's say that D-Bus and file notification
>> events are allowed to fill that buffer until (KBD_BUFFER_SIZE - 512)
>> events (arbitrary number). This would let place for key strokes, mouse
>> events and alike.
>
> That's what Po Lu was suggesting, AFAIU: limit the number of queued
> file-notification events to not more than some threshold.

Yep. And Ian is also on the same way, AFAIU.

Best regards, Michael.

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

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: ijackson@chiark.greenend.org.uk,  luangruo@yahoo.com,
>   53432@debbugs.gnu.org
> Date: Mon, 24 Jan 2022 18:12:12 +0100
> 
> >> "Key strokes first!" :-)
> >
> > But it isn't only the key strokes, it's also all the events sent to us
> > by the window-system.  Now tell me why, say, an expose event should be
> > more important than a file-notification event, and not the other way
> > around?
> 
> After all, Emacs is still a text editor, isn't it? Key strokes and mouse
> events are the most important events, I believe.

I agree, but since window-system events come from the same source as
keys and mouse, we don't have a good way of processing "keys and mouse
first", do we?  We can only process everything that comes from the
read_socket_hook before file notification (which would mean "keys,
mouse, and window-system events first"), or the other way around.

So the question whether a window-system event is more or less
important than a file-notification event still stands, and has no easy
solution if we start reading events from two queues instead of just
one.

> As rule of thumb, I would discriminate all events, which can aarive as
> burst, and which are already known to be lost sometimes. D-Bus and file
> notification events. If we classify other events into this category - no
> problem.

That's okay (and note that w32notify already does drop excess events
on the floor).

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

[Ian Jackson]

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

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

I don't follow.

Any buffer that we have can fill up.  So each thing that generates
events needs to be able to handle the buffer (that it is writing to)
becomeing full; and to be able to handle the buffer becoming
no-longer-full, so that it can restart.

So each event-generator has a "stop" hook and a "start" hook, each of
which has one call site, in the code that handles the buffer for that
event generator.  The number of these calls is the same either way.

The difference is just whether there are two buffers, which call
disjoint sets of start/stop hooks, or one buffer, which calls all the
start/stop hooks.

[reordered slightly:]
> 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.

The two sets of code would seem very similar.  It would probably be
sensible to actually use the very same code and data formats and
everything.  The advantage is precisely that we can prioritise them
differently: the keyboard buffer could keep working, even while the
other buffer is full.  In practice I don't think this is likely to be
particularly important since we don't usually expect emacs to be
overloaded in this way.  And when it is, there is an argument that
processing events in order is likely to be better in the sense that it
is less confusing to the user.


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

I think we are just having a semantic difference here.  To my mind
"handling" lost events *consists of* notifying the next layer up that
"some events may have been lost".  Eventually this informaton will get
to somewhere that can do something sensible with it.


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

If the buffer overflowed, it is quite easy to discard now-superfluous
events at the lower layer (thus coalescing events into the single
"some may have been lost" callback).  At higher levels the state
machine needed to do this becomes more complicated.

And discarding events at a lower layer helps performance because it
means they don't need to be transformed/analysed/etc.  The performance
of event processing is particularly important in the case when things
are stressed and overloaded.

But I don't think this optimisation, at this level, is inherently
*necessary* for correct operation.

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

It will help to fix lost keystrokes only if we don't plan to fix the
uncontrolled spew from the other event sources.  But it is easier to
fix the lost keystrokes:

I suggest introducing a version of kbd_buffer_store_event which is to
be called by anyone that doesn't do flow control properly.  It would
stop putting things in the buffer when the buffer is (say) 3/4 full.

The plan would be to replace all calls to this function eventually.

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

I am confused.

The file notification code must have a "check all files we are
polling" facility, for when file events are not available.  Perhaps
this is buried in a polling loop, but it could be made separately
callable.

I think that the correct response to discovering that file events have
been lost, is to do a one-off poll of every file.  Perhaps we would
want to rate limit this, but that should be done by *deferring* a
poll-all, not by *skipping* one, as skipping would introduce a race
that might lose a buffer revert.  But I don't think this is actually
necessary.  In any case no keyboard events will be lost.

An alternative would be to turn off inotify entirely and just switch
back to polling.  But we wouldn't want to do that permanently; when
things have calmed down we want emacs to back to quiescently waiting
for file events.

So there would want to be some kind of timer there, and the system
would have two modes.  It seems simpler to me to have only one mode.
Writing the code to switch modes and coordinate everything seems
error-prone to me.

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.

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

[Michael Albinus]

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

Hi Ian,

>> 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.
>
> I don't follow.
>
> Any buffer that we have can fill up.  So each thing that generates
> events needs to be able to handle the buffer (that it is writing to)
> becomeing full; and to be able to handle the buffer becoming
> no-longer-full, so that it can restart.
>
> So each event-generator has a "stop" hook and a "start" hook, each of
> which has one call site, in the code that handles the buffer for that
> event generator.  The number of these calls is the same either way.
>
> The difference is just whether there are two buffers, which call
> disjoint sets of start/stop hooks, or one buffer, which calls all the
> start/stop hooks.

My proposal was to keep the keyboard buffer as-it-is for key strokes and
alike. No need to change anything. A new buffer for D-Bus and file
notification events would need an additional handling for a burst of
events.

But as Eli has replied, this brings further problems. So I don't follow
any longer this approach.

>> 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.
>
> The two sets of code would seem very similar.  It would probably be
> sensible to actually use the very same code and data formats and
> everything.  The advantage is precisely that we can prioritise them
> differently: the keyboard buffer could keep working, even while the
> other buffer is full.  In practice I don't think this is likely to be
> particularly important since we don't usually expect emacs to be
> overloaded in this way.  And when it is, there is an argument that
> processing events in order is likely to be better in the sense that it
> is less confusing to the user.

Again, two different buffers are out of discussion ...

>> 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.
>
> I think we are just having a semantic difference here.  To my mind
> "handling" lost events *consists of* notifying the next layer up that
> "some events may have been lost".  Eventually this informaton will get
> to somewhere that can do something sensible with it.

Yes, if we are speaking about the file notification backends, that's
all. And that's what I meant (sometimes, you must forgive me if I use
unprecise English).

>> 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.
>
> If the buffer overflowed, it is quite easy to discard now-superfluous
> events at the lower layer (thus coalescing events into the single
> "some may have been lost" callback).  At higher levels the state
> machine needed to do this becomes more complicated.

Agreed.

> I suggest introducing a version of kbd_buffer_store_event which is to
> be called by anyone that doesn't do flow control properly.  It would
> stop putting things in the buffer when the buffer is (say) 3/4 full.

That's what I have answered to Eli, so we're in agreement :-)

> The file notification code must have a "check all files we are
> polling" facility, for when file events are not available.  Perhaps
> this is buried in a polling loop, but it could be made separately
> callable.

I don't believe we can implement something at filenotify.el level. It is
up to the applications to handle lost events. autorevert could fall back
to polling, and it could continue to trust file notification events when
the situation becomes normal. How it is implement shall be the
responsibility of the application.

> I think that the correct response to discovering that file events have
> been lost, is to do a one-off poll of every file.  Perhaps we would
> want to rate limit this, but that should be done by *deferring* a
> poll-all, not by *skipping* one, as skipping would introduce a race
> that might lose a buffer revert.  But I don't think this is actually
> necessary.  In any case no keyboard events will be lost.

For autorevert (as example), using polling would sync all buffers
towards the real state of the file on disk. Restarting file notification
later on wouldn't be a problem.

> An alternative would be to turn off inotify entirely and just switch
> back to polling.  But we wouldn't want to do that permanently; when
> things have calmed down we want emacs to back to quiescently waiting
> for file events.

Exactly.

> So there would want to be some kind of timer there, and the system
> would have two modes.  It seems simpler to me to have only one mode.
> Writing the code to switch modes and coordinate everything seems
> error-prone to me.

Timer doesn't sound good to me. A better approach might be, if the file
notification libraries send a "back to normal" triger, perhaps also via
a callback.

> Ian.

Best regards, Michael.

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

[Ian Jackson]

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 think we are just having a semantic difference here.  To my mind
> > "handling" lost events *consists of* notifying the next layer up that
> > "some events may have been lost".  Eventually this informaton will get
> > to somewhere that can do something sensible with it.
> 
> Yes, if we are speaking about the file notification backends, that's
> all. And that's what I meant (sometimes, you must forgive me if I use
> unprecise English).

Oh, no need to apologise!  It is very easy when discussing such
complicated things for people to misunderstand each other.  I just
meant to clarify.

> > The file notification code must have a "check all files we are
> > polling" facility, for when file events are not available.  Perhaps
> > this is buried in a polling loop, but it could be made separately
> > callable.
> 
> I don't believe we can implement something at filenotify.el level. It is
> up to the applications to handle lost events. autorevert could fall back
> to polling, and it could continue to trust file notification events when
> the situation becomes normal. How it is implement shall be the
> responsibility of the application.

Right, I think you are suggesting that the "some file event got lost"
notification should bubble up to autorevert.  Either directly, or via
filenotify, I guess - not sure which you are suggesting would be
better.

That sounds correct to me.

> > I think that the correct response to discovering that file events have
> > been lost, is to do a one-off poll of every file.  Perhaps we would
> > want to rate limit this, but that should be done by *deferring* a
> > poll-all, not by *skipping* one, as skipping would introduce a race
> > that might lose a buffer revert.  But I don't think this is actually
> > necessary.  In any case no keyboard events will be lost.
> 
> For autorevert (as example), using polling would sync all buffers
> towards the real state of the file on disk. Restarting file notification
> later on wouldn't be a problem.

Precisely.

> > So there would want to be some kind of timer there, and the system
> > would have two modes.  It seems simpler to me to have only one mode.
> > Writing the code to switch modes and coordinate everything seems
> > error-prone to me.
> 
> Timer doesn't sound good to me. A better approach might be, if the file
> notification libraries send a "back to normal" triger, perhaps also via
> a callback.

Let me consider the semantics of these notifications you are
suggesting in a bit more detail.  I hope you will forgive the
resulting screed:

As I understand your proposal, the system starts in "normal" state.
In "normal" state, the file notification system should promise that if
  1. a file notification is set up
  2. subsequently, that file is modified
then the file notification system will make one of these callbacks
  A. file notification event
  B. "file notifications not working" callback
(And, I gyuess, that this will be done "reasonably promptly" in some
sense; I'm not sure if we need to define this.)

If B happens then the system is now in "non-working" state.  In
non-working state, all bets are off.  Users like autorevert must take
alternative measures.

At some point, the file notification system will make a "file
notifications working again now" callback.  I think this has to
promise that, if:
  1a. a file notification was set up
  1b. the "notifications working" callback was made more recently
       than any "notifications not working" callback
(in either order) and then *subsequently*
  2. a relevant file is notified
then a notification will occur, as above.  Obviously it can't sensibly
make guarantees about what happened before the "working" notifcation.

How must a user of this API respond ?  Consider autorevert.  Suppose
the notifications were declared "non-working" and autorevert resorted
to polling.

When autorevert is told things are working again, it wants to go back
to not polling.  But it doesn't know whether notifications were lost
between its last poll, and the file notifications becoming normal
again.  There might be a few leftover lost events.

So autorevert must poll every file again, once.  I think it makes most
sense to do this immediately, rather than wait for the next poll
interval.  That allows autorevert to avoid a confusing special-case
transitional state where it is not polling but still has one poll to
do - and it means the user gets the buffers reverted as soon as
possible rather than after a timeout.

Now, consider the behaviour of the file notification system.

In practice, inotify.c will probably want to declare itself working as
soon as the event buffer it is writing to becomes empty.  Doing so
sooner merely imposes more load on a busy emacs; doing so later
degrades the user experience by having the emacs unnecessarily wait
before catching up with the work that needs to be done.

What this means is that the file notification system is only in the
"non-working" state when emacs is too busy handling important events
to catch its breath and handle file notifications.  Ie, only when
emacs is not idle.

In this state, we do not actually want to poll for file notifications!
It doesn't make sense to make this fire off a timer.

So autorevert does not actually need to do anything with the "not
working" notification.  It should wait for the "working again"
notification, and respond by polling all the files exactly once each.

I think a similar situation will arise for other kinds of event
generation.  So we don't actually need two separate notifications.
We only need one, which means: "err, sorry, it turns out things
weren't working, but I think they're OK now - and I'll let you know
again if not".

The result of my approach as I describe above will be an emacs which,
when overloaded, suspends file event handling in order to handle user
input, but which promptly catches up with deferred work when it can.

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

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

[Michael Albinus]

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

Hi Ian,

> Let me consider the semantics of these notifications you are
> suggesting in a bit more detail.  I hope you will forgive the
> resulting screed:
>
> As I understand your proposal, the system starts in "normal" state.
> In "normal" state, the file notification system should promise that if
>   1. a file notification is set up
>   2. subsequently, that file is modified
> then the file notification system will make one of these callbacks
>   A. file notification event
>   B. "file notifications not working" callback
> (And, I gyuess, that this will be done "reasonably promptly" in some
> sense; I'm not sure if we need to define this.)

Thinking about, we don't need another callback. We can define a special
file notification event which tells us. Something like

'(file-notify <descriptor> overflow)

where <descriptor> is the result of a respective `file-notify-add-watch'
call, and `overflow' is a new action. `file-notify-handle-event' calls
the registered application specific handler with this event; that
handler is responsible for whatever.

I see two cases where we could inject such an event: the keyboard buffer
size exceeds a given high water mark, like 80% or 90%. Or (in the
inotify case) we receive an IN_Q_OVERFLOW event. Other backends might
know further situations when such an event is needed.

> If B happens then the system is now in "non-working" state.  In
> non-working state, all bets are off.  Users like autorevert must take
> alternative measures.

Yep. Application specific handlers, like `auto-revert-notify-handler',
must react.

> At some point, the file notification system will make a "file
> notifications working again now" callback.  I think this has to
> promise that, if:
>   1a. a file notification was set up
>   1b. the "notifications working" callback was made more recently
>        than any "notifications not working" callback
> (in either order) and then *subsequently*
>   2. a relevant file is notified
> then a notification will occur, as above.  Obviously it can't sensibly
> make guarantees about what happened before the "working" notifcation.

This might be signalled by a another artifical event like

'(file-notify <descriptor> normal)

which is treated like the other event. It could happen if the keyboard
buffer size falls below a given low water mark, say 40% or 50%. Or (in
the inotify case) we receive events w/o IN_Q_OVERFLOW bit mask.

(Action names like `overflow' and `normal' are random, any better
proposal is appreciated).

> How must a user of this API respond ?  Consider autorevert.  Suppose
> the notifications were declared "non-working" and autorevert resorted
> to polling.
>
> When autorevert is told things are working again, it wants to go back
> to not polling.  But it doesn't know whether notifications were lost
> between its last poll, and the file notifications becoming normal
> again.  There might be a few leftover lost events.
>
> So autorevert must poll every file again, once.  I think it makes most
> sense to do this immediately, rather than wait for the next poll
> interval.  That allows autorevert to avoid a confusing special-case
> transitional state where it is not polling but still has one poll to
> do - and it means the user gets the buffers reverted as soon as
> possible rather than after a timeout.

I believe it would be sufficient to signal "after next poll we're going
back to file notifications". This is transitional, but not very hard to
implement. And we will wait 5 seconds max, which doesn't sound too terrible.

> Now, consider the behaviour of the file notification system.
>
> In practice, inotify.c will probably want to declare itself working as
> soon as the event buffer it is writing to becomes empty.  Doing so
> sooner merely imposes more load on a busy emacs; doing so later
> degrades the user experience by having the emacs unnecessarily wait
> before catching up with the work that needs to be done.
>
> What this means is that the file notification system is only in the
> "non-working" state when emacs is too busy handling important events
> to catch its breath and handle file notifications.  Ie, only when
> emacs is not idle.
>
> In this state, we do not actually want to poll for file notifications!
> It doesn't make sense to make this fire off a timer.

Well, we need to read the incoming events, but we won't push them to the
keyboard buffer. Until the low watermark has been reached, which means
Emacs isn't too busy anymore.

> So autorevert does not actually need to do anything with the "not
> working" notification.  It should wait for the "working again"
> notification, and respond by polling all the files exactly once each.

Why not do default polling, every 5 seconds? As said, the "not working"
state doesn't mean always that Emacs is too busy. It could also be the
case, that too many events are fired by the kernel, events are lost,
and we are informed about by IN_Q_OVERFLOW.

> I think a similar situation will arise for other kinds of event
> generation.  So we don't actually need two separate notifications.
> We only need one, which means: "err, sorry, it turns out things
> weren't working, but I think they're OK now - and I'll let you know
> again if not".
>
> The result of my approach as I describe above will be an emacs which,
> when overloaded, suspends file event handling in order to handle user
> input, but which promptly catches up with deferred work when it can.

Well, injecting the two new events gives the decision what to do to the
application handlers. They can just sit and wait, or they can try to
receive needed information otherwise, like autorevert does with polling.
And such a strategy could be customized.

Best regards, Michael.

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

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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.

It's too late to do anything of this sort on the release branch.

>  2. At least double the size of the keyboard buffer from 4k to 8k.
>     I think this is necessary because in a stable branch we don't want
>     to make anyone's experience worse.  Doing (1) will cause emacs to
>     start to lose file notifications when the buffer is half full,
>     rather than full.  Doubling the buffer (at trivial memory cost) is
>     an easy way to fix this.

Why is that?  I'm not convinced that's necessary, since you're the only
person to complain so far, and you needed to abuse the feature in order
to get keys to be dropped.

>  3. Consider a significantly larger bump to the keyboard buffer to
>     further reduce the incidence of this unreliability.  It's a single
>     static buffer; we could easily change it from 4k to 40k (say) with
>     negligible cost.

40k is unacceptable.

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

We have many other file notification systems, essentially one for each
platform we support.  The one that people actually build Emacs with is
in gfilenotify.c, and you can't get it to experience such problems
since it will silently drop file notifications once its own internal
buffer fills up.  GLib does not tell you when that happens, because it
happens rarely enough to be worth taking into account.

> For master, I still think we need to make global-auto-revert-mode
> reliable.  We don't want to stop using file notifications for it
> (where they are available), because we don't want emacs to be polling
> - that is poor for battery life on laptops.

Someone has to demonstrate that running `stat' every five seconds
actually results in the visible decrease of battery life before we take
that into account.

> IF we did this then the inotify code could use the same mechanism to
> handle the case where it dropped events due to the kbd_on_hold_p, and
> everything would work correctly.  Complicated buffer management in the
> inotify code wouldn't be needed.

We don't want to drop inotify events when keyboard input is paused, we
only want to drop them when `kbd_buffer_nr_stored' is equal to
KBD_BUFFER_SIZE.

> We would still probably want to have a significantly larger keyboard
> buffer, at leat when inotify is enabled.

The keyboard buffer is read very rapidly in most cases, so that
shouldn't be necessary at all.

> This is because we can reasonably expect a much higher rate of file
> notifications than keyboard events.  Even if we have made emacs
> reliable when the buffer fills up, we still don't *want* the buffer to
> fill up because the non-file-notification based auto revert is a lot
> less efficient, especially in a large emacs visiting many files: it
> will often be the case that only a handful of files have changed,
> perhaps very many times.

That sounds like premature optimization to me.  Between Emacs 25 and 26,
file notifications were disabled for global-auto-revert-mode, and I
don't think anyone complained very loudly.

> The inotify code has its own buffer in inotify_callback.  On each
> pass, it calls FIONREAD and passes the return value to SAFE_ALLOCA.
> I think this is quite pathological.

SAFE_ALLOCA does not usually allocate anything on the heap, so its
impact is negligible.

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

It does not.  It happens rarely enough that the GLib target audience
doesn't care about when file notifications are actually dropped.

> In emacs 27 and 28 it was necessary to be more aggressive to create a
> situation where the bug would reproduce.  This is in the nature of
> this kind of bug.

Why is that so?

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

[Eli Zaretskii]

> From: Ian Jackson <ijackson@chiark.greenend.org.uk>
> Date: Fri, 21 Jan 2022 23:36:01 +0000
> 
> I experienced a bug where my Emacs (Debian's emacs-lucid) would lose
> keystrokes under pathological conditions.  I tracked this down to the
> keyboard input buffer filling up with inotify events.
> 
> The attached patches have been rebased onto emacs-28 (fbc9b121e062)
> and I have tested before-and-after versions in parallel to verify that
> the bug appears in the upstream branch and has been fixed with my
> patches.
> 
> The bug was really quite severe in my (rather abusive) situation, with
> Debian's emacs-lucid 26.1.  With the upstream emacs-28 branch it
> requires more effort, and loses fewer keys.  I suspect that some more
> sophisticated approach to inotify requests means it's harder to get
> the buffer to fill up with the newer version.  Unfortunately I am not
> able to share my whole repro, but a key part is to visit the output
> file from this
>   while yes | dd of=t bs=1 count=1000000; do : ;
> with global-auto-revert-mode enabled.
> 
> With my patches it is still possible to get emacs to perform very
> poorly by abusing it this way, but at least all the keys arrive
> eventually.  I added some FIXMEs for further work that would be good,
> but is not critical to fixing the bug.

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.

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