unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#18861: 25.0.50; gfile-based file notifications are not immediate
@ 2014-10-28  0:29 Dima Kogan
       [not found] ` <handler.18861.B.14144561857404.ack@debbugs.gnu.org>
  0 siblings, 1 reply; 9+ messages in thread
From: Dima Kogan @ 2014-10-28  0:29 UTC (permalink / raw)
  To: 18861

This is a bug-tracker entry for this mailing list post:

 http://lists.gnu.org/archive/html/emacs-devel/2014-10/msg00941.html

Contents of the post:

In the last few days I started several threads about various details of
file notification and auto-revert behavior. This one is about some
incorrect behavior of notifications using the 'gfile' (gio from glib)
backend.

As in the inotify thread, I set up a simple test. I built emacs using

 ./configure --with-file-notification=gfile

I then ran

 ./emacs --eval "`cat  /tmp/tstnotify.el`" -Q -nw

with tstnotify.el being

 (progn
   (require 'filenotify)

   (dolist (fil '("/tmp/tst1" "/tmp/tst2"))
     (file-notify-add-watch fil  '(change attribute-change)
                            (lambda (event)
                              (message "notify event %s" event)))
     (find-file fil))
   (switch-to-buffer "*Messages*"))


Here I ask for notifications for two files, and print out the events as
they come in. While emacs is running this way, I modify those two files
using an external tool. I would expect to see modification events for
both of these files, as soon as these modifications happen. Instead, the
notifications come in either 30 seconds later, or whenever anything goes
through emacs's input queue. So the notification is greatly delayed if
the user is not touching their emacs.

The reason is that when emacs is idle, it's blocking in the pselect()
call in xg_select() in process.c. pselect() is an operating-system call,
not a glib one. Later on in xg_select() there's some code to kick the
glib event loop, but as long as we're sitting in the pselect(), that
secondary event loop remains untouched.

Even if you kick the event loop in time, the notification is still
delayed a few seconds (I verified this with some test code, outside of
emacs; can post if anybody wants it). THIS is a separate issue that is
likely a bug in glib. On Linux it uses inotify internally, so if one was
using Linux, would there be any reason to use this notification scheme
instead of using inotify directly? Are there OSs where emacs supports
notifications ONLY through glib?





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

* bug#18861: Acknowledgement (25.0.50; gfile-based file notifications are not immediate)
       [not found] ` <handler.18861.B.14144561857404.ack@debbugs.gnu.org>
@ 2014-10-28  4:22   ` Dima Kogan
  2014-10-28 21:30     ` Dima Kogan
  0 siblings, 1 reply; 9+ messages in thread
From: Dima Kogan @ 2014-10-28  4:22 UTC (permalink / raw)
  To: 18861

I just looked into this, and it appears I spoke too soon. The emacs
event loop IS correct. It asks glib for a list of file descriptors that
need attention, and then calls select() on those and on other file
descriptors that emacs cares about. The bug is in glib. It appears that
the file descriptors it gives you don't get any activity when a
notification occurs, so calling select() on them does anything. Bug report:

 https://bugzilla.gnome.org/show_bug.cgi?id=739274

If I fix this bug then select() works, but there's a delay of about 1sec
between when the file modification is reported by inotify and when glib
tells you about it. This is yet another glib bug that I haven't yet
looked into.





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

