Re: master e9e807e: Don't remove notify descriptor that is already gone

["Mattias Engdegård" <mattiase@acm.org>]

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

19 apr. 2019 kl. 16.56 skrev Michael Albinus <michael.albinus@gmx.de>:
> 
>> (Not only is `auto-revert-notify-watch-descriptor-hash-list' a
>> mouthful, it is a bit misleading. It maps descriptors to lists of
>> buffers. How about `auto-revert--buffers-by-watch-descriptor'?)
> 
> No objection.

Thanks, I'll do this separately.

>> Slightly more robust would be to stop reusing descriptors: either made
>> mutable, so that they can be invalidated, or made unique by using a
>> counter. However, the basic design is still somewhat dubious: it tells
>> us whether the descriptor is valid, but that just raises the question:
>> why do we even have to ask? Correct code should understand its own
>> invariants.
> 
> In theory you are right. But I fear there could be situations where such
> assumptions do ne keep. A double-check is OK.

What about using the checks in assertions to validate the assumptions, instead of making the code bug-tolerant? Assertions both inform the reader and check correctness.

After all, bug-tolerant code just leads to more bugs, to code where the just-in-case conditions cannot be distinguished from the essential ones, and where nobody understands the invariants.

>> Now that you `mentioned auto-revert-notify-rm-watch', does it strike
>> you as odd the way it does
>> 
>>    (maphash
>>     (lambda (key value)
>>       (when (equal key some-key)
>>         do-something))
>>     some-hashtable)
>> 
>> instead of using the hash table directly? Suggested patch to fix this
>> attached.
> 
> I'm still not convinced that we need REMOVE-DESCRIPTOR. We shall always
> remove the descriptor, and assure, that no superfluous events are raised.

Can I take it that you are happy with the patch attached to that message, which just does away with the maphash?

Regarding getting rid of remove-descriptor, that's fine with me -- I'm attaching a new patch which does the work in file-notify instead, which is how I interpret your wish. With that patch, e9e807e931 (addition of the remove-descriptor parameter) can be reverted.

>> By the way, why don't we give each buffer in auto-revert-mode a unique
>> descriptor, so that the table just maps descriptors to buffers,
>> instead of to lists of buffers? It would simplify the code in many
>> places, and it cannot be that common to have multiple buffers for the
>> same file that it warrants the descriptor-sharing optimisation.
> 
> Well, the descriptor is the one we get from filenotify. I don't believe
> we shall do double housekeeping. Sounds to me error-prone.

Agreed, and filenotify seems to ensure that each watch gets a distinct descriptor even for the same file. I'll prepare a patch for simplifying the table in autorevert.el, unless you can remember the reason for introducing the buffer lists in ef3544f6a6. I have read bug#13540, which is mentioned in the commit message, and am none the wiser.


[-- Attachment #2: 0001-Make-file-notify-rm-watch-robust-against-reentry.patch --]
[-- Type: application/octet-stream, Size: 4609 bytes --]

From 1105a7d356b1adfb2a39ae29f4970e997f37a2f6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sat, 20 Apr 2019 10:19:52 +0200
Subject: [PATCH] Make file-notify-rm-watch robust against reentry

Allow file-notify callbacks to call `file-notify-rm-watch', harmlessly,
after receiving a `stopped' event without triggering recursion.

* lisp/filenotify.el (file-notify--watch): Note that `callback' can be nil.
(file-notify--rm-descriptor): Set the `callback' field to nil before
sending `stopped'.
(file-notify-rm-watch): Don't do anything if the `callback' field is nil.
---
 lisp/filenotify.el | 57 +++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/lisp/filenotify.el b/lisp/filenotify.el
index 3f9bb960a9..62dd1cd911 100644
--- a/lisp/filenotify.el
+++ b/lisp/filenotify.el
@@ -49,7 +49,7 @@ could use another implementation.")
   directory
   ;; Watched relative filename, nil if watching the directory.
   filename
-  ;; Function to propagate events to.
+  ;; Function to propagate events to, or nil if watch is being removed.
   callback)
 
 (defun file-notify--watch-absolute-filename (watch)
@@ -72,12 +72,15 @@ struct.")
 DESCRIPTOR should be an object returned by `file-notify-add-watch'.
 If it is registered in `file-notify-descriptors', a stopped event is sent."
   (when-let* ((watch (gethash descriptor file-notify-descriptors)))
-    ;; Send `stopped' event.
-    (unwind-protect
-        (funcall
-         (file-notify--watch-callback watch)
-         `(,descriptor stopped ,(file-notify--watch-absolute-filename watch)))
-      (remhash descriptor file-notify-descriptors))))
+    (let ((callback (file-notify--watch-callback watch)))
+      ;; Make sure this is the last time the callback is invoked.
+      (setf (file-notify--watch-callback watch) nil)
+      ;; Send `stopped' event.
+      (unwind-protect
+          (funcall
+           callback
+           `(,descriptor stopped ,(file-notify--watch-absolute-filename watch)))
+        (remhash descriptor file-notify-descriptors)))))
 
 ;; This function is used by `inotify', `kqueue', `gfilenotify' and
 ;; `w32notify' events.
@@ -381,25 +384,27 @@ FILE is the name of the file whose event is being reported."
   "Remove an existing watch specified by its DESCRIPTOR.
 DESCRIPTOR should be an object returned by `file-notify-add-watch'."
   (when-let* ((watch (gethash descriptor file-notify-descriptors)))
-    (let ((handler (find-file-name-handler
-                    (file-notify--watch-directory watch)
-                    'file-notify-rm-watch)))
-      (condition-case nil
-          (if handler
-              ;; A file name handler could exist even if there is no
-              ;; local file notification support.
-              (funcall handler 'file-notify-rm-watch descriptor)
-
-            (funcall
-             (cond
-              ((eq file-notify--library 'inotify) 'inotify-rm-watch)
-              ((eq file-notify--library 'kqueue) 'kqueue-rm-watch)
-              ((eq file-notify--library 'gfilenotify) 'gfile-rm-watch)
-              ((eq file-notify--library 'w32notify) 'w32notify-rm-watch))
-             descriptor))
-        (file-notify-error nil)))
-    ;; Modify `file-notify-descriptors'.
-    (file-notify--rm-descriptor descriptor)))
+    ;; If we are called from a `stopped' event, do nothing.
+    (when (file-notify--watch-callback watch)
+      (let ((handler (find-file-name-handler
+                      (file-notify--watch-directory watch)
+                      'file-notify-rm-watch)))
+        (condition-case nil
+            (if handler
+                ;; A file name handler could exist even if there is no
+                ;; local file notification support.
+                (funcall handler 'file-notify-rm-watch descriptor)
+
+              (funcall
+               (cond
+                ((eq file-notify--library 'inotify) 'inotify-rm-watch)
+                ((eq file-notify--library 'kqueue) 'kqueue-rm-watch)
+                ((eq file-notify--library 'gfilenotify) 'gfile-rm-watch)
+                ((eq file-notify--library 'w32notify) 'w32notify-rm-watch))
+               descriptor))
+          (file-notify-error nil)))
+      ;; Modify `file-notify-descriptors' and send a `stopped' event.
+      (file-notify--rm-descriptor descriptor))))
 
 (defun file-notify-valid-p (descriptor)
   "Check a watch specified by its DESCRIPTOR.
-- 
2.20.1 (Apple Git-117)