all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
@ 2017-05-17 23:57 Dmitry Gutov
  2017-05-20 11:36 ` Eli Zaretskii
  2017-05-26 18:06 ` Paul Eggert
  0 siblings, 2 replies; 29+ messages in thread
From: Dmitry Gutov @ 2017-05-17 23:57 UTC (permalink / raw)
  To: 26973

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

1. Copy stp-test.el and test.sh to the same directory.
2. Evaluate the .el file.
3. M-x global-auto-revert-mode.
4. M-x start-file-process-test.
5. See the message log. "sleeping done!" comes before "process done!".

Without step 3, "process done!" comes before "sleeping done!".

So it looks like, effectively, global-auto-revert-mode stops sleep-for
from calling process sentinels, at least.

I don't remember seeing anything like this in Emacs 25.

sit-for is not affected, BTW.

In GNU Emacs 26.0.50 (build 10, x86_64-pc-linux-gnu, GTK+ Version 3.20.9)
  of 2017-05-14 built on zappa
Repository revision: e6f64df9c2b443d3385c2c25c29ccd5283d37e3f
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
System Description:	Ubuntu 16.10

[-- Attachment #2: sfp-test.el --]
[-- Type: text/x-emacs-lisp, Size: 408 bytes --]

(defun start-file-process-test ()
  (interactive)
  (let* ((buf (get-buffer-create "*test*"))
         (process (start-file-process "foo" buf
                                      (executable-find "sh")
                                      "./test.sh")))
    (set-process-sentinel
     process
     (lambda (proc status)
       (message "process done!")))
    (sleep-for 3)
    (message "sleeping done!")))

[-- Attachment #3: test.sh --]
[-- Type: application/x-shellscript, Size: 22 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-17 23:57 bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled Dmitry Gutov
@ 2017-05-20 11:36 ` Eli Zaretskii
  2017-05-20 12:45   ` Dmitry Gutov
  2017-05-26 18:06 ` Paul Eggert
  1 sibling, 1 reply; 29+ messages in thread
From: Eli Zaretskii @ 2017-05-20 11:36 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 26973

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 18 May 2017 02:57:17 +0300
> 
> 1. Copy stp-test.el and test.sh to the same directory.
> 2. Evaluate the .el file.
> 3. M-x global-auto-revert-mode.
> 4. M-x start-file-process-test.
> 5. See the message log. "sleeping done!" comes before "process done!".
> 
> Without step 3, "process done!" comes before "sleeping done!".
> 
> So it looks like, effectively, global-auto-revert-mode stops sleep-for
> from calling process sentinels, at least.
> 
> I don't remember seeing anything like this in Emacs 25.

Does anything change if you set auto-revert-use-notify to a nil value?





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-20 11:36 ` Eli Zaretskii
@ 2017-05-20 12:45   ` Dmitry Gutov
  2017-05-20 13:38     ` Eli Zaretskii
  0 siblings, 1 reply; 29+ messages in thread
From: Dmitry Gutov @ 2017-05-20 12:45 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 26973

On 20.05.2017 14:36, Eli Zaretskii wrote:

> Does anything change if you set auto-revert-use-notify to a nil value?

Yes: that seems to fix the problem.

In anticipation of the next question, my value of file-notify--library 
is `inotify'.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-20 12:45   ` Dmitry Gutov
@ 2017-05-20 13:38     ` Eli Zaretskii
  2017-05-21 22:38       ` Dmitry Gutov
  2017-05-22  7:52       ` Michael Albinus
  0 siblings, 2 replies; 29+ messages in thread
From: Eli Zaretskii @ 2017-05-20 13:38 UTC (permalink / raw)
  To: Dmitry Gutov, Michael Albinus; +Cc: 26973

> Cc: 26973@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sat, 20 May 2017 15:45:57 +0300
> 
> On 20.05.2017 14:36, Eli Zaretskii wrote:
> 
> > Does anything change if you set auto-revert-use-notify to a nil value?
> 
> Yes: that seems to fix the problem.
> 
> In anticipation of the next question, my value of file-notify--library 
> is `inotify'.

Right, that's what I thought.  I'm sure Michael (CC'ed) will look into
this.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-20 13:38     ` Eli Zaretskii
@ 2017-05-21 22:38       ` Dmitry Gutov
  2017-05-22  4:06         ` Eli Zaretskii
  2017-05-22  7:52       ` Michael Albinus
  1 sibling, 1 reply; 29+ messages in thread
From: Dmitry Gutov @ 2017-05-21 22:38 UTC (permalink / raw)
  To: Eli Zaretskii, Michael Albinus; +Cc: 26973

On 20.05.2017 16:38, Eli Zaretskii wrote:

>> In anticipation of the next question, my value of file-notify--library
>> is `inotify'.
> 
> Right, that's what I thought.  I'm sure Michael (CC'ed) will look into
> this.

Thanks, waiting for Michael.

BTW, accept-process-output also seems broken in similar scenarios.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-21 22:38       ` Dmitry Gutov
@ 2017-05-22  4:06         ` Eli Zaretskii
  2017-05-24  0:58           ` Dmitry Gutov
  0 siblings, 1 reply; 29+ messages in thread
From: Eli Zaretskii @ 2017-05-22  4:06 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 26973, michael.albinus

> Cc: 26973@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 22 May 2017 01:38:31 +0300
> 
> BTW, accept-process-output also seems broken in similar scenarios.

Of course: both accept-process-output and sleep-for call the same
low-level API.

I hope it isn't something related to threads...





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-20 13:38     ` Eli Zaretskii
  2017-05-21 22:38       ` Dmitry Gutov
@ 2017-05-22  7:52       ` Michael Albinus
  2017-05-22 18:24         ` Eli Zaretskii
  1 sibling, 1 reply; 29+ messages in thread
From: Michael Albinus @ 2017-05-22  7:52 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 26973, Dmitry Gutov

Eli Zaretskii <eliz@gnu.org> writes:

>> > Does anything change if you set auto-revert-use-notify to a nil value?
>> 
>> Yes: that seems to fix the problem.
>> 
>> In anticipation of the next question, my value of file-notify--library 
>> is `inotify'.
>
> Right, that's what I thought.  I'm sure Michael (CC'ed) will look into
> this.

Will do. But this might take time; sleep-for and accept-process-output
are terra incognita for me until now.

Best regards, Michael.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-22  7:52       ` Michael Albinus
@ 2017-05-22 18:24         ` Eli Zaretskii
  2017-05-25  8:12           ` Michael Albinus
  0 siblings, 1 reply; 29+ messages in thread
From: Eli Zaretskii @ 2017-05-22 18:24 UTC (permalink / raw)
  To: Michael Albinus; +Cc: 26973, dgutov

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Dmitry Gutov <dgutov@yandex.ru>,  26973@debbugs.gnu.org
> Date: Mon, 22 May 2017 09:52:45 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Does anything change if you set auto-revert-use-notify to a nil value?
> >> 
> >> Yes: that seems to fix the problem.
> >> 
> >> In anticipation of the next question, my value of file-notify--library 
> >> is `inotify'.
> >
> > Right, that's what I thought.  I'm sure Michael (CC'ed) will look into
> > this.
> 
> Will do. But this might take time; sleep-for and accept-process-output
> are terra incognita for me until now.

Let us know if you need any help.

Thanks.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-22  4:06         ` Eli Zaretskii
@ 2017-05-24  0:58           ` Dmitry Gutov
  2017-05-24 18:33             ` Eli Zaretskii
  0 siblings, 1 reply; 29+ messages in thread
From: Dmitry Gutov @ 2017-05-24  0:58 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 26973, michael.albinus

On 5/22/17 7:06 AM, Eli Zaretskii wrote:

>> BTW, accept-process-output also seems broken in similar scenarios.
> 
> Of course: both accept-process-output and sleep-for call the same
> low-level API.

Does sit-for call the same API?

Because replacing (sleep-for 3) with (sit-for 3), or even (sit-for 3 t), 
avoids triggering the problem described here.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-24  0:58           ` Dmitry Gutov
@ 2017-05-24 18:33             ` Eli Zaretskii
  0 siblings, 0 replies; 29+ messages in thread
From: Eli Zaretskii @ 2017-05-24 18:33 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 26973, michael.albinus

> Cc: michael.albinus@gmx.de, 26973@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 24 May 2017 03:58:01 +0300
> 
> On 5/22/17 7:06 AM, Eli Zaretskii wrote:
> 
> >> BTW, accept-process-output also seems broken in similar scenarios.
> > 
> > Of course: both accept-process-output and sleep-for call the same
> > low-level API.
> 
> Does sit-for call the same API?

It calls it in a slightly different manner.

> Because replacing (sleep-for 3) with (sit-for 3), or even (sit-for 3 t), 
> avoids triggering the problem described here.

Yes, I understand.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-22 18:24         ` Eli Zaretskii
@ 2017-05-25  8:12           ` Michael Albinus
  2017-05-25  9:45             ` Andreas Politz
  0 siblings, 1 reply; 29+ messages in thread
From: Michael Albinus @ 2017-05-25  8:12 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 26973, Andreas Politz, dgutov

Eli Zaretskii <eliz@gnu.org> writes:

>> Will do. But this might take time; sleep-for and accept-process-output
>> are terra incognita for me until now.
>
> Let us know if you need any help.

I've tried to bisect the master branch in order to see when the problem
started. Kind of impossible, due to the heavy changes in configuration
machinery of Emacs last weeks.

So I've followed the commits for inotify.c. With commit b2a83eed23 the
problem doesn't happen. It started to appear with next commit for
inotify.c, 158bb8555d.

I've Cc'ed Andreas, who's the author of that commit.

> Thanks.

Best regards, Michael.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-25  8:12           ` Michael Albinus
@ 2017-05-25  9:45             ` Andreas Politz
  2017-05-26 14:45               ` Michael Albinus
  0 siblings, 1 reply; 29+ messages in thread
From: Andreas Politz @ 2017-05-25  9:45 UTC (permalink / raw)
  To: Michael Albinus; +Cc: 26973, dgutov

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


I seems that the event flags used are to promiscuous and that this
prevents other processes from reading.  Removing ACCESS, OPEN and
CLOSE events appears to rectify the situation.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff, Size: 1344 bytes --]

diff --git a/src/inotify.c b/src/inotify.c
index 290701349e..d43b959747 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -41,7 +41,21 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #ifndef IN_ONLYDIR
 # define IN_ONLYDIR 0
 #endif
-#define INOTIFY_DEFAULT_MASK (IN_ALL_EVENTS | IN_EXCL_UNLINK)
+#define INOTIFY_DEFAULT_MASK                                    \
+  (IN_ATTRIB |                                                  \
+   /* IN_ACCESS | */                                            \
+   /* IN_CLOSE_WRITE | */                                       \
+   /* IN_CLOSE_NOWRITE |  */                                    \
+   IN_CREATE |                                                  \
+   IN_DELETE |                                                  \
+   IN_DELETE_SELF |                                             \
+   IN_IGNORED |                                                 \
+   IN_MODIFY |                                                  \
+   IN_MOVE_SELF |                                               \
+   IN_MOVED_FROM |                                              \
+   IN_MOVED_TO |                                                \
+   /* IN_OPEN | */                                              \
+   IN_EXCL_UNLINK)
 
 /* File handle for inotify.  */
 static int inotifyfd = -1;

[-- Attachment #3: Type: text/plain, Size: 5 bytes --]


-ap

^ permalink raw reply related	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-25  9:45             ` Andreas Politz
@ 2017-05-26 14:45               ` Michael Albinus
  2017-05-27  0:45                 ` Dmitry Gutov
  0 siblings, 1 reply; 29+ messages in thread
From: Michael Albinus @ 2017-05-26 14:45 UTC (permalink / raw)
  To: Andreas Politz; +Cc: 26973, dgutov

Andreas Politz <politza@hochschule-trier.de> writes:

Hi Andreas,

> I seems that the event flags used are to promiscuous and that this
> prevents other processes from reading.  Removing ACCESS, OPEN and
> CLOSE events appears to rectify the situation.

The patch ought to fix the bug, thanks. filenotify-tests.el pass also
all tests.

So I have committed the patch in your name. Dmitry, could you pls crosscheck?

> -ap

Best regards, Michael.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-17 23:57 bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled Dmitry Gutov
  2017-05-20 11:36 ` Eli Zaretskii
@ 2017-05-26 18:06 ` Paul Eggert
  2017-05-27 16:36   ` Andreas Politz
  1 sibling, 1 reply; 29+ messages in thread
From: Paul Eggert @ 2017-05-26 18:06 UTC (permalink / raw)
  To: Andreas Politz; +Cc: 26973, Michael Albinus, Dmitry Gutov

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

> the event flags used are to[o] promiscuous and that this
> prevents other processes from reading.  Removing ACCESS, OPEN and
> CLOSE events appears to rectify the situation.

This means that a later call to inotify-add-watch that lacks IN_OPEN 
will disable an already-existing watch that specifies IN_OPEN, right? 
That doesn't sound right.

I reviewed the inotify.c patches made since March and spotted what 
appear to be some problems related to this. What do you think of the 
attached patches? The first patch doesn't change behavior; it merely 
makes the later patches easier to write. The second patch restores 
onlydir (it appears to have been removed by mistake). The third patch 
uses IN_MASK_ADD instead of a promiscuous mask.


[-- Attachment #2: 0001-Simplify-computation-of-inotify-mask.patch --]
[-- Type: text/x-patch, Size: 2144 bytes --]

From 46eb61f91160f51e2c16c14b327a2d2f8145d186 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 26 May 2017 09:37:44 -0700
Subject: [PATCH 1/3] Simplify computation of inotify mask

* src/inotify.c (add_watch): Accept uint32_t imask instead
of Lisp_Object aspect.  Caller changed.
(Finotify_add_watch): Use aspect_to_inotifymask earlier, to
simplify the code.
---
 src/inotify.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/src/inotify.c b/src/inotify.c
index 1165293..bcf30ad 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -215,16 +215,15 @@ inotifyevent_to_event (Lisp_Object watch, struct inotify_event const *ev)
 }
 
 /* Add a new watch to watch-descriptor WD watching FILENAME and using
-   CALLBACK.  Returns a cons (DESCRIPTOR . ID) uniquely identifying the
-   new watch.  */
+   IMASK and CALLBACK.  Return a cons (DESCRIPTOR . ID) uniquely
+   identifying the new watch.  */
 static Lisp_Object
 add_watch (int wd, Lisp_Object filename,
-	   Lisp_Object aspect, Lisp_Object callback)
+	   uint32_t imask, Lisp_Object callback)
 {
   Lisp_Object descriptor = INTEGER_TO_CONS (wd);
   Lisp_Object tail = assoc_no_quit (descriptor, watch_list);
   Lisp_Object watch, watch_id;
-  uint32_t imask = aspect_to_inotifymask (aspect);
   Lisp_Object mask = INTEGER_TO_CONS (imask);
 
   EMACS_INT id = 0;
@@ -436,12 +435,10 @@ IN_ONLYDIR  */)
      (Lisp_Object filename, Lisp_Object aspect, Lisp_Object callback)
 {
   Lisp_Object encoded_file_name;
-  bool dont_follow = (CONSP (aspect)
-		      ? ! NILP (Fmemq (Qdont_follow, aspect))
-		      : EQ (Qdont_follow, aspect));
   int wd = -1;
+  uint32_t imask = aspect_to_inotifymask (aspect);
   uint32_t mask = (INOTIFY_DEFAULT_MASK
-                   | (dont_follow ? IN_DONT_FOLLOW : 0));
+		   | (imask & IN_DONT_FOLLOW));
 
   CHECK_STRING (filename);
 
@@ -459,7 +456,7 @@ IN_ONLYDIR  */)
   if (wd < 0)
     report_file_notify_error ("Could not add watch for file", filename);
 
-  return add_watch (wd, filename, aspect, callback);
+  return add_watch (wd, filename, imask, callback);
 }
 
 static bool
-- 
2.9.4


[-- Attachment #3: 0002-Restore-inotify-onlydir-support.patch --]
[-- Type: text/x-patch, Size: 2465 bytes --]

From d8ada0da7041e524b97c4354facd8803716d92bf Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 26 May 2017 09:50:56 -0700
Subject: [PATCH 2/3] Restore inotify onlydir support

There was no need to remove it in the 2017-03-26 inotify change,
as it is like IN_DONT_FOLLOW and does not affect other watchers
for the same file.
* src/inotify.c (symbol_to_inotifymask, Finotify_add_watch)
(syms_of_inotify): Bring back onlydir.
---
 src/inotify.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/inotify.c b/src/inotify.c
index bcf30ad..b3e0728 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -72,10 +72,6 @@ static int inotifyfd = -1;
    IN_EXCL_UNLINK
    IN_MASK_ADD
    IN_ONESHOT
-   IN_ONLYDIR
-
-   FIXME: Explain why IN_ONLYDIR is in the list, as it seems to be
-   in the same category as IN_DONT_FOLLOW which is allowed.
 
    Each element of this list is of the form (DESCRIPTOR . WATCHES)
    where no two DESCRIPTOR values are the same.  DESCRIPTOR represents
@@ -162,6 +158,8 @@ symbol_to_inotifymask (Lisp_Object symb)
 
   else if (EQ (symb, Qdont_follow))
     return IN_DONT_FOLLOW;
+  else if (EQ (symb, Qonlydir))
+    return IN_ONLYDIR;
 
   else if (EQ (symb, Qt) || EQ (symb, Qall_events))
     return IN_ALL_EVENTS;
@@ -397,9 +395,11 @@ all-events or t
 move
 close
 
-The following symbols can also be added to a list of aspects:
+ASPECT can also contain the following symbols, which control whether
+the watch descriptor will be created:
 
 dont-follow
+onlydir
 
 Watching a directory is not recursive.  CALLBACK is passed a single argument
 EVENT which contains an event structure of the format
@@ -430,15 +430,14 @@ shared across different callers.
 
 IN_EXCL_UNLINK
 IN_MASK_ADD
-IN_ONESHOT
-IN_ONLYDIR  */)
+IN_ONESHOT  */)
      (Lisp_Object filename, Lisp_Object aspect, Lisp_Object callback)
 {
   Lisp_Object encoded_file_name;
   int wd = -1;
   uint32_t imask = aspect_to_inotifymask (aspect);
   uint32_t mask = (INOTIFY_DEFAULT_MASK
-		   | (imask & IN_DONT_FOLLOW));
+		   | (imask & (IN_DONT_FOLLOW | IN_ONLYDIR)));
 
   CHECK_STRING (filename);
 
