[KDE/Mac] Review Request 121134: make KGlobal reference counting threadsafe

[Milian Wolff]

> On Nov. 16, 2014, 2:58 p.m., Milian Wolff wrote:
> > as I said before: Did you check whether that is actually the case? Did you add some assertions to see what other thread is calling this code?
> > 
> > I'm not sure whether this is supposed to be threadsafe or not. If it must be threadsafe, you'll also need to make s_allowQuit threadsafe (QAtomicBool).
> 
> René J.V. Bertin wrote:
>     I have indeed looked at this aspect, but not exhaustively and haven't found evidence of cross-thread reference counting. Yet.
>     It's unclear what you mean with "that" (the thing actually the case). It "that" refers to the quitting issue, I can of course never prove that it is due to a reference counting race condition because the issue isn't reproducible reliably. I can only invalidate the idea that it is due to such a race condition, whenever it happens again with properly protected reference counting.
>     
>     I think we have to leave it up to the kdelibs maintainers/authors to decide whether or not this has to be thread safe. The doxygen documentation for KJob does not mention anything about thread safety. It does however note that *KJob and its subclasses is meant to be used in a fire-and-forget way. It's deleting itself when it has finished using deleteLater() so the job instance disappears after the next event loop run.* . It is not said that a KJob cannot use a KJob itself, which could lead to concurrent reference (de)counting, nor whether a KJob instance is guaranteed to be deleted in the same thread it was created, and I think that a *concurrent processing* class that does not explicitly mention it does thread unsafe things can be assumed to be thread safe.
>     
>     There's no QAtomicBool, btw.

As I told you before in other patches you send in: Please make sure you find the actual reason before changing other code in the hope of it improving things. If there is no hard evidence that your bug manifests itself because of a race in KGlobal::ref/deref, then don't change it. Adding an assertion to the code that its only accessed from the mainthread is done easily and should be hit reliably (contrary to a race condition or lock contention that Thomas proposes).

And also note that a KJob is not neccessarily implemented using threads. You can also use e.g. QIODevice or external processes internally to get the desired behavior.


- Milian


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


On Nov. 16, 2014, 3:53 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121134/
> -----------------------------------------------------------
> 
> (Updated Nov. 16, 2014, 3:53 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and kdelibs.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> I have been experiencing unexpected exits from KDevelop that were not due to any kind of error in the KDevelop code; it was as if someone told the application to exit normally. This happens mostly on OS X, but also sometimes on Linux.
> I finally traced this to `KGlobal::deref` reaching a zero reference count and invoking `QCoreApplication::quit` when called from one of KDevelop's KJob dtors. There does not appear to be a reference counting mismatch in the code, so the issue might be due to a race condition in KGlobal::ref/deref.
> 
> This patch introduces thread-safety to KGlobal's reference counting by turning the simple global `static int s_refCount` into a `static QAtomicInt s_refCount`. I consider this an important bug fix regardless of whether it corrects the issue I have with KDevelop.
> 
> 
> Diffs
> -----
> 
>   kdecore/kernel/kglobal.cpp cf003a4 
> 
> Diff: https://git.reviewboard.kde.org/r/121134/diff/
> 
> 
> Testing
> -------
> 
> On OS X 10.6.8 only for now.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20141116/94b8d7f3/attachment.html>