Review Request 118739: Make KSharedConfig thread-safe

[Matthew Dawson]

> On June 17, 2014, 12:50 p.m., Matthew Dawson wrote:
> > src/core/ksharedconfig.cpp, line 52
> > <https://git.reviewboard.kde.org/r/118739/diff/1/?file=281111#file281111line52>
> >
> >     Why create a template function here, when the T must be GlobalSharedConfigList?  Why not just put the logic in globalSharedConfigList?
> 
> David Faure wrote:
>     Because I copied this template into 4 different files in KIO as well. It basically makes this bit of code reuseable elsewhere more easily
>     (but this isn't big enough to be worth being put into Qt, especially since there is a slight variation between the cases where we need to be able to call hasLocalData from the outside, like here, and the cases in KIO where this is not needed and therefore the QThreadStorage instance can be moved into the function).
>     
>     I see your point that when reading just that file, the template thing looks surprising.
>     grep KIO for perThreadGlobalStatic to see a pattern emerging, though...

Ok, that makes sense.  It seems to me it would good for Qt to have a getOrCreate function on QThreadStorage, as that is all the template function is doing anyways.  But Qt doesn't have such a function, so I think the template can just stay.


> On June 17, 2014, 12:50 p.m., Matthew Dawson wrote:
> > src/core/ksharedconfig.cpp, line 37
> > <https://git.reviewboard.kde.org/r/118739/diff/1/?file=281111#file281111line37>
> >
> >     Should the KSharedConfig not all be synced to disk before its thread is destroyed?  Reading QThreadStorage's documentation, the sync can probably just happen in the deconstructor, as long as the thread isn't the main thread (or maybe just that qApp is still valid).
> 
> David Faure wrote:
>     KConfig already syncs in the destructor. For secondary threads, it should therefore already happen automatically, and this code takes care of the main thread explicitly.
>     
>     So I'm not sure what you see as missing here.

I didn't think about the destructor.  On second thought, the only problematic case is if someone has a thread run with joining with the main thread before the application quits, but I'm not sure if that is something to worry about.  I think for now that case isn't worth the effort, and if it does become a problem a proper solution can be created then.


- Matthew


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


On June 14, 2014, 5:20 a.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118739/
> -----------------------------------------------------------
> 
> (Updated June 14, 2014, 5:20 a.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> -------
> 
> Make KSharedConfig thread-safe
> 
> ... by having a different list of shareable objects per thread.
> 
> 
> Diffs
> -----
> 
>   src/core/ksharedconfig.cpp f4b4c766fe19fac92a0651ecc55a89ec3b7636b0 
> 
> Diff: https://git.reviewboard.kde.org/r/118739/diff/
> 
> 
> Testing
> -------
> 
> helgrind kiocore-threadtest (not committed yet)
> 
> no races detected in KSharedConfig anymore.
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140619/3c1f2877/attachment.html>