Review Request 128829: [1/2] clang parse job: do initialization in run(), not in the constructor

[Sven Brauch]

> On Sept. 5, 2016, 1:55 p.m., Milian Wolff wrote:
> > -1
> > 
> > I agree that there is an issue we need to fix, see: https://bugs.kde.org/show_bug.cgi?id=355100
> > 
> > but I disagree with the way you tackle the issue. Imo, we should try to find a way to create the parse job without holding the background parser mutex somehow. I.e. instead of fixing N parse job implementations, only fix the background parser - somehow. I'd like to investigate this tomorrow or so. Do you have any time the following days to discuss this? Or do you think it's impossible?
> 
> Sven Brauch wrote:
>     The clang parse job is the only one which does this, all others have relatively trivial constructors and do their intialization in run(). I'm also not sure why the DefinesAndIncludes stuff requires being foreground -- why doesn't it have its own mutex? Then this would all be simpler.
>     
>     If you disagree with this solution, let's discuss what we do instead -- just ping me on IRC some evening, at least after 10pm or so I'm usually there.
> 
> Milian Wolff wrote:
>     The reason I do it in the foreground is that it accesses the project model, which is not threadsafe. adding a mutex to the DAIM won't help, and making the project model threadsafe is a no-go.
>     
>     the real problem imo is holding the background parser lock at all here, see the bug I referenced where we workarounded the issue in another section of our code

Ah, I see.

Hm, but the background parser holding the background parser mutex when creating a new job kind of makes sense. If you don't, you have to worry about things like not creating two jobs for one parse request, etc ... no?


- Sven


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


On Sept. 4, 2016, 3:37 p.m., Sven Brauch wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128829/
> -----------------------------------------------------------
> 
> (Updated Sept. 4, 2016, 3:37 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
> The constructor of the parse job is called from createParseJob(), which
> is called from the background parser with the background parser mutex
> locked. If the constructor acquires a DUChain lock (which it did), then
> any thread which acquires the DUChain lock first and then the background
> parser mutex will cause a deadlock. Other language plugins do this a lot,
> esp. qmljs and to some extent also python, by calling
> BackgroundParser::isQueued() with the DUChain lock held.
> 
> By doing the initialization in run() with the ForegroundLock held,
> we avoid this problem.
> 
> This is a severe bug, I hit it quite often and I have read more than one
> report from users complaining about freezing as well.
> 
> Requires https://git.reviewboard.kde.org/r/128830/ or it will just assert out.
> 
> 
> Diffs
> -----
> 
>   languages/clang/clangparsejob.h 7c0ab7a 
>   languages/clang/clangparsejob.cpp d676aea 
>   languages/plugins/custom-definesandincludes/compilerprovider/settingsmanager.cpp 590ba4a 
>   languages/plugins/custom-definesandincludes/definesandincludesmanager.cpp df1be68 
> 
> Diff: https://git.reviewboard.kde.org/r/128829/diff/
> 
> 
> Testing
> -------
> 
> Opened several large sessions in a debug build, didn't see any more freezes during initial parsing. Before this was really easy to observe if you opened a C++ and a JS project in parallel.
> 
> 
> Thanks,
> 
> Sven Brauch
> 
>

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