Review Request 118739: Make KSharedConfig thread-safe

[David Faure]

> On June 17, 2014, 4: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?

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...


> On June 17, 2014, 4:50 p.m., Matthew Dawson wrote:
> > src/core/ksharedconfig.cpp, line 72
> > <https://git.reviewboard.kde.org/r/118739/diff/1/?file=281111#file281111line72>
> >
> >     Can you also please document the new behaviour of openConfig, that each instance is thread specific?

Very good point. Fixed.


> On June 17, 2014, 4: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).

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.


- David


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


On June 14, 2014, 9: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, 9: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/20140618/f79a2d36/attachment.html>