Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 358349

Summary:	[builder] QueuedBuildData missing synchronization
Product:	[Modeling] TMF	Reporter:	Mark Christiaens <mark.g.j.christiaens>
Component:	Xtext	Assignee:	Project Inbox <tmf.xtext-inbox>
Status:	NEW ---	QA Contact:
Severity:	normal
Priority:	P3	CC:	lieven.lemiengre, mark.g.j.christiaens, sebastian.zarnekow, sven.efftinge
Version:	2.0.1
Target Milestone:	---
Hardware:	PC
OS:	Linux
Whiteboard:

Description Mark Christiaens

2011-09-21 02:45:17 EDT

Build Identifier: 20110615-0604

I noticed that QueuedBuildData is "partially" synchronized: queueChanges is synchronized but queue isn't.  QueuedBuildData is a singleton that suggests that it might be accessed by multiple threads.  Don't know if there is additional synchronization that makes the use of QueuedBuildData safe.  In that case you can remove the synchronization entirely else, add more. 

Reproducible: Always

Comment 1 Sebastian Zarnekow

2011-09-21 02:49:48 EDT

Until now org.eclipse.xtext.builder.impl.QueuedBuildData.queueURI(URI) is only called from the builder context which uses a single thread and locks the entire working tree in the workspace. Therefore synchronization was not necessary, but it wouldn't hurt to add it.

Comment 2 Mark Christiaens

2011-09-21 02:54:26 EDT

Indeed,

And I didn't mention that there are more member functions that need synchronization.  I think getAndRemovePendingDeltas, isEmpty, getQueue, getAllRemainingURIs ... Anything that reads, writes the internal state.

Comment 3 Sven Efftinge

2011-09-21 03:00:17 EDT

I'd rather remove the existing synchronized keywords than adding them on any member beforehand. 
There is no point in accessing it during a build. Maybe in-between builds but then clients should synchronize using workspace locking.

Comment 4 Sebastian Zarnekow

2011-09-21 03:02:59 EDT

The present synchronization is necessary since these methods are called from JDT reconciling threads.

Comment 5 Sebastian Zarnekow

2011-09-21 03:05:50 EDT

(In reply to comment #4)
> The present synchronization is necessary since these methods are called from
> JDT reconciling threads.

That was to fast: I wanted to add that I don't think that a workspace lock is necessary just to push some URIs / Deltas into a queue.

Comment 6 Sven Efftinge

2011-09-21 03:16:00 EDT

Looks like a ConcurrentModificiationException could happen if 'queueChanges' is called while the builder iterates over 'getAllRemainingURIs'.

Comment 7 Sebastian Zarnekow

2011-09-21 03:38:11 EDT

I think it's safe for now since the builder itself holds a workspace lock and (at least that's what I assume) the reconcile will try to lock the reconciled resource. However, as soon as we load resources in parallel, we'll have to create a copy in #getAllRemainingURIs (which is probably even faster to iterate than the iterables.concat ...

Comment 8 Sven Efftinge

2011-09-21 04:02:30 EDT

(In reply to comment #7)
> I think it's safe for now since the builder itself holds a workspace lock and
> (at least that's what I assume) the reconcile will try to lock the reconciled
> resource.

Since the workspace is locked there shouldn't occur any changes triggering the JDT reconciler anyway.
So the synchronized keyword seems superfluous for the current scenario.

> However, as soon as we load resources in parallel, we'll have to
> create a copy in #getAllRemainingURIs (which is probably even faster to iterate
> than the iterables.concat ...

I didn't get that was the underlying use case. Makes sense now.

Comment 9 Lieven Lemiengre

2011-09-23 10:30:12 EDT

(In reply to comment #7)
> I think it's safe for now since the builder itself holds a workspace lock and
> (at least that's what I assume) the reconcile will try to lock the reconciled
> resource. However, as soon as we load resources in parallel, we'll have to
> create a copy in #getAllRemainingURIs (which is probably even faster to iterate
> than the iterables.concat ...

Comment 10 Sebastian Zarnekow

2011-11-09 14:51:43 EST

Not 2.1