After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 635765 - Remove the virtual emission of G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT
Remove the virtual emission of G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: gio
2.27.x
Other Linux
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2010-11-25 10:00 UTC by Aleksander Morgado
Modified: 2018-05-24 12:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch for the issue (23.11 KB, patch)
2010-11-25 10:04 UTC, Aleksander Morgado
none Details | Review
Fix some indentation/alignment issues (17.90 KB, patch)
2010-11-25 16:26 UTC, Aleksander Morgado
none Details | Review
Patch for the new method (7.47 KB, patch)
2010-11-25 16:27 UTC, Aleksander Morgado
none Details | Review
Patch for the new method, doc updated (7.52 KB, patch)
2010-11-25 17:10 UTC, Aleksander Morgado
none Details | Review
[PATCH 1/2] gio: Fix some minor indentation/alignment issues (16.41 KB, patch)
2010-12-08 18:53 UTC, Aleksander Morgado
none Details | Review
[PATCH 2/2] gio: New method, g_file_monitor_set_changes_done_hint_delay() (7.58 KB, patch)
2010-12-08 18:54 UTC, Aleksander Morgado
none Details | Review
Tester of the new API method (3.22 KB, text/plain)
2010-12-08 19:08 UTC, Aleksander Morgado
  Details
patch to remove virtual emission of _EVENT_CHANGES_DONE_HINT (3.53 KB, patch)
2011-01-12 17:36 UTC, Cosimo Alfarano
rejected Details | Review
tester for path to remove virtual emission of _EVENT_CHANGES_DONE_HINT (2.67 KB, text/plain)
2011-01-12 17:43 UTC, Cosimo Alfarano
  Details

Description Aleksander Morgado 2010-11-25 10:00:25 UTC
G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT is currently issued in two cases:
 * If a file is updated, not closed, and the last update happened more than 2 seconds ago (hardcoded value).
 * If a file is updated and closed.

The 2 second delay could be configurable, and also possible to fully disable passing 0 as delay, so that CHANGES_DONE_HINT is only issued once a file has been closed after an update.
Comment 1 Aleksander Morgado 2010-11-25 10:04:35 UTC
Created attachment 175231 [details] [review]
Patch for the issue

This patch implements a new g_file_monitor_set_changes_done_hint_delay() method which will configure the internal delay for emitting virtual CHANGES_DONE_HINT events. If the delay is set to 0, these events will only get emitted when a file is updated and closed.

Also fixed several indentation/alignment issues in the same patch.
Comment 2 Christian Dywan 2010-11-25 11:38:01 UTC
It's very hard to see the relevant changes with all the noise :-)

