Review Request 118458: Fix KDirWatch to act more reliably against various backends.

Wed Jun 4 17:04:21 UTC 2014

> On June 4, 2014, 3:38 a.m., David Faure wrote:
> > autotests/kdirwatch_unittest.cpp, line 523
> > <https://git.reviewboard.kde.org/r/118458/diff/1/?file=277318#file277318line523>
> >
> >     The commit log doesn't mention any changes related to QFileSystemWatcher, only to the FAM backend. Are you sure you wanted to commit this? If it works then of course I'm happy with it (but maybe in a separate commit then)

I'm not sure how that got fixed, I just seem to remember changing things and in the process of working on that test I discovered that QFSW worked just fine.  I don't remember if my changes did that, or if it was fixed sometime ago and I just discovered the fact.  Or if Qt5 fixed it?

I'll move it to a separate commit to be safe.

> On June 4, 2014, 3:38 a.m., David Faure wrote:
> > autotests/kdirwatch_unittest.cpp, line 677
> > <https://git.reviewboard.kde.org/r/118458/diff/1/?file=277318#file277318line677>
> >
> >     This could actually use QFile::link(), would be more readable.

So, here be dragons.  QFile::link fails the unit test.  But ::link doesn't.  I'm going to leave this alone for now.

> On June 4, 2014, 3:38 a.m., David Faure wrote:
> > src/lib/io/kdirwatch.cpp, line 636
> > <https://git.reviewboard.kde.org/r/118458/diff/1/?file=277319#file277319line636>
> >
> >     I'm not 100% sure about the meaning of this boolean variable. (we started an event?)
> >     Maybe it can be called something more explicit (waitForInitialFAMEvents? or something that makes sense to you ;))

I'm not sure what this should be.  I think startedFAMMonitor is more clear, as it says what the variable is tracking, as opposed to what happens when set.  It marks when a FAMMonitor* function is called, which matches.  If you prefer the waitForInitialFAMEvents, signalling what will happen, I'll it to that.

You did basically guess what the variable meant, but it really speaks more to the FAM Monitor piece, not an event.

> On June 4, 2014, 3:38 a.m., David Faure wrote:
> > src/lib/io/kdirwatch.cpp, line 1633
> > <https://git.reviewboard.kde.org/r/118458/diff/1/?file=277319#file277319line1633>
> >
> >     Technically this could be an event about a completely unrelated file to the one we're waiting to see, couldn't it?
> >

As far as I understand the code, no.  FAM reports which request the FAM event is associated with, which each KDirWatch event stores.  So this FAMEndExists (or similar) will be associated with a single KDW event, and thus only update the correct one.

> On June 4, 2014, 3:38 a.m., David Faure wrote:
> > src/lib/io/kdirwatch.cpp, line 1719
> > <https://git.reviewboard.kde.org/r/118458/diff/1/?file=277319#file277319line1719>
> >
> >     actual -> actually ?

Whoops, fixed.

> On June 4, 2014, 3:38 a.m., David Faure wrote:
> > autotests/kdirwatch_unittest.cpp, line 297
> > <https://git.reviewboard.kde.org/r/118458/diff/1/?file=277318#file277318line297>
> >
> >     It seems that the second call should pass "created(QString)" rather than "deleted(QString)".

Copy and paste errors are the worst ...

- Matthew

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118458/#review59123
-----------------------------------------------------------

On June 1, 2014, 5:03 p.m., Matthew Dawson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118458/
> -----------------------------------------------------------
> 
> (Updated June 1, 2014, 5:03 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 329802
>     https://bugs.kde.org/show_bug.cgi?id=329802
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> -------
> 
> Fix KDirWatch to act more reliably against various backends.
> 
>  - When KDirWatch used a FAM backend, it wouldn't actually wait for the
> necessary watches to be put in place before returning, allowing for a race
> where an application may think a watch is in place when it really isn't.
> This was easily seen using gamin and running the testDeleteAndRecreateFile
> test.  Fix by delaying useFAM until the watch is in place by waiting until
> the EndExist FAM event is received.  This adds ~100-200ms per watch with
> gamin.
>  - When a file is deleted and recreated, and scanEntry detects it, the dirty
> signal would be emitted.  Fix to emit a deleted + created signal, as expected.
>  - When a deleted signal was requested, other signals were dropped.  Due to
> the above point, this would stop the created signal from being emitted.  Now,
> all signals are emitted, even when a delete is detected.
>  - When watching recreated files, the created signal could get lost as there
> was a race between when the created signal would be generated and its signal
> spy would be created.  Fix by making sure the spy is created before the signal
> could be emitted.
>  - Make sure the created signal isn't emitted twice with both FAM and the
> polling timer.  This occurs as FAM doesn't fix up the fact the entry has been
> created, and the poller thus assumes it needs to notify the world.  Fix by
> having FAM not emit the event in this case.
> 
> BUG: 329802
> 
> Note that due to a bug in the FAM daemon (which doesn't exist in the gamin daemon), the testDeleteAndRecreateDir unit test fails as FAM doesn't detect the folder deletion.  This test previously passed due to the race condition outlined in bug 329802, which was fixed in this RR.  I've confirmed this is a bug in FAM using a minimal program talking directly to the daemon, and watching the strace output from FAM.  FAM doesn't detect when a folder it is watching directly is deleted.  It does detect when a folder inside a folder it is watching is deleted, however.
> I think this should go in as is, as the unit test failing exposes the actual problem, instead of hiding behind a race when it doesn't really work.  If running against FAM is important, I can see about tracking down the issue.
> 
> 
> Diffs
> -----
> 
>   autotests/kdirwatch_unittest.cpp 4e638e089012276dc8b17b9d960732a161cc3be1 
>   src/lib/io/kdirwatch.cpp cc6feec4d2b2598e50c853172bbc295a8c719bc2 
>   src/lib/io/kdirwatch_p.h cf63c41745091cfedcf1b27c9e4aa77d3db6b31a 
> 
> Diff: https://git.reviewboard.kde.org/r/118458/diff/
> 
> 
> Testing
> -------
> 
> Unit tests all pass now with gamin.  Using FAM, one test fails as described above, but I've confirmed the daemon itself is causing the issue.
> 
> 
> Thanks,
> 
> Matthew Dawson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140604/da7494ac/attachment-0001.html>