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

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

[Michael Albinus]

Unknown <unknown@unknown.invalid> writes:

Hi Mattias,

>     Don't remove notify descriptor that is already gone
>
>     * lisp/autorevert.el (auto-revert-use-notify, auto-revert-mode,
>     global-auto-revert-mode, auto-revert-notify-rm-watch,
>     auto-revert-notify-add-watch, auto-revert-notify-handler,
>     auto-revert-notify-rm-watch-callback):
>     Don't remove a notify descriptor after receiving a `stopped' notification
>     event, because the descriptor is then already gone and any attempt to
>     remove it causes a recursive call to `auto-revert-notify-handler'.

Thanks for this!

I haven't tested thoroughly yet, but wouldn't it suffice if in
auto-revert-notify-rm-watch there is just the test

(when (file-notify-valid-p auto-revert-notify-watch-descriptor)

instead of

(when auto-revert-notify-watch-descriptor

Best regards, Michael.

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

[Mattias Engdegård]

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

16 apr. 2019 kl. 09.04 skrev Michael Albinus <michael.albinus@gmx.de>:

> I haven't tested thoroughly yet, but wouldn't it suffice if in
> auto-revert-notify-rm-watch there is just the test
> 
> (when (file-notify-valid-p auto-revert-notify-watch-descriptor)
> 
> instead of
> 
> (when auto-revert-notify-watch-descriptor

Thanks for reading my change. It is a fair question!

First of all, the descriptor wouldn't then be removed from `auto-revert-notify-watch-descriptor-hash-list' since that part is also guarded by the condition, but that's just a matter of rearranging code.

(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'?)

I'm not necessarily enamoured with `file-notify-valid-p'. It just tells whether there is, right now, a descriptor that looks like the argument, in use by someone, somewhere. These descriptors are reused, making the predicate dangerous to rely on, or requires brittle code that just knows that no reuse has occurred.

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.

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.

(For that matter, the documentation doesn't say what mutation is permitted inside `maphash'. I can guess from the source.)

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.

[-- Attachment #2: 0001-autorevert.el-auto-revert-notify-rm-watch-Simplify.patch --]
[-- Type: application/octet-stream, Size: 2009 bytes --]

From 00733fac960526e25e5f7e5d68be48d042d28315 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 16 Apr 2019 15:13:57 +0200
Subject: [PATCH] * autorevert.el (auto-revert-notify-rm-watch): Simplify.

---
 lisp/autorevert.el | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/lisp/autorevert.el b/lisp/autorevert.el
index 1dc2f0eafd..cf9dc53f85 100644
--- a/lisp/autorevert.el
+++ b/lisp/autorevert.el
@@ -501,19 +501,17 @@ will use an up-to-date value of `auto-revert-interval'"
   "Disable file notification for current buffer's associated file.
 If REMOVE-DESCRIPTOR is non-nil, remove the corresponding notification
 descriptor; otherwise assume that it has already been removed."
-  (when auto-revert-notify-watch-descriptor
-    (maphash
-     (lambda (key value)
-       (when (equal key auto-revert-notify-watch-descriptor)
-	 (setq value (delete (current-buffer) value))
-	 (if value
-	     (puthash key value auto-revert-notify-watch-descriptor-hash-list)
-	   (remhash key auto-revert-notify-watch-descriptor-hash-list)
-           (when remove-descriptor
-	     (ignore-errors
-	       (file-notify-rm-watch auto-revert-notify-watch-descriptor))))))
-     auto-revert-notify-watch-descriptor-hash-list)
-    (remove-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch-callback t))
+  (let ((desc auto-revert-notify-watch-descriptor)
+        (table auto-revert-notify-watch-descriptor-hash-list))
+    (when desc
+      (let ((buffers (delq (current-buffer) (gethash desc table))))
+        (if buffers
+            (puthash desc buffers table)
+          (remhash desc table)))
+      (when remove-descriptor
+	(ignore-errors
+	  (file-notify-rm-watch desc)))
+      (remove-hook 'kill-buffer-hook #'auto-revert-notify-rm-watch-callback t)))
   (setq auto-revert-notify-watch-descriptor nil
 	auto-revert-notify-modified-p nil))
 
-- 
2.20.1 (Apple Git-117)

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

[Michael Albinus]

Mattias Engdegård <mattiase@acm.org> writes:

Hi Mattias,

>> I haven't tested thoroughly yet, but wouldn't it suffice if in
>> auto-revert-notify-rm-watch there is just the test
>> 
>> (when (file-notify-valid-p auto-revert-notify-watch-descriptor)
>> 
>> instead of
>> 
>> (when auto-revert-notify-watch-descriptor
>
> Thanks for reading my change. It is a fair question!
>
> First of all, the descriptor wouldn't then be removed from
> `auto-revert-notify-watch-descriptor-hash-list' since that part is
> also guarded by the condition, but that's just a matter of rearranging
> code.

Yes.

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

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

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

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

Best regards, Michael.

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

[Mattias Engdegård]

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

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

[Michael Albinus]

Mattias Engdegård <mattiase@acm.org> writes:

Hi Mattias,

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

LGTM. Please push.

Best regards, Michael.