DevHeads.net

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

Review request for kdelibs.

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. <a href="ftp://ftp.kde.org/pub/kde/README_UPLOAD" title="ftp://ftp.kde.org/pub/kde/README_UPLOAD">ftp://ftp.kde.org/pub/kde/README_UPLOAD</a>, 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. <a href="http://www.kde.org" title="www.kde.org">www.kde.org</a>, 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
kio/kio/scheduler.h 9be9db0
kio/kio/scheduler.cpp 4cb33d1
kio/kio/job.cpp e34f562

Diff: <a href="http://git.reviewboard.kde.org/r/101244/diff" title="http://git.reviewboard.kde.org/r/101244/diff">http://git.reviewboard.kde.org/r/101244/diff</a>

Testing

Thanks,

Dawit

Comments

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

By Dawit A at 04/28/2011 - 09:05

(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. <a href="ftp://ftp.kde.org/pub/kde/README_UPLOAD" title="ftp://ftp.kde.org/pub/kde/README_UPLOAD">ftp://ftp.kde.org/pub/kde/README_UPLOAD</a>, 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. <a href="http://www.kde.org" title="www.kde.org">www.kde.org</a>, 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: <a href="http://git.reviewboard.kde.org/r/101244/diff" title="http://git.reviewboard.kde.org/r/101244/diff">http://git.reviewboard.kde.org/r/101244/diff</a>

Testing

Thanks,

Dawit

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

By Dawit A at 04/29/2011 - 16:20

(Updated April 29, 2011, 9:20 p.m.)

Review request for kdelibs.

Changes
Though technically unrelated, fixed the probable cause of the job accouting problem aharmtez mentioned.

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. <a href="ftp://ftp.kde.org/pub/kde/README_UPLOAD" title="ftp://ftp.kde.org/pub/kde/README_UPLOAD">ftp://ftp.kde.org/pub/kde/README_UPLOAD</a>, 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. <a href="http://www.kde.org" title="www.kde.org">www.kde.org</a>, 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: <a href="http://git.reviewboard.kde.org/r/101244/diff" title="http://git.reviewboard.kde.org/r/101244/diff">http://git.reviewboard.kde.org/r/101244/diff</a>

Testing

Thanks,

Dawit

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

By Commit Hook at 04/29/2011 - 19:54

This review has been submitted with commit f4637373f70a7a30d5cf191ef38b3f296a48d87f by Dawit Alemayehu.

- Commit

On April 29, 2011, 9:20 p.m., Dawit Alemayehu wrote:

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

By David Faure at 04/29/2011 - 17:51

Ship it!

Looks good to me, apart from two small things.

kio/kio/scheduler.cpp
<http://git.reviewboard.kde.org/r/101244/#comment2544>

Any risk pq would be null here?

kio/kio/scheduler.cpp
<http://git.reviewboard.kde.org/r/101244/#comment2545>

This if(), on the hand, surprises me. Surely q can't ever be null? I see lots of other methods in the Private class don't test for q being null.

- David

On April 29, 2011, 9:20 p.m., Dawit Alemayehu wrote:

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

By Dawit A at 04/29/2011 - 19:35

Possibly, but it is inconsistently applied through out the code. In some places it is checked for null, in others it is not. But it does not hurt if there is a check so I will put one there.

Indeed. Removed.

- Dawit

On April 29, 2011, 9:20 p.m., Dawit Alemayehu wrote:

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

By Andreas Hartmetz at 04/28/2011 - 17:44

On Thursday 28 April 2011 16:05:51 Dawit Alemayehu wrote:
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 <a href="http://paste.kde.org/39121/" title="http://paste.kde.org/39121/">http://paste.kde.org/39121/</a> (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

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

By Dawit A at 04/28/2011 - 23:39

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

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

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.

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

By David Faure at 04/28/2011 - 03:42

kio/kio/scheduler.cpp
<http://git.reviewboard.kde.org/r/101244/#comment2513>

Glad to see my idea worked ;)
Just a question, does it really matter that the emitter of the signal would set m_checkOnHold to true (if you remove m_ignoreOnHoldListChanged)? After all there is a slave on hold now, and maybe in some cases we end up opening this url in this process after all?

I'm not sure the m_ignoreOnHoldListChanged bool is necessary.

- David

On April 27, 2011, 10:57 p.m., Dawit Alemayehu wrote:

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

By Dawit A at 04/28/2011 - 08:39

Indeed, your idea definitely works and is much easier to implement. :) Regarding the flag, I added it simply because I was copying what the other dbus signal, reparseSlaveConfiguration, was doing without really thinking about it. Now that you have raised that point, though I do not see how the current process would end up reusing the ioslave that it caused to be put on hold, it does not really hurt to let its check on hold flag get set to true in this case either ; so I will change that.

- Dawit

On April 27, 2011, 10:57 p.m., Dawit Alemayehu wrote: