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

[Andrew Gvozdev <angvoz.dev@xxxxxxxxx>]

Hi Christian,

On Wed, May 21, 2014 at 5:16 AM, Christian Walther <walther@xxxxxxxx> wrote:

I am having a problem with a race condition between the GCCBuiltinSpecsDetector and a job of my own plugin: I am trying to modify the project description by calling

  ICProjectDescription writableProject = CoreModel.getDefault().getProjectDescription(m_project);
  // .. modify writableProject ...
  CoreModel.getDefault().setProjectDescription(m_project, writableProject, false, monitor);

but find that my modifications are overwritten soon after by GCCBuiltinSpecsDetector writing back what appears to be an older project description.

This happens in org.eclipse.cdt.internal.core.settings.model.CProjectDescriptionManager.updateProjectDescriptions(IProject[], IProgressMonitor), called indirectly from org.eclipse.cdt.managedbuilder.language.settings.providers.AbstractBuiltinSpecsDetector.execute() via AbstractBuiltinSpecsDetector.runForEachLanguage() ... LanguageSettingsSerializableProvider.serializeLanguageSettings() ... LanguageSettingsProvidersSerializer.notifyLanguageSettingsChangeListeners() ... CModelManager.handleEvent(ILanguageSettingsChangeEvent).

CProjectDescriptionManager.updateProjectDescriptions() as per the logic in AbstractBuiltinSpecsDetector.execute() runs with the scheduling rule of the project and gets the project description (A), then schedules its second part to write it back (B) with the scheduling rule of the workspace root (because project does not contain workspace root). Between the execution of A and B, no scheduling rules are held and others are free to call getProjectDescription() and setProjectDescription() themselves, only to have their modifications overwritten by B.

Before I file a bugzilla I'd like to ask here: Is this a bug in CDT or is there anything I can do in my code to prevent this? I couldn't think of anything so far. It seems like a bug to me, updateProjectDescriptions() shouldn't let its two parts be separated like this, getProjectDescription() and setProjectDescription() should be executed as an atomic unit while holding the scheduling rule of at least the affected project (in fact, it must be at least the workspace root because that's what setProjectDescription() uses internally).

As far as I understand it was separated because it was causing deadlocks but I don't know enough details.

 

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 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.

Thanks,

Andrew

Hope someone can shed some light on this.

 -Christian

_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/cdt-dev