* bug#18861: Acknowledgement (25.0.50; gfile-based file notifications are not immediate)
  2014-10-28  4:22   ` bug#18861: Acknowledgement (25.0.50; gfile-based file notifications are not immediate) Dima Kogan
@ 2014-10-28 21:30     ` Dima Kogan
  2014-10-30 16:12       ` Stefan Monnier
  0 siblings, 1 reply; 9+ messages in thread
From: Dima Kogan @ 2014-10-28 21:30 UTC (permalink / raw)
  To: 18861

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

Control: tags -1 + patch

Hi. A glib maintainer responded to the bug report, and it turns out
emacs was using glib slightly incorrectly. I'm attaching a patch to fix
the issue, as suggested by the maintainer.

With this patch, the pselect() call in emacs does see the glib
notification as it should. There is still an issue (also in glib) where
this notification comes about 1 second after the actual file system
activity, so I'm keeping this bug open. The glib bug tracker entry about
THIS problem is here:

 https://bugzilla.gnome.org/show_bug.cgi?id=739322


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-xg_select-now-acquires-the-glib-context-before-query.patch --]
[-- Type: text/x-diff, Size: 1961 bytes --]

From eb3579400f098a7cb43f55e48262c2939ff33254 Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Tue, 28 Oct 2014 14:29:01 -0700
Subject: [PATCH] xg_select() now acquires the glib context before querying it

Prior to this patch we were calling g_main_context_query() without calling
g_main_context_acquire(). This resulted in the file descriptors returned by
g_main_context_query() missing activity. I.e. something would happen in glib,
but a select() on the file descriptors would keep blocking.

We now acquire the context, which makes select() return on activity, as it
should.

This is emacs and glib bugs:

 http://debbugs.gnu.org/cgi/bugreport.cgi?bug=18861
 https://bugzilla.gnome.org/show_bug.cgi?id=739274
---
 src/xgselect.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/xgselect.c b/src/xgselect.c
index bf889a9..a830b7d 100644
--- a/src/xgselect.c
+++ b/src/xgselect.c
@@ -55,11 +55,20 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
   GPollFD *gfds = gfds_buf;
   int gfds_size = ARRAYELTS (gfds_buf);
   int n_gfds, retval = 0, our_fds = 0, max_fds = fds_lim - 1;
+  bool context_acquired = false;
   int i, nfds, tmo_in_millisec;
   bool need_to_dispatch;
   USE_SAFE_ALLOCA;
 
   context = g_main_context_default ();
+  if( g_main_context_acquire(context) != TRUE )
+    {
+      // we couldn't acquire the context. I let this function proceed because it
+      // handles more than just glib file descriptors
+      retval = -1;
+    }
+  else
+    context_acquired = true;
 
   if (rfds) all_rfds = *rfds;
   else FD_ZERO (&all_rfds);
@@ -152,6 +161,9 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
       errno = pselect_errno;
     }
 
+  if (context_acquired)
+    g_main_context_release(context);
+
   /* To not have to recalculate timeout, return like this.  */
   if ((our_fds > 0 || (nfds == 0 && tmop == &tmo)) && (retval == 0))
     {
-- 
2.0.0


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

* bug#18861: Acknowledgement (25.0.50; gfile-based file notifications are not immediate)
  2014-10-28 21:30     ` Dima Kogan
@ 2014-10-30 16:12       ` Stefan Monnier
  2014-11-05 19:19         ` Dima Kogan
  0 siblings, 1 reply; 9+ messages in thread
From: Stefan Monnier @ 2014-10-30 16:12 UTC (permalink / raw)
  To: Dima Kogan; +Cc: 18861

> Hi.  A glib maintainer responded to the bug report, and it turns out
> emacs was using glib slightly incorrectly.  I'm attaching a patch to fix
> the issue, as suggested by the maintainer.

Thanks.  This is not my area of expertise at all, but since it seems
that noone else has replied yet, I'll try to move it along.

> +  if( g_main_context_acquire(context) != TRUE )
> +    {
> +      // we couldn't acquire the context. I let this function proceed because it
> +      // handles more than just glib file descriptors
> +      retval = -1;
> +    }
> +  else
> +    context_acquired = true;

