Review Request 128762: qmljs: restructure duchain locking to prevent deadlocks

[Sven Brauch]

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



Actually ... I think the problem here is in clang, not JS. The JS locking code is inefficient, but not wrong. Calling BackgroundParser::isQueued() or so with the DUChainLock held should be allowed.

What really causes the deadlocks (https://bugs.kde.org/show_bug.cgi?id=367825, http://blog.svenbrauch.de/2016/06/16/kdevelop-5-0-standalone-executable-for-linux/#comment-529) is that Clang's parsejob constructor acquires a DUChain lock (in translationUnitForURL()). That must not happen, because createParseJob is called from BackgroundParser::parseDocuments(), and that holds the background parser mutex. So clang first acquires the background parser mutex, then the duchain lock; and JS first acquires the duchain lock and then the background parser mutex (by calling isQueued() with the duchain lock held) -> deadlock. The second lock ordering (used by JS and other languages) is far more sensible, because the background parser mutex is needed for much shorter times much less frequently.

I'll see that I can fix that in clang.

- Sven Brauch


On Sept. 1, 2016, 8:22 p.m., Sven Brauch wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128762/
> -----------------------------------------------------------
> 
> (Updated Sept. 1, 2016, 8:22 p.m.)
> 
> 
> Review request for KDevelop and Kevin Funk.
> 
> 
> Bugs: 367825
>     http://bugs.kde.org/show_bug.cgi?id=367825
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
> We _must not_ hold a duchain lock when contextOfFile() is called. That
> might try to add something to the background parser queue, which locks
> the background parser mutex; then both locks are held, which causes
> deadlocks.
> 
> 
> Diffs
> -----
> 
>   languages/qmljs/codecompletion/context.cpp 3a44f45 
>   languages/qmljs/codecompletion/tests/test_qmljscompletion.cpp b689418 
>   languages/qmljs/duchain/declarationbuilder.cpp fb0cb5d 
>   languages/qmljs/duchain/expressionvisitor.cpp 3b641f5 
>   languages/qmljs/duchain/frameworks/nodejs.cpp 41964e4 
>   languages/qmljs/duchain/helper.cpp e537103 
>   languages/qmljs/duchain/parsesession.cpp d1ad4a9 
> 
> Diff: https://git.reviewboard.kde.org/r/128762/diff/
> 
> 
> Testing
> -------
> 
> Opened some test project, no crashes, no freezes, and the (I think) reason for the freeze is gone.
> 
> 
> Thanks,
> 
> Sven Brauch
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160904/de0bd108/attachment.html>