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

[Albert Astals Cid]

> On Aug. 8, 2011, 1:53 p.m., David Faure wrote:
> >
> 
> David Faure wrote:
>     (Sorry, flaky wifi lost the comment)
>     
>     I am very much against a nested event loop (QEventLoop::exec), it's a well-known fact nowadays that it creates unexpected re-entrancy and crashes.
>     
>     And since I just fixed the crash (missing wait() after terminate(), see commit log), I don't think we need this change. However reusing threads might be a good idea (separate issue).
> 
> Albert Astals Cid wrote:
>     What you did seems like breaking the idea of the timeout parameter altogether since you are basically waiting (and blocking the UI) until my slow DNS answers instead of only waiting for the time the timeout specifies.
> 
> Dawit Alemayehu wrote:
>     I have the same concerns. See http://lists.kde.org/?l=kde-commits&m=131281315828663&w=2. Under most circumstances, I too am against using local event loops because they are simply evil for reasons David mentioned above. However, I rather make the exception and go that route in this case to avoid constant UI blocking for those with slow DNS.
> 
> David Faure wrote:
>     No no, I call wait after *terminate* ! Terminate is "almost instant termination", it kills the thread.
>     So this does not wait for the DNS lookup to be finished, it only waits for terminate to call the cleanup callback, which should be almost instant (it's just that it's in another thread).
>     
>     I admit that I didn't fully test the case of a very slow DNS, but quick testing on the flaky network here seemed ok, and until someone proves the contrary, I firmly believe that this does not block for the whole duration of the dns lookup.
>     If it did, it would mean that terminate would basically do nothing ;)
>     
>     And no, we definitely want no nested event loop. Anything else, but not that.

I just tried a test calling that function with 0 as timeout (so it always timeouts) and sometimes it waits until 1 second so it is quite clear it is waiting for the thread.


- Albert


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102238/#review5499
-----------------------------------------------------------


On Aug. 7, 2011, 4:07 a.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102238/
> -----------------------------------------------------------
> 
> (Updated Aug. 7, 2011, 4:07 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This patch replaces the creation of a new thread for every DNS lookup with a local even loop based solution. This change addresses the issue of crashes that can arise from terminating the thread when the desired timeout duration is exceeded. See http://git.reviewboard.kde.org/r/102179/.
> 
> 
> Diffs
> -----
> 
>   kio/kio/hostinfo.cpp fcb7889 
> 
> Diff: http://git.reviewboard.kde.org/r/102238/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit
> 
>

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