It seems to me 0 could be confusing because you normally read it as "no delay before the emission of the event" and you wouldn't expect semantics to change. This being a signed integer already, I find -1 a better choice because it visibly shows a special case.
Comment 3 Aleksander Morgado 2010-11-25 11:49:30 UTC
(In reply to comment #2)
> It's very hard to see the relevant changes with all the noise :-)
> 

Yeah, sorry for that...

> It seems to me 0 could be confusing because you normally read it as "no delay
> before the emission of the event" and you wouldn't expect semantics to change.
> This being a signed integer already, I find -1 a better choice because it
> visibly shows a special case.

Well, the thing is that a 0 delay wouldn't make much sense... This is, you will be probably issuing CHANGED and CHANGES_DONE_HINT together always.
Comment 4 Aleksander Morgado 2010-11-25 16:26:13 UTC
Created attachment 175246 [details] [review]
Fix some indentation/alignment issues

This patch fixes the indentation/alignment issues in the gfilemonitor.c file
Comment 5 Aleksander Morgado 2010-11-25 16:27:22 UTC
Created attachment 175247 [details] [review]
Patch for the new method

This patch contains only the code related to the new method, easier to review now
Comment 6 Christian Dywan 2010-11-25 16:55:57 UTC
Thanks for separating the cleanup.

How about making the last sentence "Set to 0 to fully disable..." a separate paragraph and making it a bit more explicit? Something like "So events are only issued when a file has been closed. No event will be emitted as long as the file descriptor is open." Then I think that would prevent the confusion.
Comment 7 Aleksander Morgado 2010-11-25 17:10:51 UTC
Created attachment 175250 [details] [review]
Patch for the new method, doc updated

Thanks for the comments Christian.

New patch attached with the documentation updated.
Comment 8 Lionel Dricot 2010-12-08 11:06:38 UTC
Is there any usecase to try out the patch ?
Comment 9 Aleksander Morgado 2010-12-08 18:53:43 UTC
Created attachment 176084 [details] [review]
[PATCH 1/2] gio: Fix some minor indentation/alignment issues
Comment 10 Aleksander Morgado 2010-12-08 18:54:28 UTC
Created attachment 176085 [details] [review]
[PATCH 2/2] gio: New method, g_file_monitor_set_changes_done_hint_delay()

Patches rebased with git master.
Comment 11 Aleksander Morgado 2010-12-08 19:08:49 UTC
Created attachment 176088 [details]
Tester of the new API method

Attached a small C program using GFileMonitor. Expects the path of an existing directory as argument, for example: 
 $> glib-test-monitor /tmp/tempdir
Or, if you want to apply the new option to disable the virtual CHANGES_DONE_HINT:
 $> glib-test-monitor --disable-virtual /tmp/tempdir

Then, I use a small perl script to write in a file without closing it:
#!/usr/bin/perl
use IO::Handle;
@ARGV or die "give filename";
open my($f), '>', $ARGV[0];
while (1) {
    print $f "line here\n";
    $f->flush;
    sleep (10);
}
close $f;

Just execute that script passing the path of a file inside the directory you're monitoring with glib-test-monitor, for example:
 $> test.pl /tmp/tempdir/file.txt

When using the tester without --disable-virtual the tester doesn't use the new API method, so it behaves like git master glib:
$ ./glib-test-monitor /tmp/testdir
1291834727: Received 'G_FILE_MONITOR_EVENT_CREATED' event in 'file:///tmp/testdir/file.txt'
1291834727: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///tmp/testdir/file.txt'
1291834729: Received 'G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT' event in 'file:///tmp/testdir/file.txt'
1291834737: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///tmp/testdir/file.txt'
1291834739: Received 'G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT' event in 'file:///tmp/testdir/file.txt'
1291834747: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///tmp/testdir/file.txt'
1291834749: Received 'G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT' event in 'file:///tmp/testdir/file.txt'
1291834757: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///tmp/testdir/file.txt'
1291834759: Received 'G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT' event in 'file:///tmp/testdir/file.txt'
1291834760: Received 'G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT' event in 'file:///tmp/testdir/file.txt'

There's a CHANGED event every 10s, which is the update rate in the perl script; and 2 seconds after each update we get a CHANGES_DONE_HINT due to the virtual emission of the event; plus a last CHANGES_DONE_HINT when we stop the perl script and the file gets finally closed.

When using the new API method passing a 0 as delay so that the virtual CHANGES_DONE_HINT is disabled, we get the following output:
$ ./glib-test-monitor --disable-virtual /tmp/testdir
1291834859: Received 'G_FILE_MONITOR_EVENT_CREATED' event in 'file:///tmp/testdir/file.txt'
1291834859: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///tmp/testdir/file.txt'
1291834869: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///tmp/testdir/file.txt'
1291834879: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///tmp/testdir/file.txt'
1291834883: Received 'G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT' event in 'file:///tmp/testdir/file.txt'

In this case, there's also an UPDATED event every 10s, but we won't get any CHANGES_DONE_HINT event until the perl script ends and the file is closed.
Comment 12 Benjamin Otte (Company) 2010-12-17 14:58:55 UTC
Configuring a timeout for when changes are done sounds like an "unbreak me" setting. I could understand arguments being made for only ever emitting G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT when the file is closed, but configuring a timeout for when changes are done sounds like a very wrong API to export.

Could someone explain to me why we need a configurable timeout for that?
Comment 13 Aleksander Morgado 2011-01-10 08:10:44 UTC
(In reply to comment #12)
> Configuring a timeout for when changes are done sounds like an "unbreak me"
> setting. I could understand arguments being made for only ever emitting
> G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT when the file is closed, but configuring
> a timeout for when changes are done sounds like a very wrong API to export.
> 
> Could someone explain to me why we need a configurable timeout for that?

During a discussion in IRC it was agreed that the good fix would be to fully remove the virtual emission of CHANGES_DONE_HINT, so that it's only emitted on file close(). Email was sent to the devel ML to see if anyone had any comments on that:
http://mail.gnome.org/archives/gtk-devel-list/2010-December/msg00136.html

So I guess it should be ok to try to remove it, instead of setting the delay configurable. Will try to prepare a patch for that.
Comment 14 Cosimo Alfarano 2011-01-12 17:36:49 UTC
Created attachment 178158 [details] [review]
patch to remove virtual emission of _EVENT_CHANGES_DONE_HINT

(In reply to comment #13)
> So I guess it should be ok to try to remove it, instead of setting the delay
> configurable. Will try to prepare a patch for that.

I already have a patch for it, sorry :-)

It passes a modified version of comment #11 test, which I'll attach ASAP.

Can anyone review/integrate it, please?
Comment 15 Cosimo Alfarano 2011-01-12 17:43:04 UTC
Created attachment 178160 [details]
tester for path to remove virtual emission of _EVENT_CHANGES_DONE_HINT

tester, based on comment #11 one

Using this perl script (again based on comment #11 one):
----
#!/usr/bin/perl
use IO::Handle;
@ARGV or die "give filename";
open my($f), '>', $ARGV[0];
my $i = 0;

while ($i<3) {
    print $f "line here\n";
    $f->flush;
    sleep (10);
    $i=$i+1;
}

close ($f);
----

the expected output should be something like:

1294852554: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///tmp/tempdir/a'
1294852564: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///tmp/tempdir/a'
1294852574: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///tmp/tempdir/a'
1294852584: Received 'G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT' event in 'file:///tmp/tempdir/a'

Meaning that CHANGES_DONE_HINT is emitted only on closing and never 2 secs after the 3 updates.
Comment 16 Akhil Laddha 2011-10-14 05:39:31 UTC
ping, any resolution on patch ?
Comment 17 Simon McVittie 2012-01-13 14:14:29 UTC
I'm happy with Cosimo's patch, for what it's worth (but I'm not a GLib reviewer). Could a reviewer approve it, please? (Company, perhaps?)
Comment 18 Benjamin Otte (Company) 2012-01-13 14:23:32 UTC
You are more qualified to approve it than I am at this point. I guess you have two solutions now:
1) Commit it because you think it's the right thing to do
2) ping Alex or Ryan on IRC about it and see if they care