[ Please follow our coding conventions: put a space before open parens
  (and not after, nor before close parens); use /*...*/ for comments;
  capitalize and punctuate comments; use two spaces between sentences.
  Also, I think "!g_main_context_acquire (context)" would be cleaner than
  "g_main_context_acquire (context) != TRUE".  I find comparing to NULL,
  true, or false generally ugly, tho maybe that's just a personal taste
  of mine that others don't share.  ]

Why set retval to -1?  It seems safer to leave it unchanged (ie. set to 0).


        Stefan





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

* bug#18861: Acknowledgement (25.0.50; gfile-based file notifications are not immediate)
  2014-10-30 16:12       ` Stefan Monnier
@ 2014-11-05 19:19         ` Dima Kogan
  2014-11-06  3:02           ` Stefan Monnier
  0 siblings, 1 reply; 9+ messages in thread
From: Dima Kogan @ 2014-11-05 19:19 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 18861

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

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> Hi.  A glib maintainer responded to the bug report, and it turns out
>> emacs was using glib slightly incorrectly.  I'm attaching a patch to fix
>> the issue, as suggested by the maintainer.
>
> Thanks.  This is not my area of expertise at all, but since it seems
> that noone else has replied yet, I'll try to move it along.
>
>> +  if( g_main_context_acquire(context) != TRUE )
>> +    {
>> +      // we couldn't acquire the context. I let this function proceed because it
>> +      // handles more than just glib file descriptors
>> +      retval = -1;
>> +    }
>> +  else
>> +    context_acquired = true;
>
> [ Please follow our coding conventions: put a space before open parens
>   (and not after, nor before close parens); use /*...*/ for comments;
>   capitalize and punctuate comments; use two spaces between sentences.
>   Also, I think "!g_main_context_acquire (context)" would be cleaner than
>   "g_main_context_acquire (context) != TRUE".  I find comparing to NULL,
>   true, or false generally ugly, tho maybe that's just a personal taste
>   of mine that others don't share.  ]
>
> Why set retval to -1?  It seems safer to leave it unchanged (ie. set to 0).

OK. How about the attached? The error-handling logic is better, and we
don't touch glib if we couldn't acquire the context. However in this
patch if we COULDN'T talk to glib, but we COULD talk to our own file
descriptors, then xg_select() returns success, and there is no error
reporting. Probably not what we want.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-xg_select-now-acquires-the-glib-context-before-query.patch --]
[-- Type: text/x-diff, Size: 2488 bytes --]

From 1fcced5536c8d99032303fb0f547aa67a5cccd30 Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Tue, 28 Oct 2014 14:29:01 -0700
Subject: [PATCH] xg_select() now acquires the glib context before querying it

Prior to this patch we were calling g_main_context_query() without calling
g_main_context_acquire(). This resulted in the file descriptors returned by
g_main_context_query() missing activity. I.e. something would happen in glib,
but a select() on the file descriptors would keep blocking.

We now acquire the context, which makes select() return on activity, as it
should.

This is emacs and glib bugs:

 http://debbugs.gnu.org/cgi/bugreport.cgi?bug=18861
 https://bugzilla.gnome.org/show_bug.cgi?id=739274
---
 src/xgselect.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/xgselect.c b/src/xgselect.c
index bf889a9..7dee38d 100644
--- a/src/xgselect.c
+++ b/src/xgselect.c
@@ -55,19 +55,28 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
   GPollFD *gfds = gfds_buf;
   int gfds_size = ARRAYELTS (gfds_buf);
   int n_gfds, retval = 0, our_fds = 0, max_fds = fds_lim - 1;
-  int i, nfds, tmo_in_millisec;
+  bool context_acquired = false;
+  int i, nfds, tmo_in_millisec = -1;
   bool need_to_dispatch;
   USE_SAFE_ALLOCA;
 
+  /* If we couldn't acquire the context, I let this function proceed because it
+     handles more than just glib file descriptors.  Note that, as implemented,
+     this failure is completely silent: there is no feedback to the caller.  */
   context = g_main_context_default ();
+  if (g_main_context_acquire(context))
+      context_acquired = true;
 
   if (rfds) all_rfds = *rfds;
   else FD_ZERO (&all_rfds);
   if (wfds) all_wfds = *wfds;
   else FD_ZERO (&all_wfds);
 
-  n_gfds = g_main_context_query (context, G_PRIORITY_LOW, &tmo_in_millisec,
-				 gfds, gfds_size);
+  n_gfds = context_acquired ?
+      g_main_context_query (context, G_PRIORITY_LOW, &tmo_in_millisec,
+                            gfds, gfds_size) :
+      -1;
+
   if (gfds_size < n_gfds)
     {
       SAFE_NALLOCA (gfds, sizeof *gfds, n_gfds);
@@ -152,6 +161,9 @@ xg_select (int fds_lim, fd_set *rfds, fd_set *wfds, fd_set *efds,
       errno = pselect_errno;
     }
 
+  if (context_acquired)
+    g_main_context_release(context);
+
   /* To not have to recalculate timeout, return like this.  */
   if ((our_fds > 0 || (nfds == 0 && tmop == &tmo)) && (retval == 0))
     {
-- 
2.0.0


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

* bug#18861: Acknowledgement (25.0.50; gfile-based file notifications are not immediate)
  2014-11-05 19:19         ` Dima Kogan
@ 2014-11-06  3:02           ` Stefan Monnier
  2014-11-06  8:37             ` Michael Albinus
  2014-11-06 18:26             ` Dima Kogan
  0 siblings, 2 replies; 9+ messages in thread
From: Stefan Monnier @ 2014-11-06  3:02 UTC (permalink / raw)
  To: Dima Kogan; +Cc: 18861-done

Thanks Dima,

Seeing as noone made any comment, I just installed your patch (with minor
tweaks) and hope it does the right thing.


        Stefan


> +  if (g_main_context_acquire(context))
                              ^^
Missing space before paren.

> +  n_gfds = context_acquired ?
> +      g_main_context_query (context, G_PRIORITY_LOW, &tmo_in_millisec,
> +                            gfds, gfds_size) :
> +      -1;

The GNU coding style recommends to break lines before rather than after
operators.

> +    g_main_context_release(context);
                            ^^
Same.





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

* bug#18861: Acknowledgement (25.0.50; gfile-based file notifications are not immediate)
  2014-11-06  3:02           ` Stefan Monnier
@ 2014-11-06  8:37             ` Michael Albinus
  2014-11-06 18:26             ` Dima Kogan
  1 sibling, 0 replies; 9+ messages in thread
From: Michael Albinus @ 2014-11-06  8:37 UTC (permalink / raw)
  To: 18861; +Cc: dima

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Seeing as noone made any comment, I just installed your patch (with minor
> tweaks) and hope it does the right thing.

Thanks. It is still on my list to step through all the file
notifications discussion from the last 3 weeks. But this list is much
too long, and I've started with Tramp isues.

>         Stefan

Best regards, Michael.





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

* bug#18861: Acknowledgement (25.0.50; gfile-based file notifications are not immediate)
  2014-11-06  3:02           ` Stefan Monnier
  2014-11-06  8:37             ` Michael Albinus
@ 2014-11-06 18:26             ` Dima Kogan
  2014-11-06 23:35               ` Stefan Monnier
  1 sibling, 1 reply; 9+ messages in thread
From: Dima Kogan @ 2014-11-06 18:26 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 18861

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Seeing as noone made any comment, I just installed your patch (with minor
> tweaks) and hope it does the right thing.

Thank you very much, Stefan. Seeing as how there's still a 1-second
delay here that's caused by glib, should this bug remain open? There
were 2 issues here, and the patch only fixes one of them. glib bugzilla:

 https://bugzilla.gnome.org/show_bug.cgi?id=739322
 https://bugzilla.gnome.org/show_bug.cgi?id=627285

This isn't strictly under our control, so maybe it does make sense to
close?





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

* bug#18861: Acknowledgement (25.0.50; gfile-based file notifications are not immediate)
  2014-11-06 18:26             ` Dima Kogan
@ 2014-11-06 23:35               ` Stefan Monnier
  0 siblings, 0 replies; 9+ messages in thread
From: Stefan Monnier @ 2014-11-06 23:35 UTC (permalink / raw)
  To: Dima Kogan; +Cc: 18861-done

> This isn't strictly under our control, so maybe it does make sense to
> close?

Yes, closing.


        Stefan





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

end of thread, other threads:[~2014-11-06 23:35 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-28  0:29 bug#18861: 25.0.50; gfile-based file notifications are not immediate Dima Kogan
     [not found] ` <handler.18861.B.14144561857404.ack@debbugs.gnu.org>
2014-10-28  4:22   ` bug#18861: Acknowledgement (25.0.50; gfile-based file notifications are not immediate) Dima Kogan
2014-10-28 21:30     ` Dima Kogan
2014-10-30 16:12       ` Stefan Monnier
2014-11-05 19:19         ` Dima Kogan
2014-11-06  3:02           ` Stefan Monnier
2014-11-06  8:37             ` Michael Albinus
2014-11-06 18:26             ` Dima Kogan
2014-11-06 23:35               ` Stefan Monnier

Code repositories for project(s) associated with this public inbox

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).