Re: Review Request 127437: Fix many threading issues in KUrlCompletion

2016-03-24 Thread David Faure 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127437/
---

(Updated March 24, 2016, 8:18 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit 15fd9b17eb2f4d686891d2364e698e40260f9131 by David Faure 
to branch master.


Repository: kio


Description
---

* Major race with other threads due to using QDir::setCurrent()
* Race conditions on m_terminationRequested and m_matches
* Race with previous completion thread when its posted event arrives after 
cancelling
* Cancellation code spread out in many methods and never done fully correctly
* isRunning() was missing one of the two threads, making unit test fail in 
valgrind
* Fix the rarely-hit code path where the thread isn't finished after 200ms
   - the current search string was lost because finished() wasn't called
   - the matches were not used, in case of user-completion (AFAICS)
   - changing the search string while the thread was running could lead to the 
old search
 string still being used for completion
 (the misnamed finished() wasn't called, so KCompletion didn't get the new 
string)
  => added a variant of the unittest which doesn't wait for the thread initially

+ Simplify code using signal/slots rather than a custom event
+ Simplify code using enum rather than casting to/from int

Most of these bugs are from 2004 (e.g. commit ec83c408619e3) ;)

REVIEW: 127437


Diffs
-

  autotests/CMakeLists.txt 4dff0a24d31897a9641a70017bd87e33415cef14 
  autotests/kurlcompletiontest.cpp eef39ff17940fadcb9492e5e7092070c976310d4 
  src/widgets/CMakeLists.txt 87dac50b377f6ea4c3cd39e9afa37a4680aecf31 
  src/widgets/kurlcompletion.h 14fda22a0e08bcf2da30e19fed577c8bda64a4ca 
  src/widgets/kurlcompletion.cpp 1dc8f4898fb78d6f49e687462446007a10305f98 

Diff: https://git.reviewboard.kde.org/r/127437/diff/


Testing
---

Hit a crash in DirectoryListThread when playing with kopenwithtest.

kurlcompletiontest extended, kurlcompletion-nowait added, both pass, also in 
valgrind (different timings).


Thanks,

David Faure

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127437: Fix many threading issues in KUrlCompletion

2016-03-22 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127437/#review93863
---


Ship it!




Ship It!

- David Edmundson


On March 20, 2016, 2:46 p.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127437/
> ---
> 
> (Updated March 20, 2016, 2:46 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> * Major race with other threads due to using QDir::setCurrent()
> * Race conditions on m_terminationRequested and m_matches
> * Race with previous completion thread when its posted event arrives after 
> cancelling
> * Cancellation code spread out in many methods and never done fully correctly
> * isRunning() was missing one of the two threads, making unit test fail in 
> valgrind
> * Fix the rarely-hit code path where the thread isn't finished after 200ms
>- the current search string was lost because finished() wasn't called
>- the matches were not used, in case of user-completion (AFAICS)
>- changing the search string while the thread was running could lead to 
> the old search
>  string still being used for completion
>  (the misnamed finished() wasn't called, so KCompletion didn't get the 
> new string)
>   => added a variant of the unittest which doesn't wait for the thread 
> initially
> 
> + Simplify code using signal/slots rather than a custom event
> + Simplify code using enum rather than casting to/from int
> 
> Most of these bugs are from 2004 (e.g. commit ec83c408619e3) ;)
> 
> REVIEW: 127437
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 4dff0a24d31897a9641a70017bd87e33415cef14 
>   autotests/kurlcompletiontest.cpp eef39ff17940fadcb9492e5e7092070c976310d4 
>   src/widgets/CMakeLists.txt 87dac50b377f6ea4c3cd39e9afa37a4680aecf31 
>   src/widgets/kurlcompletion.h 14fda22a0e08bcf2da30e19fed577c8bda64a4ca 
>   src/widgets/kurlcompletion.cpp 1dc8f4898fb78d6f49e687462446007a10305f98 
> 
> Diff: https://git.reviewboard.kde.org/r/127437/diff/
> 
> 
> Testing
> ---
> 
> Hit a crash in DirectoryListThread when playing with kopenwithtest.
> 
> kurlcompletiontest extended, kurlcompletion-nowait added, both pass, also in 
> valgrind (different timings).
> 
> 
> Thanks,
> 
> David Faure
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127437: Fix many threading issues in KUrlCompletion

