[Nepomuk] Review Request: Misc Filewatch fixes

[David Faure]

> On Oct. 29, 2012, 11:18 a.m., David Faure wrote:
> > This assumes that QRegExp::exactMatch is re-entrant, i.e. that it doesn't update an internal cache on demand. I'm not so sure of that, the regexp parsing sounds like a very good candidate for on-demand parsing, unless you make extra sure that it's all done upfront.
> 
> Simeon Bird wrote:
>     The Qt documentation claims it is:
>     
>     http://qt-project.org/doc/qt-4.8/qregexp.html
>     "Note: All functions in this class are reentrant."
>     
>     (although it's true that I hadn't checked up to now, so that was still a good question)
> 
> David Faure wrote:
>     Never trust documentation :-)
>     
>     I had a look at the source code, and found indeed
>     1) on-demand initializations
>     2) no locking mechanism.
>     
>     So I wrote a test program, and guess what? It gives different output from one run to the next. It's memcheck-clean, but helgrind manages to spot the issue in some runs.
>     http://www.davidfaure.fr/2012/qregexp_race.cpp
>     http://www.davidfaure.fr/2012/qregexp-helgrind-log
>     The program is supposed to write out "true true true" but only does so 1 time out of 8 on average, in my tests. Nice, heh?
>     
>     => The documentation is wrong. I'll submit a docu fix to Qt...

Wait, the documentation is right, the class is reentrant (i.e. different instances can be used in different threads).

It's just not threadsafe (i.e. you can't use the same instance of QRegExp in different threads).
Therefore the documentation isn't wrong, but this patch is.
You must use one QRegExp per thread, or ensure exclusive use of the QRegExp instance.

A QThreadStorage would make it possible to use one QRegExp per thread, but is rather difficult to handle if the regexp could change at runtime. So I think the only solution is "exclusive use of the QRegExp instance", i.e. QWriteLocker rather than QReadLocker (new conclusion: careful with what appears to be readonly).


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107082/#review21082
-----------------------------------------------------------


On Oct. 27, 2012, 6:29 p.m., Simeon Bird wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107082/
> -----------------------------------------------------------
> 
> (Updated Oct. 27, 2012, 6:29 p.m.)
> 
> 
> Review request for Nepomuk, Vishesh Handa and Sebastian Trueg.
> 
> 
> Description
> -------
> 
> Some fairly trivial misc improvements to the filewatch service. Probably don't make a big difference, but probably a good idea. 
> 
> - Use QReadWriteLock instead of QMutex in FileIndexerConfig, thus allowing multiple threads to call shouldFolderBeIndexed at once (not that we really do that right now).
> 
> - Add the IN_EXCL_UNLINK inotify flag. On recent (2.6.36) kernels, this means we don't generate events for files once
> they have been unlinked from the directory we are watching. Prevents waking up for some already-deleted temporary files.
> 
> 
> Diffs
> -----
> 
>   services/fileindexer/fileindexerconfig.h 7debaf3 
>   services/fileindexer/fileindexerconfig.cpp 5226a79 
>   services/filewatch/kinotify.h 6e3f1c0 
>   services/filewatch/kinotify.cpp 9868b90 
> 
> Diff: http://git.reviewboard.kde.org/r/107082/diff/
> 
> 
> Testing
> -------
> 
> Compiled, ran for a bit, didn't seem to break anything. 
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20121029/6521d6aa/attachment-0001.html>