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