Review Request: Make Sonnet::Speller::setLanguage threadsafe, fixing segfaults in krunner

[Simeon Bird]

> On Sept. 6, 2012, 12:29 p.m., David Faure wrote:
> > A number of comments on the implementation, but also a more general comment: does it even make sense to use a Speller in multiple threads, and to  change the language from one thread while another one is using the speller for spell-checking? It sounds like, even if a mutex/lock can prevent crashes, this is a weird thing to do anyway, since you have no idea at which point the spell-checking will switch to another language... could happen in the very middle of a sentence...
> > 
> > Maybe it would make more sense to "post" the language change operation to the spell-checking thread using the same mechanism as the one used to "post" spellchecking requests to it? (Disclaimer: I know nothing of the krunner architecture).
> 
> Simeon Bird wrote:
>     Thanks for the review. 
>     
>     I agree that this is a slightly weird thing to do, but, as you surmise, it was the only way I could think of to make the feature work properly. 
>     
>     As far as I understand it (from reading the documentation), the way krunner works is to call Plasma::AbstractRunner::match in a new thread for every runner every time input is entered. 
>     
>     So if I enter:
>     
>     "spell hello"
>     
>     I will be called with 's' 'sp' 'spe'...and so on until I get the whole match, without waiting for the previous threads to return. Thus match has to be thread-safe.
>     
>     The feature I'm trying to fix here is to be able to enter "spell french bonjour" and have it spell something. 
>     The runner is responsible for parsing the input and checking whether it should do anything else.
>     So we don't actually post spellchecking requests, we just post some input, and detect that we should spell-check, and change the language, by parsing strings. So I couldn't see a way to do this except to call Sonnet::Speller::setLanguage() in Plasma::AbstractRunner::match, 
>     which meant that for it to work I had to make match thread-safe. 
>     
>     Sorry. I'm open to suggestions if you have a better idea. 
>     
>     I've fixed your other comments in v2 of the diff, let me know if there is anything else.
> 
> Thomas Lübking wrote:
>     would it not make more sense to just kill the pending (and now worthless?) thread instead (no idea how expensive this spellchecking is to be threaded) to not waste cpu on a result that will be ignored anyway?
> 
> David Faure wrote:
>     Forcefully killing threads (QThread::terminate()) is a big no-no, it leaks memory, potentially worse: it can leave a mutex locked, so any further thread trying to lock it, will hang for ever.
>     
>     The only way to terminate a thread is to ask it nicely to terminate from within (i.e. to exit run()), which means it has to finish what it's currently doing, first, before it looks up for the next command or posted event which tells it to quit.
> 
> Thomas Lübking wrote:
>     That is generally true and i admit to not know how the plasma spell checker is implemented (or even "where" ;-), but would expect some sort of ro non-joinable threading, operating on shared data (as the only purpose seems to prevent blocking the main event loop - not running two checks at the same  time and merge their results)  ... or does QThread alone bring structures to forbid such approach (The API doc does not mention inevitable leaks for QThread itself on ::terminate())?
>     
>     (PS: I just start to wonder whether the speller threads are then tagged to catch 2,1 exits...)
> 
> Simeon Bird wrote:
>     I don't entirely follow what you mean in the last comment; what do you mean 'tagged to catch 2,1 exits'?
>     
>     The spellchecking runner is, I think, very cheap compared to some of the other runners (eg, the nepomuk search), all of which are called at once.
>     
>     But, be that as it may, the spell-check has no control over the threading model of krunner, which it just inherits from Plasma::AbstractRunner, and changing that model for everything seems a bit overkill for this, even if we understood fully how it works (I certainly don't; I just took the documentation at face value, which may or may not be a good idea).
>     
>     A reasonable alternative, if you don't like doing the commit in principle (the mutex might well slow down the spell-check), is to just remove the problematic feature (which has never worked anyway). If krunner only spell-checked words in the default dictionary, we wouldn't need to call setLanguage(), and everything would be fine, as it was before 2010.
> 
> Thomas Lübking wrote:
>     Thread execution is not predictable so in general it's possible that you start a thread (1), then start another thread (2) and thread 2 finishes before thread 1
>     Since i assume that in this case the last exiting thread will determine the sanity of the token, this would eg. mean that if you spellcheck "threa" and then "thread" but "threa" finishes last, the token "thread" would be determined misspelled.
>     
>     A common approach to handle this is to "tag" (basically with an increment) the threads/results so that if you get a result from thread "1" and already got a result from thread "2" before, you know that the result from thread "1" is already irrelevant and simply drop it instead of polluting your result.
>     
>     I have no particular opinion on this patch or the krunner spell checker in general (except assuming that threading it will likely be overhead anyway - esp. if one sequentially navigated through a tree structure - and given the type rate a normal human being can achieve ;-)

Ah, I see. This is getting somewhat educational for me :)


- Simeon


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


On Sept. 9, 2012, 10:06 p.m., Simeon Bird wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106242/
> -----------------------------------------------------------
> 
> (Updated Sept. 9, 2012, 10:06 p.m.)
> 
> 
> Review request for kdelibs and Plasma.
> 
> 
> Description
> -------
> 
> Krunner's spellcheck plugin has been pretty broken since bd291d21f096a714a171e7af3a534ba345ca5659 (about two years ago) because it called Sonnet::Speller::setLanguage every time the spellchecker was invoked, which was (very much) not thread-safe.
> 
> This patch makes Sonnet::Speller::setLanguage threadsafe by protecting all access to the internal dict pointer using QReadWriteLock. 
> 
> A related review request is 106244, which adds more fixes to the spellcheck feature.
> 
> 
> This addresses bugs 264779 and 303831.
>     http://bugs.kde.org/show_bug.cgi?id=264779
>     http://bugs.kde.org/show_bug.cgi?id=303831
> 
> 
> Diffs
> -----
> 
>   kdecore/sonnet/speller.cpp b19e74d 
> 
> Diff: http://git.reviewboard.kde.org/r/106242/diff/
> 
> 
> Testing
> -------
> 
> Compiled, installed, used for a week or so, spellchecked a bunch of things.
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20120911/ca6530c2/attachment.html>