2016-03-22 Thread David Faure 


> On March 21, 2016, 6:18 p.m., David Edmundson wrote:
> > src/widgets/kurlcompletion.cpp, line 1445
> > 
> >
> > this looks wrong, shouldn't it be insertItems (or ::addMatches which 
> > does the same thing)
> > 
> > we'll get this method called twice, once for each thread that takes 
> > longer than the initial timeout, setItems will discard matches from the 
> > first thread.

Only *one* of the threads is running. We are *either* running user completion 
("~") or directory completion.
Note how the previous code was calling stop+clear, as well.

I could have kept clear+addMatches, it would make no difference, it's the same 
as setItems (AFAICS).

I just don't call stop() anymore, because now stop() is "asking the thread to 
finish (abort)", while this slot is called once the thread has finished (or 
just before it really finishes, but at least it's about to, it doesn't need to 
be requested to stop).

Maybe an "else" between the last two if()s in the slot would make it clearer?

Thanks for the review, I'm open for more suggestions :-)


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127437/#review93829
---


On March 20, 2016, 2:46 p.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127437/
> ---
> 
> (Updated March 20, 2016, 2:46 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> * Major race with other threads due to using QDir::setCurrent()
> * Race conditions on m_terminationRequested and m_matches
> * Race with previous completion thread when its posted event arrives after 
> cancelling
> * Cancellation code spread out in many methods and never done fully correctly
> * isRunning() was missing one of the two threads, making unit test fail in 
> valgrind
> * Fix the rarely-hit code path where the thread isn't finished after 200ms
>- the current search string was lost because finished() wasn't called
>- the matches were not used, in case of user-completion (AFAICS)
>- changing the search string while the thread was running could lead to 
> the old search
>  string still being used for completion
>  (the misnamed finished() wasn't called, so KCompletion didn't get the 
> new string)
>   => added a variant of the unittest which doesn't wait for the thread 
> initially
> 
> + Simplify code using signal/slots rather than a custom event
> + Simplify code using enum rather than casting to/from int
> 
> Most of these bugs are from 2004 (e.g. commit ec83c408619e3) ;)
> 
> REVIEW: 127437
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 4dff0a24d31897a9641a70017bd87e33415cef14 
>   autotests/kurlcompletiontest.cpp eef39ff17940fadcb9492e5e7092070c976310d4 
>   src/widgets/CMakeLists.txt 87dac50b377f6ea4c3cd39e9afa37a4680aecf31 
>   src/widgets/kurlcompletion.h 14fda22a0e08bcf2da30e19fed577c8bda64a4ca 
>   src/widgets/kurlcompletion.cpp 1dc8f4898fb78d6f49e687462446007a10305f98 
> 
> Diff: https://git.reviewboard.kde.org/r/127437/diff/
> 
> 
> Testing
> ---
> 
> Hit a crash in DirectoryListThread when playing with kopenwithtest.
> 
> kurlcompletiontest extended, kurlcompletion-nowait added, both pass, also in 
> valgrind (different timings).
> 
> 
> Thanks,
> 
> David Faure
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127437: Fix many threading issues in KUrlCompletion

2016-03-21 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127437/#review93829
---




src/widgets/kurlcompletion.cpp (line 1362)


this looks wrong, shouldn't it be insertItems (or ::addMatches which does 
the same thing)

we'll get this method called twice, once for each thread that takes longer 
than the initial timeout, setItems will discard matches from the first thread.


- David Edmundson