@@ -548,6 +547,7 @@ syms_of_inotify (void)
   DEFSYM (Qclose, "close");		/* IN_CLOSE */
 
   DEFSYM (Qdont_follow, "dont-follow");	/* IN_DONT_FOLLOW */
+  DEFSYM (Qonlydir, "onlydir");		/* IN_ONLYDIR */
 
   DEFSYM (Qignored, "ignored");		/* IN_IGNORED */
   DEFSYM (Qisdir, "isdir");		/* IN_ISDIR */
-- 
2.9.4


[-- Attachment #4: 0003-Depromiscuify-inotify-with-IN_MASK_ADD.patch --]
[-- Type: text/x-patch, Size: 1968 bytes --]

From 0c581863f4b1f3ef31397357dd454fe68416e645 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 26 May 2017 10:53:02 -0700
Subject: [PATCH 3/3] Depromiscuify inotify with IN_MASK_ADD

Use IN_MASK_ADD instead of using a no-longer-promiscuous-enough
mask.  Without this fix, a later watch that omits IN_OPEN disables
an earlier watch that uses IN_OPEN (Bug#26973).
* src/inotify.c (INOTIFY_DEFAULT_MASK): Remove.
(Finotify_add_watch): Use IN_MASK_ADD instead.
---
 src/inotify.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/src/inotify.c b/src/inotify.c
index b3e0728..3d5d3d2 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -42,21 +42,6 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 # define IN_ONLYDIR 0
 #endif
 
-/* Events that inotify-add-watch waits for.  This list has all the
-   events that any watcher could include, because we want to support
-   multiple watches on the same file even though inotify uses the same
-   descriptor for all watches to that file.  This list omits
-   IN_ACCESS, IN_CLOSE_WRITE, IN_CLOSE_NOWRITE, and IN_OPEN because
-   they would prevent other processes from reading; see Bug#26973.
-
-   FIXME: Explain why it is OK to omit these four bits here even
-   though a inotify-add-watch call might specify them.  */
-
-#define INOTIFY_DEFAULT_MASK                                    \
-  (IN_ATTRIB | IN_CREATE | IN_DELETE | IN_DELETE_SELF		\
-   | IN_IGNORED | IN_MODIFY | IN_MOVE_SELF | IN_MOVED_FROM	\
-   | IN_MOVED_TO | IN_EXCL_UNLINK)
-
 /* File handle for inotify.  */
 static int inotifyfd = -1;
 
@@ -436,8 +421,7 @@ IN_ONESHOT  */)
   Lisp_Object encoded_file_name;
   int wd = -1;
   uint32_t imask = aspect_to_inotifymask (aspect);
-  uint32_t mask = (INOTIFY_DEFAULT_MASK
-		   | (imask & (IN_DONT_FOLLOW | IN_ONLYDIR)));
+  uint32_t mask = imask | IN_MASK_ADD | IN_EXCL_UNLINK;
 
   CHECK_STRING (filename);
 
-- 
2.9.4


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-26 14:45               ` Michael Albinus
@ 2017-05-27  0:45                 ` Dmitry Gutov
  2017-05-27  7:33                   ` Michael Albinus
  0 siblings, 1 reply; 29+ messages in thread
From: Dmitry Gutov @ 2017-05-27  0:45 UTC (permalink / raw)
  To: Michael Albinus, Andreas Politz; +Cc: 26973

On 5/26/17 5:45 PM, Michael Albinus wrote:

> The patch ought to fix the bug, thanks. filenotify-tests.el pass also
> all tests.
> 
> So I have committed the patch in your name. Dmitry, could you pls crosscheck?

Looks fixed to me as well, thank you.

But I'm also looking forward to your response to Paul's concerns





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-27  0:45                 ` Dmitry Gutov
@ 2017-05-27  7:33                   ` Michael Albinus
  0 siblings, 0 replies; 29+ messages in thread
From: Michael Albinus @ 2017-05-27  7:33 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: 26973, Andreas Politz

Dmitry Gutov <dgutov@yandex.ru> writes:

> But I'm also looking forward to your response to Paul's concerns

I let this for Andreas.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-26 18:06 ` Paul Eggert
@ 2017-05-27 16:36   ` Andreas Politz
  2017-05-27 18:19     ` Paul Eggert
  0 siblings, 1 reply; 29+ messages in thread
From: Andreas Politz @ 2017-05-27 16:36 UTC (permalink / raw)
  To: Paul Eggert; +Cc: 26973, Michael Albinus, Dmitry Gutov


I think your patches are against an older revision.

Paul Eggert <eggert@cs.ucla.edu> writes:

> [...] a later call to inotify-add-watch that lacks IN_OPEN will
> disable an already-existing watch that specifies IN_OPEN, right? [...]

I don't think so. It means that no one can watch for open/close/access
events, because the flags from the caller are ignored when invoking the
library function (except for IN_DONT_FOLLOW, which in my understanding
does not change the underlying watch).

> [...] The second patch restores onlydir (it appears to have been
> removed by mistake). 

See above, I guess.

> The third patch uses IN_MASK_ADD instead of a promiscuous mask.

We could do that.

-ap





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-27 16:36   ` Andreas Politz
@ 2017-05-27 18:19     ` Paul Eggert
  2017-05-27 21:13       ` Andreas Politz
  0 siblings, 1 reply; 29+ messages in thread
From: Paul Eggert @ 2017-05-27 18:19 UTC (permalink / raw)
  To: Andreas Politz; +Cc: 26973-done, Michael Albinus, Dmitry Gutov

Andreas Politz wrote:

> I think your patches are against an older revision.

OK, I rebased the patches to master and installed them there (after adjusting to 
the comments below) so you shouldn't have to worry about version mismatch.

>> [...] a later call to inotify-add-watch that lacks IN_OPEN will
>> disable an already-existing watch that specifies IN_OPEN, right? [...]
> 
> I don't think so. It means that no one can watch for open/close/access
> events

Oh, right. I fixed the commit message accordingly.

>> [...] The second patch restores onlydir (it appears to have been
>> removed by mistake).
> 
> See above, I guess.

Yeah, I checked the GNU/Linux source code, and it is a flag that affects only 
the inotify_add_watch call itself, not the watch descriptor, so we don't need to 
worry about it colliding with other watches.

>> The third patch uses IN_MASK_ADD instead of a promiscuous mask.
> 
> We could do that.

Thanks, it's now done as part of the installed patches. I verified that the 
originally-reported bug is still fixed in master, and am marking the bug as done.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-27 18:19     ` Paul Eggert
@ 2017-05-27 21:13       ` Andreas Politz
  2017-05-27 21:29         ` Paul Eggert
  0 siblings, 1 reply; 29+ messages in thread
From: Andreas Politz @ 2017-05-27 21:13 UTC (permalink / raw)
  To: Paul Eggert; +Cc: 26973-done, Michael Albinus, Dmitry Gutov

Paul Eggert <eggert@cs.ucla.edu> writes:

>>> [...] The second patch restores onlydir [...].

> Yeah, I checked the GNU/Linux source code, and it is a flag that
> affects only the inotify_add_watch call itself [...]

OK, I haven't been that thorough.  So, I suppose the call will fail for
a combination of onlydir with a non-directory filename.

>>> The third patch uses IN_MASK_ADD instead of a promiscuous mask.

But we still need to inhibit open/close/access events from being used by
any client in order to fix this bug.  (Unless someone finds a better way
by looking more closely into the problem relating to processes. Though
filenotify.el does not use them and so it may not be worth it.)

-ap





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-27 21:13       ` Andreas Politz
@ 2017-05-27 21:29         ` Paul Eggert
  2017-05-27 21:56           ` Andreas Politz
  2017-05-28  9:19           ` Andreas Politz
  0 siblings, 2 replies; 29+ messages in thread
From: Paul Eggert @ 2017-05-27 21:29 UTC (permalink / raw)
  To: Andreas Politz; +Cc: 26973-done, Michael Albinus, Dmitry Gutov

Andreas Politz wrote:
> I suppose the call will fail for
> a combination of onlydir with a non-directory filename.

Yes, that's right.

> we still need to inhibit open/close/access events from being used by
> any client in order to fix this bug.  (Unless someone finds a better way
> by looking more closely into the problem relating to processes. Though
> filenotify.el does not use them and so it may not be worth it.)

I didn't observe the problem with a little test case involving OPEN that I wrote 
myself. I agree it may not be worth looking into it. That being said, if you can 
easily explain the bug or supply a test case I can briefly look into fixing it.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-27 21:29         ` Paul Eggert
@ 2017-05-27 21:56           ` Andreas Politz
  2017-05-28 18:21             ` Paul Eggert
  2017-05-28  9:19           ` Andreas Politz
  1 sibling, 1 reply; 29+ messages in thread
From: Andreas Politz @ 2017-05-27 21:56 UTC (permalink / raw)
  To: Paul Eggert; +Cc: 26973-done, Michael Albinus, Dmitry Gutov

Paul Eggert <eggert@cs.ucla.edu> writes:

> I didn't observe the problem with a little test case involving OPEN
> that I wrote myself. I agree it may not be worth looking into it. That
> being said, if you can easily explain the bug or supply a test case I
> can briefly look into fixing it.

I didn't really look into it and just assumed that one of those flags
might trigger a circular transition preventing processes from reading.
For that I used the posted function, added a watch to the /tmp directory
and noticed that the problematic behavior occured, if any one of those
flags (open/close/access) were added.

-ap





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-27 21:29         ` Paul Eggert
  2017-05-27 21:56           ` Andreas Politz
@ 2017-05-28  9:19           ` Andreas Politz
  2017-05-28 15:13             ` Eli Zaretskii
  2017-06-04 11:49             ` Michael Albinus
  1 sibling, 2 replies; 29+ messages in thread
From: Andreas Politz @ 2017-05-28  9:19 UTC (permalink / raw)
  To: Paul Eggert; +Cc: 26973-done, Michael Albinus, Dmitry Gutov


It just occured to me that the other back-ends may exhibit the same
behavior, when used outside of filenotify.el . For example w32notify
provides the flag

  'last-access-time'   -- report changes in last-access time

-ap





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-28  9:19           ` Andreas Politz
@ 2017-05-28 15:13             ` Eli Zaretskii
  2017-06-04 11:49             ` Michael Albinus
  1 sibling, 0 replies; 29+ messages in thread
From: Eli Zaretskii @ 2017-05-28 15:13 UTC (permalink / raw)
  To: Andreas Politz; +Cc: 26973, eggert, michael.albinus, dgutov

> From: Andreas Politz <politza@hochschule-trier.de>
> Date: Sun, 28 May 2017 11:19:17 +0200
> Cc: 26973-done@debbugs.gnu.org, Michael Albinus <michael.albinus@gmx.de>,
> 	Dmitry Gutov <dgutov@yandex.ru>
> 
> 
> It just occured to me that the other back-ends may exhibit the same
> behavior, when used outside of filenotify.el . For example w32notify
> provides the flag
> 
>   'last-access-time'   -- report changes in last-access time

Sorry, I've lost track of the discussion -- could you please tell what
"same behavior" did you refer to, and what could be the problem with
the last-access-time flag provided by w32notify.c?

Thanks.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-27 21:56           ` Andreas Politz
@ 2017-05-28 18:21             ` Paul Eggert
  2017-05-28 21:18               ` Andreas Politz
  0 siblings, 1 reply; 29+ messages in thread
From: Paul Eggert @ 2017-05-28 18:21 UTC (permalink / raw)
  To: Andreas Politz; +Cc: 26973-done, Michael Albinus, Dmitry Gutov

Andreas Politz wrote:
> For that I used the posted function, added a watch to the /tmp directory
> and noticed that the problematic behavior occured, if any one of those
> flags (open/close/access) were added.

I couldn't reproduce the problem with the current master (commit 288b3ca). That 
is, I ran src/emacs -Q with this:

M-x load-file RET sfp-test.el RET
M-x global-auto-revert-mode RET
M-x start-file-process-test RET
(inotify-add-watch "/tmp" 'open (lambda (event) (message "event %s" event))) C-x C-e
M-x start-file-process-test RET

Both instances of start-file-process-test did "process done!" before "sleeping 
done!", which I gather is what is wanted.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-28 18:21             ` Paul Eggert
@ 2017-05-28 21:18               ` Andreas Politz
  0 siblings, 0 replies; 29+ messages in thread
From: Andreas Politz @ 2017-05-28 21:18 UTC (permalink / raw)
  To: Paul Eggert; +Cc: 26973-done, Michael Albinus, Dmitry Gutov

Paul Eggert <eggert@cs.ucla.edu> writes:

> I couldn't reproduce the problem with the current master (commit
> 288b3ca). That is, I ran src/emacs -Q with this:

I can't either anymore.

-ap





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-05-28  9:19           ` Andreas Politz
  2017-05-28 15:13             ` Eli Zaretskii
@ 2017-06-04 11:49             ` Michael Albinus
  2017-06-04 14:00               ` Eli Zaretskii
  1 sibling, 1 reply; 29+ messages in thread
From: Michael Albinus @ 2017-06-04 11:49 UTC (permalink / raw)
  To: Andreas Politz; +Cc: 26973-done, Paul Eggert, Dmitry Gutov

Andreas Politz <politza@hochschule-trier.de> writes:

> It just occured to me that the other back-ends may exhibit the same
> behavior, when used outside of filenotify.el . For example w32notify
> provides the flag
>
>   'last-access-time'   -- report changes in last-access time

I've checked the kqueue and gfilenotify cases; the problem does not
occur. I cannot check w32notify.

> -ap

Best regards, Michael.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-06-04 11:49             ` Michael Albinus
@ 2017-06-04 14:00               ` Eli Zaretskii
  2017-06-04 15:30                 ` Andreas Politz
  0 siblings, 1 reply; 29+ messages in thread
From: Eli Zaretskii @ 2017-06-04 14:00 UTC (permalink / raw)
  To: Michael Albinus; +Cc: 26973-done, eggert, politza, dgutov

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Sun, 04 Jun 2017 13:49:27 +0200
> Cc: 26973-done@debbugs.gnu.org, Paul Eggert <eggert@cs.ucla.edu>,
> 	Dmitry Gutov <dgutov@yandex.ru>
> 
> Andreas Politz <politza@hochschule-trier.de> writes:
> 
> > It just occured to me that the other back-ends may exhibit the same
> > behavior, when used outside of filenotify.el . For example w32notify
> > provides the flag
> >
> >   'last-access-time'   -- report changes in last-access time
> 
> I've checked the kqueue and gfilenotify cases; the problem does not
> occur. I cannot check w32notify.

Like I said: I've lost track of this discussion.  So could someone
please tell what could be "the problem", and in what situations it
might happen?  Then I could try test that with w32notify.

Thanks.





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-06-04 14:00               ` Eli Zaretskii
@ 2017-06-04 15:30                 ` Andreas Politz
  2017-06-04 16:20                   ` Eli Zaretskii
  0 siblings, 1 reply; 29+ messages in thread
From: Andreas Politz @ 2017-06-04 15:30 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 26973-done, eggert, Michael Albinus, dgutov

Eli Zaretskii <eliz@gnu.org> writes:

> Like I said: I've lost track of this discussion.  [...]

Activating global-auto-revert-mode seems to lead to starvation of the
collection of output from running processes in an inotify build.
Inhibiting the generation of open, close and access file-events seems to
have fixed this problem.

This lead to the hypotheses that, in case the above types of events are
reported, reading file-events while idling may by itself lead to the
generation of more file-events and thus making this process stuck in a
cycle.

Autorevert resp. filenotify.el does not include these events in its
event-mask passed to the various back-ends, such that this problem could
not be perceived, unless the back-ends are used directly.  (Except for
inotify, were a constant, promiscuous mask was previously used in order
to work around a different problem.)

So, if the above is true, other back-ends could exhibit the same
behavior, if they were to be instructed to generate open/close/access
events.  Again, since these events are not used by filenotify.el, it
would only show, if the back-end is invoked directly,
e.g. w32notify-add-watch with last-access-time.

-ap





^ permalink raw reply	[flat|nested] 29+ messages in thread

* bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled
  2017-06-04 15:30                 ` Andreas Politz
@ 2017-06-04 16:20                   ` Eli Zaretskii
  0 siblings, 0 replies; 29+ messages in thread
From: Eli Zaretskii @ 2017-06-04 16:20 UTC (permalink / raw)
  To: Andreas Politz; +Cc: 26973, eggert, michael.albinus, dgutov

> From: Andreas Politz <politza@hochschule-trier.de>
> Cc: Michael Albinus <michael.albinus@gmx.de>,  26973-done@debbugs.gnu.org,  eggert@cs.ucla.edu,  dgutov@yandex.ru
> Date: Sun, 04 Jun 2017 17:30:15 +0200
> 
> Activating global-auto-revert-mode seems to lead to starvation of the
> collection of output from running processes in an inotify build.
> Inhibiting the generation of open, close and access file-events seems to
> have fixed this problem.

I thought this was inotify-specific, because of the flags used in that
back-end, is that correct?  w32notify uses only the flags specified by
the caller, it doesn't add any flags of its own.

> This lead to the hypotheses that, in case the above types of events are
> reported, reading file-events while idling may by itself lead to the
> generation of more file-events and thus making this process stuck in a
> cycle.

I'm not sure I understand how this could happen.  How can reading
events generate those same events?  Also, is this still related to
invoking sub-processes, or unrelated?

> So, if the above is true, other back-ends could exhibit the same
> behavior, if they were to be instructed to generate open/close/access
> events.  Again, since these events are not used by filenotify.el, it
> would only show, if the back-end is invoked directly,
> e.g. w32notify-add-watch with last-access-time.

So you are saying that if I install a watch for last-access-time
changes, and then change the last-access time of a watched file, I
could have more than one event read by Emacs?  If so, that's not what
I see: I see exactly one event for each change of the last-access
time.

Or should I try something else?  I can easily post the code and the
procedure I used, if that would help clarify the issue.

Thanks.





^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2017-06-04 16:20 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-17 23:57 bug#26973: 26.0.50; sleep-for behavior changes with global-auto-revert-mode enabled Dmitry Gutov
2017-05-20 11:36 ` Eli Zaretskii
2017-05-20 12:45   ` Dmitry Gutov
2017-05-20 13:38     ` Eli Zaretskii
2017-05-21 22:38       ` Dmitry Gutov
2017-05-22  4:06         ` Eli Zaretskii
2017-05-24  0:58           ` Dmitry Gutov
2017-05-24 18:33             ` Eli Zaretskii
2017-05-22  7:52       ` Michael Albinus
2017-05-22 18:24         ` Eli Zaretskii
2017-05-25  8:12           ` Michael Albinus
2017-05-25  9:45             ` Andreas Politz
2017-05-26 14:45               ` Michael Albinus
2017-05-27  0:45                 ` Dmitry Gutov
2017-05-27  7:33                   ` Michael Albinus
2017-05-26 18:06 ` Paul Eggert
2017-05-27 16:36   ` Andreas Politz
2017-05-27 18:19     ` Paul Eggert
2017-05-27 21:13       ` Andreas Politz
2017-05-27 21:29         ` Paul Eggert
2017-05-27 21:56           ` Andreas Politz
2017-05-28 18:21             ` Paul Eggert
2017-05-28 21:18               ` Andreas Politz
2017-05-28  9:19           ` Andreas Politz
2017-05-28 15:13             ` Eli Zaretskii
2017-06-04 11:49             ` Michael Albinus
2017-06-04 14:00               ` Eli Zaretskii
2017-06-04 15:30                 ` Andreas Politz
2017-06-04 16:20                   ` Eli Zaretskii

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.