Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 349445 - [xtext][builder] AbstractBuilderState does not load the persisted contents in all cases
Summary: [xtext][builder] AbstractBuilderState does not load the persisted contents in...
Status: REOPENED
Alias: None
Product: TMF
Classification: Modeling
Component: Xtext (show other bugs)
Version: 2.0.0   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 major (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-15 10:11 EDT by Sebastian Zarnekow CLA
Modified: 2011-11-09 14:47 EST (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sebastian Zarnekow CLA 2011-06-15 10:11:48 EDT
Access to the resourceDescriptionData has to be guarded by ensureLoaded() in all cases.
Comment 1 Sebastian Zarnekow CLA 2011-06-15 11:25:57 EDT
Pushed to master.
Comment 2 Mark Christiaens CLA 2011-06-16 05:08:15 EDT
Just wanted to report that I applied your fix (injected the patched ClusteringBuilderState) and it does seem to resolve the issue during startup. 

Apparently though, that's only half of the story.  I have pretty much the same issue during cleaning of a project:
- I have an editor open that is properly linked
- I do a project clean
- The editor gets a lot of error markers (I guess that's ok; while doing a clean build I don't expect cross references to be resolved)
- Once the clean build is done, the error markers remain and I can't follow the cross references (F3).  
- When I close and open that editor, things are fine again. 
- I also have a second editor that's open but not visible (another tab).  Once the cleaning is done, that editor seems to be fine.  Very strange. 

I disabled the IResourceScopeCache just to make sure that I wasn't looking at a caching issue but that didn't help.  

Currently, I kind of suspect that after the clean build, a refresh of the editor's state is omitted but I'm not familiar with that part of the code. 

I'll get back when I know more.
Comment 3 Sebastian Zarnekow CLA 2011-06-16 05:15:16 EDT
Hi Mark,

thanks for the follow up. A build will trigger a IResourceDescription.Event that contains a list of deltas. This event finally ends up in

DirtyStateEditorSupport#isReparseRequired(XtextResource, Event)

where 

IResourceDescription.Manager#isAffected(Delta, IResourceDescription)

is used to decide whether the editor has to be re-validated or not. Could you please verify that these methods are actually called in your case? Did you do any customizing in the implementation of the IResourceDescription.Manager?
Comment 4 Mark Christiaens CLA 2011-06-16 05:50:01 EDT
I'm using the DefaultResourceDescriptionManager. 

When I request a clean build, the isReparseRequired method is invoked twice for the resource corresponding to the open editor.  In both cases, it returns false.  

Note BTW that the editor window itself already has lost its semantic highlighting and already contains annotations of broken cross reference when entering the isReparseRequired method for the first time.  So there must be another trigger to refreshing the editor.
Comment 5 Mark Christiaens CLA 2011-06-16 09:45:59 EDT
I suspect that this is the problem:  When I request a clean build, a DirtyStateEditorStateJob is created and runs.  That job will eventually reparse the content of my editor.  While that job is running, the builder starts and iterates over all resources to perform the clean build.  It too will reparse the content of my editor (well, I guess the content of the underlying file of the editor).  Unless, those two happen in complete isolation and the update to the ResourceSet occurs atomically, I think this is a race.  

To confirm that this is happening I have the following to stack traces that were simultaneously visible in my debugger:

Thread [Worker-39] (Suspended (breakpoint at line 27 in AbstractParser))	
	VhdlParser(AbstractParser).parse(Reader) line: 27	
	LazyLinkingResource(XtextResource).doLoad(InputStream, Map<?,?>) line: 151	
	LazyLinkingResource.doLoad(InputStream, Map<?,?>) line: 69	
	LazyLinkingResource(XtextResource).reparse(String) line: 173	
	DirtyStateEditorSupport$UpdateEditorStateJob$2.process(XtextResource) line: 143	
	DirtyStateEditorSupport$UpdateEditorStateJob$2.process(Object) line: 1	
	DirtyStateEditorSupport$UpdateEditorStateJob$2(IUnitOfWork$Void<T>).exec(T) line: 36	
	XtextDocument$XtextDocumentLocker(AbstractReadWriteAcces<P>).modify(IUnitOfWork<T,P>) line: 49	
	XtextDocument$XtextDocumentLocker.modify(IUnitOfWork<T,XtextResource>) line: 189	
	XtextDocument.internalModify(IUnitOfWork<T,XtextResource>) line: 98	
	DirtyStateEditorSupport$UpdateEditorStateJob.run(IProgressMonitor) line: 131	
	Worker.run() line: 54	

The LazyLinkingResource is org.eclipse.xtext.linking.lazy.LazyLinkingResource@d21532ea uri='platform:/resource/demo/top2.vhd'

Thread [Worker-46] (Suspended (breakpoint at line 27 in AbstractParser))	
	owns: VhdlClusteringBuilderState  (id=1870)	
	VhdlParser(AbstractParser).parse(Reader) line: 27	
	LazyLinkingResource(XtextResource).doLoad(InputStream, Map<?,?>) line: 151	
	LazyLinkingResource.doLoad(InputStream, Map<?,?>) line: 69	
	LazyLinkingResource(ResourceImpl).load(InputStream, Map<?,?>) line: 1494	
	LazyLinkingResource(ResourceImpl).load(Map<?,?>) line: 1282	
	XtextResourceSet(ResourceSetImpl).demandLoad(Resource) line: 255	
	XtextResourceSet(ResourceSetImpl).demandLoadHelper(Resource) line: 270	
	XtextResourceSet(ResourceSetImpl).getResource(URI, boolean) line: 397	
	VhdlClusteringBuilderState(ClusteringBuilderState).writeNewResourceDescriptions(BuildData, IResourceDescriptions, CurrentDescriptions, IProgressMonitor) line: 238	
	VhdlClusteringBuilderState(ClusteringBuilderState).doUpdate(BuildData, ResourceDescriptionsData, IProgressMonitor) line: 93	
	VhdlClusteringBuilderState(AbstractBuilderState).update(BuildData, IProgressMonitor) line: 107	
	XtextBuilder.doBuild(ToBeBuilt, IProgressMonitor, IXtextBuilderParticipant$BuildType) line: 158	
	XtextBuilder.fullBuild(IProgressMonitor, boolean) line: 182	
	XtextBuilder.build(int, Map, IProgressMonitor) line: 85	
	BuildManager$2.run() line: 629	
	SafeRunner.run(ISafeRunnable) line: 42	
	BuildManager.basicBuild(int, IncrementalProjectBuilder, Map, MultiStatus, IProgressMonitor) line: 172	
	BuildManager.basicBuild(IProject, int, ICommand[], MultiStatus, IProgressMonitor) line: 203	
	BuildManager$1.run() line: 255	
	SafeRunner.run(ISafeRunnable) line: 42	
	BuildManager.basicBuild(IProject, int, MultiStatus, IProgressMonitor) line: 258	
	BuildManager.basicBuildLoop(IProject[], IProject[], int, MultiStatus, IProgressMonitor) line: 311	
	BuildManager.build(int, IProgressMonitor) line: 343	
	AutoBuildJob.doBuild(IProgressMonitor) line: 144	
	AutoBuildJob.run(IProgressMonitor) line: 242	
	Worker.run() line: 54	

The LazyLinkingResource is org.eclipse.xtext.linking.lazy.LazyLinkingResource@dc84320f uri='platform:/resource/demo/top2.vhd'

I must say that it's unclear to me why the UpdateEditorStateJob is actually created.  It's triggered because org.eclipse.xtext.ui.editor.DirtyStateEditorSupport.UpdateEditorStateJob.run(IProgressMonitor).affectedResources is not empty.  Don't know why because I'm working with a completely clean project.

Whatever the reason for this, the UpdateEditorStateJob and the Builder probably need more synchronization.
Comment 6 Sebastian Zarnekow CLA 2011-06-20 19:20:58 EDT
Hi Mark,

I guess you are right. The scheduling rule that is used for the update editor job may not be sufficient. We'll investigate further for SR1.

Regards,
Sebastian
Comment 7 Mark Christiaens CLA 2011-06-21 05:36:44 EDT
We've noticed that something similar seems to be happening when a project is first created.  Our new project wizard creates a project with a single file that's immediately opened.  That file is syntactically colored but not semantically.  Since, the project starts building immediately, I suspect that a similar race is present between the builder and updating the editor.  I'll dig a bit deeper to see whether I can find a workaround since this is quite a critical problem for us.
Comment 8 Sebastian Zarnekow CLA 2011-06-21 07:21:49 EDT
Hi Mark,

to verify your assumption, you could override DirtyStateEditorSupport.createUpdateEditorJob() and the IWorkspace.getRuleFactory().buildRule() as the scheduling rule.
Comment 9 Mark Christiaens CLA 2011-06-22 04:14:49 EDT
(In reply to comment #8)
> Hi Mark,
> 
> to verify your assumption, you could override
> DirtyStateEditorSupport.createUpdateEditorJob() and the
> IWorkspace.getRuleFactory().buildRule() as the scheduling rule.

I've added this scheduling constraint.  It's _slightly_ better.  Previously, when I had an open editor and did a clean build, the editor would lose its semantic highlighting.  Now, it does not.  

What's still broken is when I perform _two_ clean builds back to back while an editor is open.  So, I start a clean build and while that is running I start another one (that's of course waiting for the first).  Right when the second one starts, the editor again loses its semantic highlighting.

I suspect that semantic highlighting sees an empty global scope right before the second clean build but I'm still investigating.  

I would expect that there is a time-window during which the global index is empty or incomplete and in the process of being refilled.  During that period, the highlighter can't follow cross file references.  During that window, I see two sensible strategies:
- Let the highlighter run but re-trigger it once the global scope is rebuilt.
- Block the highlighter from running until the global scope is rebuilt.  

I think the first strategy is probably what Xtext is trying to do but I'm not sure.  If so, there's probably a refresh trigger missing somewhere.  That refresh would re-highlight the broken editor.
Comment 10 Mark Christiaens CLA 2011-06-22 05:43:48 EDT
I've been following the execution of the refresh of the editor after the second clean.  I see that a UpdateEditorStateJob is (re-)scheduled.  It doesn't do anything though.  I think the cause is that affectedResources is empty.  Digging deeper, the cause seems to be that collectAffectedResources filters out the current resource:

...
List<Resource> resources = resourceSet.getResources();
		for(int i = 0; i< resources.size(); i++) {
			Resource res = resources.get(i);
			if (res != resource && res != null) {
				URI uri = res.getURI();
...

The resource of the open editor is present in the event.getDeltas () but this filter blocks it from ending up in the result List.

I don't know why this filter is present.  I would be inclined to remove it?
Comment 11 Sebastian Zarnekow CLA 2011-06-22 06:08:25 EDT
The resource itself should not be contained in the list of affectedResources because these will be unloaded. You should make sure that isReparseRequired returns true, instead.
Comment 12 Mark Christiaens CLA 2011-06-22 09:17:48 EDT
I noticed that removing that filter is not a good idea :)  

I now added the following code which triggers a refresh of the editor after the second consecutive clean build.  Seems to do the trick.  (I also have the code in place that uses the builder's scheduling rule for the editor update).

In DirtyStateEditorSupport:

	@Override
	protected boolean isReparseRequired(XtextResource resource, Event event) {
		return super.isReparseRequired(resource, event) || isCleaned (resource, event);
	}

	private boolean isCleaned(XtextResource resource, Event event) {
		ImmutableList<Delta> deltas = event.getDeltas();
		ResourceSet resourceSet = resource.getResourceSet();
		URIConverter converter = resourceSet.getURIConverter();
		
		for (Delta delta : deltas) {
			if (delta.getOld() == null) {
				final URI newUri = delta.getNew().getURI();
				final URI normalizeNewUri = converter.normalize(newUri);
				if (normalizeNewUri.equals(resource.getURI())) {
					return true; 
				}
			}
		}
		
		return false;
	}
Comment 13 Sebastian Zarnekow CLA 2011-11-09 14:47:47 EST
Not 2.1