I'd probably go with (1), but I can take a beating when things break... ;)
Comment 19 Emmanuele Bassi (:ebassi) 2012-01-13 14:45:14 UTC
Review of attachment 178158 [details] [review]:

looks good to me; another review from a GIO developer would seal the deal.
Comment 20 Alexander Larsson 2012-12-19 12:55:37 UTC
This kinda completely misses the point of G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT.
Only the inotify backend supports (can support) sending events on close if there was a change. All the other backends are unable to support this.

For such backends, rather than handling things on every byte written you want to wait a while after the last file change and then handle the change, in order to not get a million change events that you need to handle.

Its *possible* that we should not send changes done hint from the inotify backend, but that means you will never handle changes to things like log files that are continuosly written but not closed.
Comment 21 Alexander Larsson 2012-12-19 12:57:08 UTC
Review of attachment 178158 [details] [review]:

This will break non-inotify backends.
Comment 22 Aleksander Morgado 2012-12-21 13:26:07 UTC
So, currently there is no way, when using inotify, to reliably know when the file got closed. This means that file indexers like Tracker will not know when e.g. a file is fully downloaded from the Internet before starting to index it, as the download may be slow and virtual emissions of CHANGES_DONE_HINT may happen before the real CHANGES_DONE_HINT due to close().

For the case of wanting to handle changes to things like log files, remember that you still have plain CHANGED events. From my POV, CHANGES_DONE_HINT is something that, if not supported by the backend, should really be handled by the user of the library, not by gio itself.

Anyway, my original approach to try to tackle the issue was to make the virtual emission of CHANGES_DONE_HINT configurable with a a new per-monitor g_file_monitor_set_changes_done_hint_delay(), which if passed '0' would completely disable that emission (see first comments in the bug). This would be useful when using inotify in order to completely avoid CHANGES_DONE_HINT emission while the file is still open. And in other backends, it would be useful if you want to handle the live-update logic yourself looking at CHANGED events. Would this approach be better?
Comment 23 Krzysztof Kotlenga 2013-03-18 15:03:55 UTC
+1 for comment #22.
I've just wasted a full working day just to find out that GFileMonitor is half useless.
Comment 24 Matthias Clasen 2015-08-21 03:21:13 UTC
I think this was obsoleted with the rewrite of the file monitoring infrastructure this cycle.
Comment 25 Aleksander Morgado 2015-08-26 10:10:32 UTC
(In reply to Matthias Clasen from comment #24)
> I think this was obsoleted with the rewrite of the file monitoring
> infrastructure this cycle.

I don't believe this has been obsoleted, or what am I missing?

I just retested with glib git master and still see no way to disable the virtual emission of changes done hint, which are still being emitted; e.g. open file, do one update every 10s without closing (with the perl script from comment 15):
1440583544: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///home/aleksander/out.txt'
1440583545: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///home/aleksander/out.txt'
1440583547: Received 'G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT' event in 'file:///home/aleksander/out.txt'
1440583554: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///home/aleksander/out.txt'
1440583556: Received 'G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT' event in 'file:///home/aleksander/out.txt'
1440583564: Received 'G_FILE_MONITOR_EVENT_CHANGED' event in 'file:///home/aleksander/out.txt'
1440583566: Received 'G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT' event in 'file:///home/aleksander/out.txt'
Comment 26 GNOME Infrastructure Team 2018-05-24 12:54:22 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/376.