Review Request 125123: [WIP] CMake plugin: prevent the QFileSystemWatcher from hitting the open file limit

[René J.V. Bertin]

> On Sept. 10, 2015, 4 p.m., Milian Wolff wrote:
> > where does it crash? we get the warnings even on linux but it never crashes. are there parts of this patch you want us to include or not?

It crashes in the dynamic_cast from `f` to `folder`, which is why I create a fully new folder instance when there was an error in adding the path. I don't know why it crashes nor exactly on what, but for now it hasn't crashed with this approach.
For now I'm just putting this up for reference. I'm testing the patch on Linux too, and haven't even yet been able to get warnings. I was expecting the kqueue backend to be used on Linux too, but apparently another one is used there, which uses less file descriptors (or none at all?).


> On Sept. 10, 2015, 4 p.m., Milian Wolff wrote:
> > projectmanagers/cmake/cmakecommitchangesjob.cpp, line 227
> > <https://git.reviewboard.kde.org/r/125123/diff/2/?file=402393#file402393line227>
> >
> >     wth? just use the assert above. if you hit that in a release build then you'll hit it in a debug build - and that should _not_ happen. thus assertion is fine no need to add this for release only

I didn't add these for nothing: the assert macros evaluate to noops in the release builds. The application is likely to crash in that case, of course, but in this case I find it better to abort it with an explicit message rather than crash in a random later location.


> On Sept. 10, 2015, 4 p.m., Milian Wolff wrote:
> > projectmanagers/cmake/cmakecommitchangesjob.cpp, line 233
> > <https://git.reviewboard.kde.org/r/125123/diff/2/?file=402393#file402393line233>
> >
> >     in Qt 5 you can simply use the return value of this function. Why do you keep working on Qt 4 based KDevplatform on Mac - I really don't get it... You are making your life unneccessarily complicated.

It's really not hard to get once you remember that KF5 doesn't work on OS X. Parts of it build in CI, but that's about it. I've more or less given up hope that this situation will approve anytime soon.


- René J.V.


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


On Sept. 9, 2015, 10:18 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125123/
> -----------------------------------------------------------
> 
> (Updated Sept. 9, 2015, 10:18 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDevelop.
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
> This is a confirmed bug in the 4.7 branch (and possibly in the KF5 version) where the cmake project importer can add (many) more paths to QFileSystemWatcher objects than the maximum supported number of open files (https://bugs.kde.org/show_bug.cgi?id=341551). On OS X at least this leads almost inevitably to a crash.
> 
> This patch prevents this by keeping track of the number of added paths, and by not adding more than a preconfigured maximum (determined empirically in this draft implementation). 
> The crash occurs in `folder = dynamic_cast<CMakeFolderItem*>(f);` in `CMakeCommitChangesJob::makeChanges()`; as an additional protection that function watches the `errno` variable for errors occurring during the `addPath()` operation; if an error is signalled, `folder` is not obtained by dynamic casting but by initialision a new `CMakeFolderItem`.
> `makeChanges()` also contains a few ASSERT statements that should remain effective even when building with `QT_NO_DEBUG`; the patch takes care of that as well.
> 
> I'm presenting this mostly for reference, in hope it may be useful for Kdevelop/KF5 too. In a real implementation the maximum number of patch watchers would of course be configurable.
> 
> 
> Diffs
> -----
> 
>   projectmanagers/cmake/cmakecommitchangesjob.cpp 7ce24fb 
>   projectmanagers/cmake/cmakemanager.h 19fc0c1 
>   projectmanagers/cmake/cmakemanager.cpp 2caecdb 
> 
> Diff: https://git.reviewboard.kde.org/r/125123/diff/
> 
> 
> Testing
> -------
> 
> On OS X 10.9.5 with kdelibs 4.14.11 and Qt 4.8.7 .
> m_MaxAllowedWatchedPaths is set to a value that is reasonably close to the limit where errors start occurring, and large enough to allow loading for instance kdepimlibs, kdepim and kdepim-runtime in a single session.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20150910/a094f0cc/attachment-0001.html>