Review Request: [PATCH] Make KIO::Scheduler correctly re-use ioslaves on hold...

[Dawit Alemayehu]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/6271/
-----------------------------------------------------------

(Updated 2011-01-04 21:46:47.110697)


Review request for kdelibs.


Changes
-------

Actually the original patch and my analysis is incorrect! The m_checkOnHold flag is intentionally set to true only the the first try because that is how handing an ioslave over to a new application supposed to work. IOW, you only want to check with the launcher when the KDE application and hence a new instance of the scheduler is initiated for the first time. The cause of bugs like the ones listed above an incorrect if statement order in KIO::SchedulerPrivate::heldSlaveFor. Unfortunately this this logic flaw seems to have been present since the implementation of this mechanism and was never discovered because it is very diffcult to spot or even encounter the bug. Certain conditions had to be met. Anyhow, the gist of the bug is order of the following if statements:

        if (canJobReuse) {
            if (job->url() == m_urlOnHold) {
               // use slave...
            } else {
                // kill slave...
            }
            ...
            ...
        }


which should actually have been written as

        if (job->url() == m_urlOnHold) {
            if (canJobReuse) {
               // use slave...
            } else {
                // kill slave...
            }
            ...
            ...
        }

Spot the difference ? The condition that checks whether or not the slave-on-hold should be reused (canJobReuse) should come after it is determined that the current request and the slave-on-hold are retrieving the same resource! Otherwise, any request unrelated to the slave-on-hold will cause the scheduler to kill the slave that is on hold! And that is exactly why it is so difficult to duplicate this problem. Unless the scheduler processes another request before the slave-on-hold can be reused, the problem will never be encountered! The best place to see this bug is attempting to download email attachments in chatty websites like gmail which makes XMLHttpRequests in the background. There this bug becomes very apparent.

The previous patch has been amended to fix this flaw instead of unnecessarily messing around with the m_checkOnHold flag...


Summary (updated)
-------

Actually the original patch and my analysis is incorrect! The m_checkOnHold flag is intentionally set to true only the the first try because that is how handing an ioslave over to a new application supposed to work. IOW, you only want to check with the launcher when the KDE application and hence a new instance of the scheduler is initiated... 

The bug that caused issues like the ones listed above is a logic flaw, an incorrect if statement order to be precise, in KIO::SchedulerPrivate::heldSlaveFor. I checked the source code of KDE 3.5.x branch and the bug was also present there and as such must have been there from the beginning. The gist of the problem is order of the following if statements:

        if (canJobReuse) {     
            if (job->url() == m_urlOnHold) {
               // use slave...
            } else {
                // kill slave...
            }
        }


which should actually have been written as
        if (job->url() == m_urlOnHold) {
            if (canJobReuse) {
               // use slave...
            } else {
                // kill slave...
            }
        }

Spot the problem ? The ioslave should never be killed just because a new request gets processed before the slave-on-hold can be reused! And yet that is what the current code actually perpetuates that. It also explains why the problem seems random. So long as the scheduler does not process another request before the slave-on-hold is reused, the problem will never show up. Anyways, the patch has been amended to fix this flaw...


This addresses bugs 123121 and 148307.
    https://bugs.kde.org/show_bug.cgi?id=123121
    https://bugs.kde.org/show_bug.cgi?id=148307


Diffs (updated)
-----

  /trunk/KDE/kdelibs/kio/kio/scheduler.cpp 1211439 

Diff: http://svn.reviewboard.kde.org/r/6271/diff


Testing
-------


Thanks,

Dawit

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20110104/32bdcc27/attachment.htm>