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

[Sven Brauch]

> On Sept. 3, 2016, 9:07 p.m., Milian Wolff wrote:
> > languages/qmljs/codecompletion/tests/test_qmljscompletion.cpp, line 125
> > <https://git.reviewboard.kde.org/r/128762/diff/2/?file=475864#file475864line125>
> >
> >     did you make sure that the code in here does the locking itself?

It must. The lock is not held by the caller of completionItems() in the "real world" either.


> On Sept. 3, 2016, 9:07 p.m., Milian Wolff wrote:
> > languages/qmljs/duchain/declarationbuilder.cpp, line 202
> > <https://git.reviewboard.kde.org/r/128762/diff/2/?file=475865#file475865line202>
> >
> >     further down in this patch you relock before closing the context. also above you do the same. so why is this not required here?

It's apparently never required, closeContext() acquires a write lock itself.


> On Sept. 3, 2016, 9:07 p.m., Milian Wolff wrote:
> > languages/qmljs/duchain/declarationbuilder.cpp, line 347
> > <https://git.reviewboard.kde.org/r/128762/diff/2/?file=475865#file475865line347>
> >
> >     it's not safe to access the contents of argumentDecls without holding the lock.

Hmm. True in this case. Further below it checks that funcDecl->topContext() == topContext() though. I moved that check further up; if that is true, then it's safe to access the declarations even after releasing the lock (since they are in the topContext we currently hold the parse lock for, so they cannot be unloaded).


> On Sept. 3, 2016, 9:07 p.m., Milian Wolff wrote:
> > languages/qmljs/duchain/declarationbuilder.cpp, line 365
> > <https://git.reviewboard.kde.org/r/128762/diff/2/?file=475865#file475865line365>
> >
> >     this is now unsafe

moved that out into its own extra loop, that fixes your complaint of repeatedly re-locking as well.


> On Sept. 3, 2016, 9:07 p.m., Milian Wolff wrote:
> > languages/qmljs/duchain/declarationbuilder.cpp, line 438
> > <https://git.reviewboard.kde.org/r/128762/diff/2/?file=475865#file475865line438>
> >
> >     unsafe now

Hmm. If you are right about this (and I'm afraid you are) then this is by far not the only place where it's done incorrectly. And also this whole patch is not really a viable approach at fixing the problem it tries to fix.
I need to think about it ... will update this patch as soon as I know what to do.


- Sven


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


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/20160903/d468cf38/attachment-0001.html>