GNOME Bugzilla – Bug 635765
Remove the virtual emission of G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT
Last modified: 2018-05-24 12:54:22 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.
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.
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.
(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.
Created attachment 175246 [details] [review] Fix some indentation/alignment issues This patch fixes the indentation/alignment issues in the gfilemonitor.c file
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
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.
Created attachment 175250 [details] [review] Patch for the new method, doc updated Thanks for the comments Christian. New patch attached with the documentation updated.
Is there any usecase to try out the patch ?
Created attachment 176084 [details] [review] [PATCH 1/2] gio: Fix some minor indentation/alignment issues
Created attachment 176085 [details] [review] [PATCH 2/2] gio: New method, g_file_monitor_set_changes_done_hint_delay() Patches rebased with git master.
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.
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?
(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.
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?
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.
ping, any resolution on patch ?
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?)
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... ;)
Review of attachment 178158 [details] [review]: looks good to me; another review from a GIO developer would seal the deal.
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.
Review of attachment 178158 [details] [review]: This will break non-inotify backends.
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?
+1 for comment #22. I've just wasted a full working day just to find out that GFileMonitor is half useless.
I think this was obsoleted with the rewrite of the file monitoring infrastructure this cycle.
(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'
-- 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.