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

[Dawit A]

On Thu, Apr 28, 2011 at 6:44 PM, Andreas Hartmetz <ahartmetz at gmail.com> wrote:
> 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.

If you are talking about my previous patch, then I have already
reverted that. As I have already stated it was incorrect since I did
not really understand what it was doing even though I thought I did.
When I realized my patch was wrong, I reverted everything except the
the flipped if statement in heldSlaveForJob. The was the only fix that
was needed in the first place.

> 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

Did you do the test before or after I reverted the patch ?

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

Well, since my original patch was reverted, if you never got the above
message before my changes then you should not get them now either ;
due to the revert of course. Anyhow, the process of putting the
ioslave on hold first kills the job and most importantly emits a
slaveDied signal ; so I do not understand why the job accounting stuff
would be affected any differently than the standard normal way a slave
finishes its job and ceases to exist.

Regards,
Dawit A.