Review Request: Replace thread usage with local event loop in kio/kio/hostinfo.cpp

[Dawit A]

On Mon, Aug 8, 2011 at 3:56 PM, Thomas Zander <zander at kde.org> wrote:
> On Monday 08 August 2011 21.28.45 Dawit A wrote:
>> On Mon, Aug 8, 2011 at 3:20 PM, Thomas Zander <zander at kde.org> wrote:
>> > On Monday 08 August 2011 21.02.02 Dawit A wrote:
>> >> On Mon, Aug 8, 2011 at 2:31 PM, Thomas Zander <zander at kde.org> wrote:
>> >> > On Monday 08 August 2011 18.35.13 Dawit A wrote:
>> >> >> #2. The original functions in this class were non-blocking. It is
>> >> >> only the new function I added that is a blocking call. And that is
>> >> >> required because of the need for a timeout when doing name lookups
>> >> >> from the urifilter plugins. Thos plugins perform a job that is time
>> >> >> critical and as such cannot be impeded by name lookups that will
>> >> >> take too long and hence the need for a timeout. If QHostInfo offered
>> >> >> such interface, then there will be no need for the newly added
>> >> >> functions.
>> >> >
>> >> > Maybe naive; but would it not be simpler to wrap the Qt method like
>> >> > this?
>> >> >
>> >> >  const int id = QHostInfo::lookupHost ( hostname, receiver,  slot);
>> >> >  QTimer::singleshot(timeout, QHostInfo::abortHostLookup (id );
>> >>
>> >> eh ? The version of QHostInfo::lookupHost you used above is non
>> >> blocking so the entire function becomes a non-blocking function which
>> >> is not what we want.
>> >
>> > In Qt (networking) stuff is non-blocking, which carries over to KDELibs
>> > using non-blocking calls.
>> > So by my reading we *do* want it to be non-blocking.
>> > The main reason I see to use blocking calls  is because they are easier
>> > to use as a programmer as they allow you to avoid having an extra slot
>> > to handle the callback.
>> > Am I missing a reason for this case?
>>
>> Yes. The uri filter plugins that are the primary users of this new
>> function require a synchronous function call or they would all have to
>> implement this syncing part individually for themselves.
>
> Apologies for still not getting it..
> You stated you wanted to have a timeout to avoid a blocking UI, which as far
> as I understand you would also avoid if you don't create a new method that
> never blocks in the first place.

The uri filter plugins, which are primarly used to filter user input,
e.g. user typing into a konqueror's address, are time critical for the
obvious reasons. The architecture for these plugins relies on a direct
synchrounous call. See KUriFilterPlugin in kio/kio/kurifilter.h.
Perhaps looking at the parent of the plugin classes will help clarify
the problem for you. KUriFilter loads all allowed uri filter plugins
and filters the user input by invoking KUriFilterPlugin::filterUri.

> My confusion lies in the idea that the new lookupHost method  introduces
> blocking which we both agree will be bad for UI responsiveness.
> Using non-blocking callbacks using the main threads eventloop sounds like its
> the way to go, then.
> It doesn't have to be very complicated for those plugins to do; some of those
> uri filter plugins can use the trick of a QTimer if thats what they want.

They used to do that. I decided to do refactoring in order to get rid
of code duplication and hence created a new protected function any uri
filter plugin can call to do a name lookup, see
KUriFilterPlugin::resolveName in kio/kio/kurifilter.{h|cpp}.

> There are other solutions too, I guess I can google for other usages of this
> Qt class on the net and see what others did.

I am all ears. Any solution better than creating all these threads or
using a local even loop is welcome though I doubt there is one. To me
the correct fix would be for QHostInfo to change lookupHost to accept
a timeout parameter which by default is -1 so that it can block
forever. That would resolve everything without the need for all of
this and without changing the current behavior of the current look up
function.

> Maybe that solution is less complicated then creating an extra thread?

Actually it is two threads that get created per each request because
QHostInfo is also multi-threaded according to Thiago. :( Anyhow,
outside of using a local even loop as suggested by this patch in this
review request or changing QHostInfo to accept a timeout parameter, I
do not know any other optimal solution.