[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#35418: [PATCH] Don't poll auto-revert files that use notification
From: |
Mattias Engdegård |
Subject: |
bug#35418: [PATCH] Don't poll auto-revert files that use notification |
Date: |
Sat, 27 Apr 2019 18:19:36 +0200 |
27 apr. 2019 kl. 11.27 skrev Michael Albinus <michael.albinus@gmx.de>:
>
> Well, in inotify you still get undesired notifications. Like this:
>
> --8<---------------cut here---------------start------------->8---
> (write-region "foo" nil "/tmp/foo")
> (add-name-to-file "/tmp/foo" "/tmp/bar" 'ok)
>
> (inotify-add-watch "/tmp/foo" t (lambda (event) (message "inotify %S" event)))
> => (1 . 0)
> (inotify-add-watch "/tmp/bar" t (lambda (event) (message "inotify %S" event)))
> => (1 . 1)
> (write-region "foo" nil "/tmp/foo")
> => inotify ((1 . 0) (modify) "/tmp/foo" 0)
> inotify ((1 . 1) (modify) "/tmp/bar" 0)
Thanks for the example!
I wouldn't call this undesired. Create a hard link to a file, ask for
notification on both links, and then modify the file. Then both notifiers
trigger, because someone has modified the file they were watching. The kqueue
back-end behaves similarly.
> However, in filenotify this is fixed:
>
> --8<---------------cut here---------------start------------->8---
> (file-notify-add-watch "/tmp/foo" '(change attribute-change)
> (lambda (event) (message "file-notify %S" event)))
> => (2 . 0)
> (file-notify-add-watch "/tmp/bar" '(change attribute-change)
> (lambda (event) (message "file-notify %S" event)))
> => (2 . 1)
> (write-region "foo" nil "/tmp/foo")
> => file-notify ((2 . 0) changed "/tmp/foo")
> inotify ((1 . 0) (modify) "/tmp/foo" 0)
> inotify ((1 . 1) (modify) "/tmp/bar" 0)
Actually, it is (arguably) a bug. With two buffers referring to distinct hard
links for the same file, surely we want a change in that file to trigger
notification for both! (It's quite an exotic case, not the least because Emacs
normally recognises hard links as if they were the same file name.)
However, with the kqueue back-end, file-notify watches do trigger for both, as
expected.
The reason is that file-notify does not call inotify-add-watch on individual
files, as in your example above, but on their containing directory ("/tmp" in
your example). When monitoring a directory with two hard links to the same
file, and the file is changed, inotify (sensibly) only reports a change to one
of the links (the one employed for the change). Thus, the logic is in the Linux
kernel, not in filenotify.
For kqueue it is different: here, changes to files are not reported when a
watch is monitoring their directory, so filenotify.el sets kqueue watches on
each file instead. The same could be done with inotify (and w32notify, if I
read the code right), but watching directories has certain advantages.
> Unrelated events for "/tmp/bar" are filtered out in
> `file-notify-callback'. So yes, the inotify problems seem to be fixed.
Are you really sure that the inotify problems were really about changes to
files with multiple hard links? It sounds very unlikely, and as showed above,
the behaviour differs between back-ends.
If I were to guess, the problem was rather that the inotify back-end used to
return the kernel-provided descriptor number, which is the same for the same
directory: when /dir/a and /dir/b (distinct files, not hard links) both were
watched, inotify would monitor /dir twice and give the same descriptor for
both, with the ensuing chaos. This was subsequently fixed by making inotify
return unique descriptors.
> We might extend this variable. *If* this regexp matches a file name, we
> know that we need polling. But it is clear, that we cannot catch all
> cases by just parsing file names.
>
> (Btw, we should use the value of `mounted-file-systems', introduced in
> Emacs 26.1, when initializing `auto-revert-notify-exclude-dir-regexp'.)
That variable contains "^//[^/]+/" on Windows, so we need to make up our minds
about it.
> One alternative approach could be to analyze the file system device
> number, as returned by `file-attributes'. By this, we could detect
> mounted file systems.
Sort of; the interpretation is tricky, and as Eli commented, quite
platform-specific.
> But I don't believe that this information is always trustworty, given it
> isn't used anywhere. And at least for remote files it doesn't tell you
> anything. Furthermore, mounted file systems are not the only reason that
> file notification doesn't work, and we need to poll.
What other reasons are you thinking about?
- bug#35418: [PATCH] Don't poll auto-revert files that use notification, (continued)
bug#35418: [PATCH] Don't poll auto-revert files that use notification, Mattias Engdegård, 2019/04/25
- bug#35418: [PATCH] Don't poll auto-revert files that use notification, Eli Zaretskii, 2019/04/25
- bug#35418: [PATCH] Don't poll auto-revert files that use notification, Mattias Engdegård, 2019/04/25
- bug#35418: [PATCH] Don't poll auto-revert files that use notification, Michael Albinus, 2019/04/27
- bug#35418: [PATCH] Don't poll auto-revert files that use notification, Eli Zaretskii, 2019/04/27
- bug#35418: [PATCH] Don't poll auto-revert files that use notification, Michael Albinus, 2019/04/27
bug#35418: [PATCH] Don't poll auto-revert files that use notification,
Mattias Engdegård <=
bug#35418: [PATCH] Don't poll auto-revert files that use notification, Eli Zaretskii, 2019/04/27
bug#35418: [PATCH] Don't poll auto-revert files that use notification, Mattias Engdegård, 2019/04/28
bug#35418: [PATCH] Don't poll auto-revert files that use notification, Michael Albinus, 2019/04/29
bug#35418: [PATCH] Don't poll auto-revert files that use notification, Mattias Engdegård, 2019/04/29
bug#35418: [PATCH] Don't poll auto-revert files that use notification, Michael Albinus, 2019/04/29
bug#35418: [PATCH] Don't poll auto-revert files that use notification, Eli Zaretskii, 2019/04/29
bug#35418: [PATCH] Don't poll auto-revert files that use notification, Mattias Engdegård, 2019/04/29
bug#35418: [PATCH] Don't poll auto-revert files that use notification, Michael Albinus, 2019/04/29
bug#35418: [PATCH] Don't poll auto-revert files that use notification, Eli Zaretskii, 2019/04/29
bug#35418: [PATCH] Don't poll auto-revert files that use notification, Mattias Engdegård, 2019/04/30