Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5910 --- Ship it! Hmm, indeed. I had another version of these patches in mind. This one looks fine. - David On Aug. 11, 2011, 10:10 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:10 p.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5914 --- This review has been submitted with commit a4871052d09eecd928666d482427abebb5f39c56 by Albert Astals Cid to branch KDE/4.7. - Commit On Aug. 11, 2011, 10:10 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:10 p.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5915 --- This review has been submitted with commit d634bc01c50e0a312a0ff2f4ec4e67cfde344232 by Albert Astals Cid to branch frameworks. - Commit On Aug. 11, 2011, 10:10 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:10 p.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5875 --- I think this patch is almost ready, but Dawit's comment seems right about an improvement that should be done to it: - Move all the preemptive lookup logic from the thread's run function to HostInfo::lookupHost. [No need to create a thread when DNS is already cached]. - David On Aug. 11, 2011, 10:10 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:10 p.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
On Aug. 21, 2011, 10:29 a.m., David Faure wrote: I think this patch is almost ready, but Dawit's comment seems right about an improvement that should be done to it: - Move all the preemptive lookup logic from the thread's run function to HostInfo::lookupHost. [No need to create a thread when DNS is already cached]. I am a bit confused, as far as I can see, all the preemptive lookup logic is already in HostInfo::lookupHost, what more do you want me to move there? - Albert --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5875 --- On Aug. 11, 2011, 10:10 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:10 p.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
On Sunday 14 August 2011 23.05.41 Albert Astals Cid wrote: #2 Your patch has several issues i mentioned there I noticed those too, and I wanted to just say that I'd trust David and Thiago on these concepts any day. Maybe we can just use the structure they suggested (and Albert coded) and move on?
Re: Review Request: Do not terminate threads
On Aug. 12, 2011, 3:45 a.m., Dawit Alemayehu wrote: #1. Doesn't this approach have similar issues to the one that forced me to change the previous QtConcurrent::run based implementation ? That is, doesn't the use of a single thread expose this function to lookup requests backlog and hence cause noticable delays ? #2. Isn't all this complexity a bit too much for such a small functionality ? Wouldn't a much simpler approach work just as well ? For example, - Keep the current look up thread, but connect its finished signal to its parent deleteLater signal in its ctor. [Automatic cleanup of the lookup thread]. - Make the lookup thread cache the looked up information in the global cache instead of storing it locally. [No need to store this inform 2x]. - Move all the preemtive lookup logic from the thread's run function to HostInfo::lookupHost. [No need to create a thread when DNS is already cached]. - Create the lookup thread on the heap and let it run until it finishes. When it is done it will clean itself up. See a patch that implements the above approach at https://git.reviewboard.kde.org/r/102238/ #1 Each request itself is threaded and non blocking, so i don't see why multiple request should cause delays #2 Your patch has several issues i mentioned there - Albert --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5654 --- On Aug. 11, 2011, 10:10 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:10 p.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5623 --- Good job, this is tricky code indeed. Some comments below. kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5034 The interesting question is, what if hostname was already in the m_lookups map? Either we want to detect this upfront and not create a second request, or we need a list of requests for a given hostname in the map. kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5033 reviewboard is highlighting quite a number of lines with trailing whitespace kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5035 You could also just create the worker on the stack here. kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5036 Maybe move nameLookupThread as a member of hostInfoAgentPrivate, to avoid multiplying the global statics (and therefore have more control over order of destruction)? Or does this make no sense? kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5037 Ouch, a busy loop. The standard solution is to use ... yes ... a semaphore :-) I love semaphores. Acquire the semaphore here, release it in the thread once the worker object has been created. - David On Aug. 11, 2011, 9:23 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 9:23 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:32 a.m.) Review request for kdelibs and Dawit Alemayehu. Changes --- Addressed David concerns Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs (updated) - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
On Aug. 11, 2011, 9:41 a.m., David Faure wrote: kio/kio/hostinfo.cpp, line 236 http://git.reviewboard.kde.org/r/102179/diff/5/?file=31680#file31680line236 Maybe move nameLookupThread as a member of hostInfoAgentPrivate, to avoid multiplying the global statics (and therefore have more control over order of destruction)? Or does this make no sense? Could make that, but they seem somewhat separate things to me so i'd prefer to keep them separate unless you have a strong feeling about it On Aug. 11, 2011, 9:41 a.m., David Faure wrote: kio/kio/hostinfo.cpp, line 224 http://git.reviewboard.kde.org/r/102179/diff/5/?file=31680#file31680line224 You could also just create the worker on the stack here. Done On Aug. 11, 2011, 9:41 a.m., David Faure wrote: kio/kio/hostinfo.cpp, line 179 http://git.reviewboard.kde.org/r/102179/diff/5/?file=31680#file31680line179 reviewboard is highlighting quite a number of lines with trailing whitespace Fixed On Aug. 11, 2011, 9:41 a.m., David Faure wrote: kio/kio/hostinfo.cpp, line 266 http://git.reviewboard.kde.org/r/102179/diff/5/?file=31680#file31680line266 Ouch, a busy loop. The standard solution is to use ... yes ... a semaphore :-) I love semaphores. Acquire the semaphore here, release it in the thread once the worker object has been created. Changed. On Aug. 11, 2011, 9:41 a.m., David Faure wrote: kio/kio/hostinfo.cpp, line 177 http://git.reviewboard.kde.org/r/102179/diff/5/?file=31680#file31680line177 The interesting question is, what if hostname was already in the m_lookups map? Either we want to detect this upfront and not create a second request, or we need a list of requests for a given hostname in the map. Found out i can actually use the request id as key so we should not have this problem now. - Albert --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5623 --- On Aug. 11, 2011, 10:32 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:32 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:34 a.m.) Review request for kdelibs and Dawit Alemayehu. Changes --- Whitespace fixes Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs (updated) - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5636 --- kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5046 This class could be simplified to a simple struct. kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5047 This class isn't necessary. We can do the same with QThread alone. To quit the thread, connect the worker's destroyed() signal to the thread's quit() slot. Then just deleteLater the worker. kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5048 Remove the cache too. QHostInfo already caches. kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5049 Huh? What is the acquire/release doing here? - Thiago On Aug. 11, 2011, 10:34 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:34 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5653 --- kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5065 Ah, the wifi lost my comment here: the name Petition surprised me a lot, I couldn't see what this was about. I had to use an english dictionary to see that this was a synonym of Request, which is a much more common name for this. How about renaming to NameLookupRequest? kio/kio/hostinfo.cpp http://git.reviewboard.kde.org/r/102179/#comment5066 m_lookups.insert(lookupId, petition); is faster (as recommended by Effective C++ iirc). - David On Aug. 11, 2011, 10:34 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:34 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:10 p.m.) Review request for kdelibs and Dawit Alemayehu. Changes --- Rename Petition to Request Use QMap::insert instead of [] = Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs (updated) - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5654 --- #1. Doesn't this approach have similar issues to the one that forced me to change the previous QtConcurrent::run based implementation ? That is, doesn't the use of a single thread expose this function to lookup requests backlog and hence cause noticable delays ? #2. Isn't all this complexity a bit too much for such a small functionality ? Wouldn't a much simpler approach work just as well ? For example, - Keep the current look up thread, but connect its finished signal to its parent deleteLater signal in its ctor. [Automatic cleanup of the lookup thread]. - Make the lookup thread cache the looked up information in the global cache instead of storing it locally. [No need to store this inform 2x]. - Move all the preemtive lookup logic from the thread's run function to HostInfo::lookupHost. [No need to create a thread when DNS is already cached]. - Create the lookup thread on the heap and let it run until it finishes. When it is done it will clean itself up. See a patch that implements the above approach at https://git.reviewboard.kde.org/r/102238/ - Dawit On Aug. 11, 2011, 10:10 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 11, 2011, 10:10 p.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp 344b1d8 Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
On Dijous 04 Agost 2011 15:28:49 Dawit A wrote: On Thu, Aug 4, 2011 at 5:31 AM, Albert Astals Cid tsdg...@terra.es wrote: This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ On August 4th, 2011, 3:19 a.m., *Dawit Alemayehu* wrote: I do not like this patch for the very reason you stated. I do not want the mutex there either because it is rather expensive. As it stands we start a thread for each lookup right now which in of itself is already too expensive for my taste. Hence, I will have to think of some other way to avoid the even worse solution of creating a local even loop. Having said that, are you using a slow DNS server ? The only way the lookup thread gets terminated is if your nameserver takes longer than 1000 ms per query. That is because the two KUriFilterPlugins that use it set a timeout value of that duration. Otherwise, that code path should not be encountered at all! So you do not want a patch that fixes konqueror crashing 75% times i use it in exchange of having a small memory leak once in a blue moon? Awesome. Did I say I do not want the patch ?? That's what i understood when reading your comment, sorry if it is not what you meant. All I said was need to think of a better way to fix this patch! Clam down. Since you do not want a mutex and you do not want my current solution either I can not think of much more ways of fixing this. Maybe by doing what Thiago suggests and instead of starting a short lived thread each time start a long lived one, though we will still need the mutex if we want it to be 100% correct. If you are at the Berlin Desktop Summit maybe we can meet and try to find a solution together. You can usually find me surrounded by a bunch of other spaniards. Albert
Review Request: Do not terminate threads
On Sat, Aug 6, 2011 at 7:46 AM, Albert Astals Cid aa...@kde.org wrote: On Dijous 04 Agost 2011 15:28:49 Dawit A wrote: On Thu, Aug 4, 2011 at 5:31 AM, Albert Astals Cid tsdg...@terra.es wrote: This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ On August 4th, 2011, 3:19 a.m., *Dawit Alemayehu* wrote: I do not like this patch for the very reason you stated. I do not want the mutex there either because it is rather expensive. As it stands we start a thread for each lookup right now which in of itself is already too expensive for my taste. Hence, I will have to think of some other way to avoid the even worse solution of creating a local even loop. Having said that, are you using a slow DNS server ? The only way the lookup thread gets terminated is if your nameserver takes longer than 1000 ms per query. That is because the two KUriFilterPlugins that use it set a timeout value of that duration. Otherwise, that code path should not be encountered at all! So you do not want a patch that fixes konqueror crashing 75% times i use it in exchange of having a small memory leak once in a blue moon? Awesome. Did I say I do not want the patch ?? That's what i understood when reading your comment, sorry if it is not what you meant. Well, it is always misunderstandings that cause issues. I am sure I too would have misunderstood my response if I had bothered to re-read it. :) Sorry about that. It did not come out how I intended it to come out. All I said was need to think of a better way to fix this patch! Clam down. Since you do not want a mutex and you do not want my current solution either I can not think of much more ways of fixing this. Maybe by doing what Thiago suggests and instead of starting a short lived thread each time start a long lived one, though we will still need the mutex if we want it to be 100% correct. I think the event loop based solution is probably the way to go. It is what both the localdomain and fixhost uri filter plugins used to use before I refactored the code out and put in the parent class to reduce the DNS lookups and also avoid code duplication. Anyhow, I have posted a patch that uses the aforementioned event loop solution at https://git.reviewboard.kde.org/r/102238/. Can you please see if it resolves your issues ? If you are at the Berlin Desktop Summit maybe we can meet and try to find a solution together. You can usually find me surrounded by a bunch of other spaniards. Unfortunately, I won't be there. kio_hostinfo.patch Description: Binary data
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5371 --- QHostInfo is already threaded. This code is unnecessary today and should be removed. - Thiago On Aug. 2, 2011, 11:30 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 2, 2011, 11:30 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp fef39fc Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
On Aug. 4, 2011, 7:14 a.m., Thiago Macieira wrote: QHostInfo is already threaded. This code is unnecessary today and should be removed. If we remove the thread we need to introduce a nested eventloop since that method is public and supposed to be blocking. Is that OK? - Albert --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5371 --- On Aug. 2, 2011, 11:30 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 2, 2011, 11:30 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp fef39fc Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5390 --- Ok, then there's something wrong. QHostInfo has had a blocking method since Qt 4.0. - Thiago On Aug. 2, 2011, 11:30 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 2, 2011, 11:30 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp fef39fc Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
On Aug. 4, 2011, 1:11 p.m., Thiago Macieira wrote: Ok, then there's something wrong. QHostInfo has had a blocking method since Qt 4.0. Can not use it since we have a timeout parameter. Thus we either need the thread (to use the blocking method and forget about the thread when the timeout hits) or the eventloop (to wait for the non blocking method and stop looping when the timeout hits). - Albert --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5390 --- On Aug. 2, 2011, 11:30 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 2, 2011, 11:30 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp fef39fc Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5396 --- That makes sense. Then we need only one thread, one that runs the event loop so it can be notified of finished lookups. We definitely don't need one thread per lookup, since that thread will only sleep. - Thiago On Aug. 2, 2011, 11:30 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 2, 2011, 11:30 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp fef39fc Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
On Thu, Aug 4, 2011 at 5:31 AM, Albert Astals Cid tsdg...@terra.es wrote: This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ On August 4th, 2011, 3:19 a.m., *Dawit Alemayehu* wrote: I do not like this patch for the very reason you stated. I do not want the mutex there either because it is rather expensive. As it stands we start a thread for each lookup right now which in of itself is already too expensive for my taste. Hence, I will have to think of some other way to avoid the even worse solution of creating a local even loop. Having said that, are you using a slow DNS server ? The only way the lookup thread gets terminated is if your nameserver takes longer than 1000 ms per query. That is because the two KUriFilterPlugins that use it set a timeout value of that duration. Otherwise, that code path should not be encountered at all! So you do not want a patch that fixes konqueror crashing 75% times i use it in exchange of having a small memory leak once in a blue moon? Awesome. Did I say I do not want the patch ?? All I said was need to think of a better way to fix this patch! Clam down.
Re: Review Request: Do not terminate threads
On Thu, Aug 4, 2011 at 9:11 AM, Thiago Macieira thi...@kde.org wrote: This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ Ok, then there's something wrong. QHostInfo has had a blocking method since Qt 4.0. Thiago, you must have forgotten but you and I had this discussion before on IRC. :) There is nothing wrong with QHostInfo except its lookupHost function does not offer a timeout parameter we need for this use case. IOW if QHostInfo provided a lookup function that took a timeout parameter, then there would be no need for the code I added here at all.
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/#review5369 --- I do not like this patch for the very reason you stated. I do not want the mutex there either because it is rather expensive. As it stands we start a thread for each lookup right now which in of itself is already too expensive for my taste. Hence, I will have to think of some other way to avoid the even worse solution of creating a local even loop. Having said that, are you using a slow DNS server ? The only way the lookup thread gets terminated is if your nameserver takes longer than 1000 ms per query. That is because the two KUriFilterPlugins that use it set a timeout value of that duration. Otherwise, that code path should not be encountered at all! - Dawit On Aug. 2, 2011, 11:30 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 2, 2011, 11:30 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs - kio/kio/hostinfo.cpp fef39fc Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 2, 2011, 11:29 a.m.) Review request for kdelibs and Dawit Alemayehu. Changes --- Whitespace fixes Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs (updated) - kio/kio/hostinfo.cpp fef39fc Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
Re: Review Request: Do not terminate threads
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102179/ --- (Updated Aug. 2, 2011, 11:29 a.m.) Review request for kdelibs and Dawit Alemayehu. Summary --- Each time the terminate code triggers my Konqueror crashes, i'm substituting the terminate for just waiting the thread to finish and we just ignoring it. The code has a race condition in which wait() returns false, then we switch to the thread and m_autoDelete is still not set and thus noone will delete the thread. I can add a mutex if you guys think this is unacceptable. Diffs (updated) - kio/kio/hostinfo.cpp fef39fc Diff: http://git.reviewboard.kde.org/r/102179/diff Testing --- When the kDebug() Name look up for hostName failed; if triggers i do not get a crash anymore. Thanks, Albert