On March 20, 2016, 2:46 p.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127437/
> ---
> 
> (Updated March 20, 2016, 2:46 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> * Major race with other threads due to using QDir::setCurrent()
> * Race conditions on m_terminationRequested and m_matches
> * Race with previous completion thread when its posted event arrives after 
> cancelling
> * Cancellation code spread out in many methods and never done fully correctly
> * isRunning() was missing one of the two threads, making unit test fail in 
> valgrind
> * Fix the rarely-hit code path where the thread isn't finished after 200ms
>- the current search string was lost because finished() wasn't called
>- the matches were not used, in case of user-completion (AFAICS)
>- changing the search string while the thread was running could lead to 
> the old search
>  string still being used for completion
>  (the misnamed finished() wasn't called, so KCompletion didn't get the 
> new string)
>   => added a variant of the unittest which doesn't wait for the thread 
> initially
> 
> + Simplify code using signal/slots rather than a custom event
> + Simplify code using enum rather than casting to/from int
> 
> Most of these bugs are from 2004 (e.g. commit ec83c408619e3) ;)
> 
> REVIEW: 127437
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 4dff0a24d31897a9641a70017bd87e33415cef14 
>   autotests/kurlcompletiontest.cpp eef39ff17940fadcb9492e5e7092070c976310d4 
>   src/widgets/CMakeLists.txt 87dac50b377f6ea4c3cd39e9afa37a4680aecf31 
>   src/widgets/kurlcompletion.h 14fda22a0e08bcf2da30e19fed577c8bda64a4ca 
>   src/widgets/kurlcompletion.cpp 1dc8f4898fb78d6f49e687462446007a10305f98 
> 
> Diff: https://git.reviewboard.kde.org/r/127437/diff/
> 
> 
> Testing
> ---
> 
> Hit a crash in DirectoryListThread when playing with kopenwithtest.
> 
> kurlcompletiontest extended, kurlcompletion-nowait added, both pass, also in 
> valgrind (different timings).
> 
> 
> Thanks,
> 
> David Faure
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 127437: Fix many threading issues in KUrlCompletion

2016-03-20 Thread David Faure 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127437/
---

Review request for KDE Frameworks.


Repository: kio


Description
---

* Major race with other threads due to using QDir::setCurrent()
* Race conditions on m_terminationRequested and m_matches
* Race with previous completion thread when its posted event arrives after 
cancelling
* Cancellation code spread out in many methods and never done fully correctly
* isRunning() was missing one of the two threads, making unit test fail in 
valgrind
* Fix the rarely-hit code path where the thread isn't finished after 200ms
   - the current search string was lost because finished() wasn't called
   - the matches were not used, in case of user-completion (AFAICS)
   - changing the search string while the thread was running could lead to the 
old search
 string still being used for completion
 (the misnamed finished() wasn't called, so KCompletion didn't get the new 
string)
  => added a variant of the unittest which doesn't wait for the thread initially

+ Simplify code using signal/slots rather than a custom event
+ Simplify code using enum rather than casting to/from int

Most of these bugs are from 2004 (e.g. commit ec83c408619e3) ;)

REVIEW: 127436


Diffs
-

  autotests/CMakeLists.txt 4dff0a24d31897a9641a70017bd87e33415cef14 
  autotests/kurlcompletiontest.cpp eef39ff17940fadcb9492e5e7092070c976310d4 
  src/widgets/CMakeLists.txt 87dac50b377f6ea4c3cd39e9afa37a4680aecf31 
  src/widgets/kurlcompletion.h 14fda22a0e08bcf2da30e19fed577c8bda64a4ca 
  src/widgets/kurlcompletion.cpp 1dc8f4898fb78d6f49e687462446007a10305f98 

Diff: https://git.reviewboard.kde.org/r/127437/diff/


Testing
---

Hit a crash in DirectoryListThread when playing with kopenwithtest.

kurlcompletiontest extended, kurlcompletion-nowait added, both pass, also in 
valgrind (different timings).


Thanks,

David Faure

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel