Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.

[Matthew Dawson]

> On Jan. 27, 2015, 3:55 p.m., Matthew Dawson wrote:
> > Unforunately, this cause test system failures in the the kconfigskeletontest test suite.  I'm not sure why this should create issues there.
> > 
> > However, I have a partial solution that avoids creating a full KSharedConfig.  Instead, only globalData needs to be called in KConfig to ensure the object is created as soon as possible (without needing a QCoreApplication).  This should avoid having any other globals created before.
> > 
> > However, this causes a crash later.  The root of the issue is in the KConfigPrivate constructor, line 98.  Creating the QLocale after QCoreApplication is gone is apparently broken as well.  I'm not sure how to solve that, maybe cache the value we need from QLocale, similar to caching the arguments?
> > 
> > Also, I think the copied over KConfig test case really needs to match the KSharedConfig test, as using KSharedConfig can mask the problem since KSharedConfig's pointer is cached, avoiding the KConfig constructor.
> 
> David Faure wrote:
>     Oh. Indeed. I can explain the test failure: that test was removing the config file and then creating a KConfigSkeleton around it (which was supposed to be the instanciation of the KSharedConfig, starting from "no file"). With my change, the KSharedConfig already exists before that, we delete the file underneath, but it keeps existing, so the KConfigSkeleton also uses the data from that now-deleted file. It's more or less like refcounting - we kept an old object alive by creating it upfront.
>     
>     You're right, we could just store the args instead, for a more minimal change.
>     But I tried it and I hit the same QLocale issue as you did. Argh.
>     We're really doing too much in that global object dtor. Wrapping up is ok, creating new stuff is not.
>     
>     I tried working around the issue by keeping a KSharedConfig::Ptr data member in the Tester class - which sounds like an extremely good idea for apps to do, in order to reuse it rather than recreate and reparse at shutdown time. This takes care of that issue, but another one then shows up:
>     ~Tester -> ~KSharedConfig -> KConfig::sync -> QLockFile::lock -> "QApplication::qAppName: Please instantiate the QApplication object first".
>     We didn't hit that in kdelibs4 either because I wrote QLockFile for Qt5, and KLockFile didn't call qAppName, it used a refcounting KComponentData.
>     I guess I could add an if (qApp) in QLockFile, but we're really going into fragile territories. We're definitely not just fixing a regression anymore since kdelibs4 asserted with this testcase.
>     
>     The KSharedConfig testcase doesn't avoid the KConfig constructor, the way it's written. The caching is refcounting, it only actually caches if there's something storing a Ptr somewhere (hence my suggestion to actually do that, because apps should really do that, especially since they don't have KComponentData doing that for them anymore). I'm fine with any change you want to see in the KConfig testcase though, but let's sort out the main testcase first.
>     
>     I'm currently thinking Revision 2 of this patch was the best, it fixed the regression compared to kdelibs4 without introducing new issues.

I'm thinking using KConfig when a QCoreApplication is not present should just not be supported.  At the very least, the new issue in the save path means you can't actually save anything when the global destructors are running, which makes using KConfig then pointless.  If we can get QLockFile to work correctly in this case, we could revisit this.

For now, I think the best plan is going back to revision 2 (as you suggested) + a comment on KConfig/KSharedConfig warning about using them without a valid QCoreApplication is not supported.  Does that sound good to you?


- Matthew


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


On Jan. 27, 2015, 3:10 a.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122232/
> -----------------------------------------------------------
> 
> (Updated Jan. 27, 2015, 3:10 a.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> -------
> 
> kconfig_in_global_object.cpp comes from kdelibs4support
> (after porting to Q_GLOBAL_STATIC)
> 
> ksharedconfig_in_global_object.cpp is new (but works with kdelibs4)
> and reproduces Albert's KgDifficulty testcase.
> 
> 
> Diffs
> -----
> 
>   autotests/kconfig_in_global_object.cpp PRE-CREATION 
>   autotests/ksharedconfig_in_global_object.cpp PRE-CREATION 
>   src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a 
>   src/core/ksharedconfig.cpp e059b87a1cc1df50693a668ef791e7a61050ef88 
>   autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace 
> 
> Diff: https://git.reviewboard.kde.org/r/122232/diff/
> 
> 
> Testing
> -------
> 
> Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone.
> 
> 
> Thanks,
> 
> David Faure
> 
>

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