Fixing krunner's threading

[Aaron J. Seigo]

On February 22, 2010, Andreas Hartmetz wrote:
> Hi,
> 
> Now that many of the more prominent crashes in krunner have been fixed
> I think it's time to attack something more "theoretic": runners run in
> threads and often don't pay any attention to thread-safety of API they
> use. Two weeks ago I've added a warning to KIO::Scheduler() to warn
> about its use from non-main threads, but so far nobody paid attention.
> The offending runner here is the places runner.

given that this evidently is not going to appear on plasma-devel@ and that i 
would like to see it addressed .......

in the Places runner, we use KFilePlacesModel which uses KIO internally. we 
can't create the KFilePlacesModel outside of the thread because if we then 
access it in the thread, QObjects get created in that thread that are 
(internally in KFilePlacesModel) parented to the KFilePlacesModel and we get 
new errors about cross-thread-qobject-parenting. so we need to create the 
places mode in the thread of the runner.

the only alternative i can think of is putting the KFilePlacesModel in the 
main thread in the PlacesRunner and using queued signals connected to a slot 
to do the actual places matching (blocking) in the GUI thread. not at all 
desirable, tbh, but it's what i've done and committed to trunk.

what we -really- need, however, is a KIO that can at least schedule and create 
jobs safely from different threads. if a job itself has to stay within the 
same thread, that's fine afaics. but limiting KIO calls to just the main 
thread, especially as they are used in random places hidden in our frameworks, 
is really sub-optimal.


in KIO::Scheduler, i see this:

    if (QThread::currentThread() != QCoreApplication::instance()->thread()) {
        kWarning(7006) << "KIO is not thread-safe.";
    }

tbh, that isn't overly helpful :) my immediate question was: WHAT parts of KIO 
are not thread-safe? looking at the code in there, it seems that the offending 
bits in KIO::Scheduler::doJob are:

-> ProtoQueue *KIO::SchedulerPrivate::protoQ, which needs a write lock on the 
collection in the case of mt usage.

-> KProtocolManager::slaveProtocol, which is static, but which updates 
internal state on each call (so i assume there's a 1:1 relationship between 
KIO::Scheduler and KProtocolManager, looking at the code); this too would need 
a write lock

-> ProtoQueue::queueJob, also needs a write lock

since it is already checking to see if it is running outside the main thread, 
would it be possible to simply create a write lock conditionally in that case, 
and delete it on the way out? this would be (practically) no additional 
overhead in the single threaded case and allow for MT access to doJob.

i assume, however, that the rest of this code is similarly non-thread-safe and 
so just protecting doJob and other KIO::Scheduler calls in this way wouldn't 
be enough? or would it?

could we perhaps do a KIO::Scheduler per-thread? would that be enough?

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Qt Development Frameworks