Re: [cdt-dev] Race condition in CProjectDescriptionManager.updateProject

[Christian Walther <walther@xxxxxxxx>]

Thanks Doug and Andrew for your responses.

My current state of research is the following:

- When I move getting project descriptions into the scheduled part B of updateProjectDescriptions(), I get the same infinite loop of GCCBuiltinSpecsDetector executions as already described.

- I found out what causes the infinite loop: It is that GCCBuiltinSpecsDetectors are cloned faster than they can execute. As part of every setProjectDescription() operation (or possibly earlier if the description is modified, I haven't checked exactly), the project description including all its contained GCCBuiltinSpecsDetectors is cloned. If the GCCBuiltinSpecsDetector (a) hasn't gotten around to executing yet, the clone (b) inherits the isExecuted = false flag, so now both the original (a) and the clone (b) are set to execute some time in the future. When the original (a) finally executes (even though the project description it belongs to is no longer the current one and only waiting to be garbage-collected), it calls updateProjectDescriptions() at the end, causing another round of cloning, where (b), which is now the one belonging to the current project description and hasn't executed yet, is cloned into another one (c) that still needs to execute, and so on ad infinitum. It gets worse when there is more than one configuration, because every updateProjectDescriptions() call from *one* executing GCCBuiltinSpecsDetector causes the unexecuted GCCBuiltinSpecsDetectors of *all* configurations to be cloned - exponential explosion.

The reason why the infinite loop does not occur with the current (arguably flawed) implementation of updateProjectDescriptions() is presumably that because the project description that it sets is an older one, its GCCBuiltinSpecsDetectors have had a chance to execute by then and further cloning will create ones with isExecuted = true, so that their execution job will exit early and not call updateProjectDescriptions() to perpetuate the loop.

I seem to be able to master the issue by additionally having the execution job of AbstractBuiltinSpecsDetector.execute() check if its project description is still the current one, and exiting if not. But I still need to do some more thorough testing. I will also try to find a simple reproducing example that doesn't involve as much of our own custom code as my current situation, and file a bug when I arrive at something useful.

Andrew Gvozdev wrote:
>> As an experiment I tried modifying AbstractBuiltinSpecsDetector.execute() to always run its job with the workspace root scheduling rule, so that updateProjectDescriptions() wouldn't schedule its part B but run it synchronously, however that just resulted in an infinite stream of GCCBuiltinSpecsDetector executions, because every call of setProjectDescription() causes another execution via XmlProjectDescriptionStorage.setCurrentDescription() ... LanguageSettingsProvidersSerializer.reRegisterListeners() ... AbstractBuiltinSpecsDetector.registerListener(). (Why is it doing that anyway? This has annoyed me in the past already. GCCBuiltinSpecsDetector seems to run unnecessarily every (?) time you make some small change to the project using get/setProjectDescription().)
> It is not known in advance what kind of changes done to the project. AbstractBuiltinSpecsDetector reads new configuration description to determine if it needs to be rerun. If not, it does not. At least that is the intention. Function validateEnvironment() is responsible for that. If you have a case where it runs unnecessarily and willing to investigate I can help although I will have limited time to work on that. If you find a way to fix it I can take a look at your patch.

I'll give it a closer look, thanks. The actual condition is "if (validateEnvironment() && isExecuted) return;". Based on my current understanding, I now suspect that the instances that annoyed me were ones where isExecuted == false was causing the supposedly unnecessary execution, and had come about as described above.

>> I am not sure what a proper solution would be. Maybe updateProjectDescriptions() should put both the getting and the setting of project descriptions into its scheduled part (B)? I don't even completely understand yet what the purpose of updateProjectDescriptions() is - documentation says it recalculates "cached data", but I'm not familiar enough with CDT internals to know what "cached data" is.
> Those are dark parts of CDT where knowledge is lost... Maybe updateProjectDescriptions() should check if the project description is still up to date before setting? If not just forget about it, this old description is gone already.

That's a good idea, I'll give it a try.

 -Christian