Review Request: Fix for a couple of KIO put-slave-on-hold bugs

[Andreas Hartmetz]

On Thursday 28 April 2011 16:05:51 Dawit Alemayehu wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101244/
> -----------------------------------------------------------
> 
> (Updated April 28, 2011, 2:05 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Changes
> -------
> 
> Removed the m_ignoreOnHoldListChanged flag per discussion with David.
> 
> 
> Summary
> -------
> 
> The attached patch address a couple of bugs in the KIO put-slave-on-hold
> functionality:
> 
> Problem:
> ======
> #1. If a user clicks a link on a page, e.g.
> ftp://ftp.kde.org/pub/kde/README_UPLOAD, then KRun will first issue a get
> (CMD_GET) to determine its content-type and put the ioslave on hold so
> that the application associated with the returned content-type can handle
> it. In case of our example that would be kate/kwrite. Unfortunately, the
> ioslave put on hold will not be reused because almost all applications
> will use KIO::file_copy (CMD_COPY) to make a local copy before opening it.
> Even then for ioslaves that do not optimize their copy command, the call
> to KIO::file_copy will properly reuse the ioslave on hold so long as it do
> not have an optimized copying method like the ftp ioslave.
> 
> #2. If a user types a web address, e.g. www.kde.org, into KRunner to open
> it in Konqueror and repeats the same process with another address, then
> whether the ioslave put on hold the second time around will get reused or
> not depends how the application works. If the application allows multiple
> documents or tabs and opens the second url as in the already running
> instance, then the ioslave on hold will NOT be reused the second time
> around.
> 
> Solution:
> ======
> #1. Simply force the KIO::file_copy call not to do the optimized copying if
> there is a slave on hold for the request. This will force it to use two
> separate jobs, KIO::get (CMD_GET) and KIO::put (CMD_PUT) ensuring the
> reuse of the ioslave that was put on hold.
> 
> #2. Whenever a call to KIO::Scheduler::publishSlaveOnHold is made, send a
> dbus message to update all the other scheduler so that they can look for
> an ioslave-on-hold to service the next request.
> 
> 
> Diffs (updated)
> -----
> 
>   kio/kio/job.cpp e34f562
>   kio/kio/scheduler.h 9be9db0
>   kio/kio/scheduler.cpp 4cb33d1
> 
> Diff: http://git.reviewboard.kde.org/r/101244/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit

The previous slave-on-hold patches trigger a -probably- previously existing 
job accounting bug in the scheduler that was hidden before due to the mostly 
non-working state of the slave-on-hold mechanism. AFAICS this bug was present 
before my scheduler rewrite where I tried not to change anything I didn't 
completely understand.
After enabling SCHEDULER_DEBUG I get the following debug output from 
konqueror:

konqueror(5203)/kio (Scheduler) KIO::ProtoQueue::startAJob: 
m_runningJobsCount: 3

while some jobs stall and websites don't load. I can trigger this by running 
e.g. konqueror http://paste.kde.org/39121/ (unrelated paste).
The bug is probably somewhere in KIO::SchedulerPrivate::putSlaveOnHold(),
publishSlaveOnHold(), or heldSlaveForJob(). The job's slave is never marked as 
idle or not idle in any of these methods and it probably should be, somewhere.

Please take care of this before you continue. I know it's not really "your" 
bug, but it's probably not really "mine" either.
Contact me if you'd prefer me to have a look. I am trying not to spend much 
time on coding right now so I can spend more time on university stuff.

Cheers,
Andreas