Community
Participate
Working Groups
To reproduce the problem, run the org.eclipse.gmf.tests.runtime.diagram.ui test suite and you will see several ConcurrentModificationException stack traces on the console. The DiagramEventBrokerThreadSage is asynchronously processing a ResourceSetChangeEvent, which it must not attempt to do, because the event is only valid during the listener call-back (see the ResourceSetChangeEvent javadoc for details). If the events are to be processed asynchronously, then they must first be copied into a new list. Otherwise, this exception occurs (which is bad) but, also, notifications will be missed by the broker (which is worse).
Created attachment 42044 [details] Patch to at least partially fix the DiagramEventBrokerThreadSafe Attached is a patch that updates the DiagramEventBrokerThreadSafe to clone the ResourceSetChangeEvent for asynchronous processing. This makes the stack traces in the JUnit tests go away, and ensures that events will not be missed. However, there are probably still problems that need to be considered: - by the time the async runnable is processed, the transaction referenced by the event may no longer be active. Any code that depends on this transaction could be invalid - by the time the async runnable is processed, other changes may also have occurred *on the UI thread* and been processed. This means that events will be processed out of order I recommend finding a way to asynchronously update the UI without asynchronously processing the model change notifications.
Thanks for the info. This issue exists for a very specific use case, where notifications are occurring on a worker thread and the display updates haven't been disabled (i.e. execution didn't originate from the OperationHistory). Ideally, I'd like to run these notifications synchronously on the main thread, but this inevitably seems to end in deadlock. Regarding the remaining problems: - is there a way for clients to query the validity of a transaction? - ordering is a risk, but this is handled at the resource change event, so aren't the events batched at this level?
Regarding validity of a transaction, what's valid depends on how a client uses the transaction. Most commonly, the transaction's ChangeDescription (obtained from the getChangeDescription() method) might be used by a listener to analyze the changes. This is really only subject to the same staleness problem as the Notification list. I guess a more serious problem, actually, is that asynchronous processing of the Notifications is not occurring in a read transaction: it is not in a transactional context at all, which breaks the transaction API's thread-safety for read access to the model. The ordering problem is not in the ordering of the Notifications within the ResourceSetChangeEvent. The problem is that this batch of notifications might be processed out of order with respect to another batch in another ResourceSetChangeEvent. Say that thread T1 makes changes in a write transaction and the UI thread also wants to make changes. The UI thread will wait for T1 to finish. When T1's transaction commits, the event broker schedules an asyncExec to process the changes. Before running the async runnable, the UI thread's transaction starts (it was waiting for T1 to commit) and when it commits, the event broker processes the UI's changes immediately. Then the asyncExec is executed and the event broker processes T1's changes, out of order.
So if we wrapped the asynchronous runnable inside a read transaction, that should at least make the contract consistent?
Created attachment 42071 [details] Updated patch that addresses read transaction issue Here's an updated patch that ensures a read transaction is open during the notification handling... Let me know what you think...
Oo ... that getEditingDomain.yield() call will be sure to throw IllegalStateException because the UI thread won't have an active transaction to yield. This would only work withing the runExclusive runnable, but it wouldn't be necessary anyway. Just remove the yield call. With that fix, the UI thread will at least have read access. Then, you will only have to evaluate the impact of processing Notifications that potentially do not correspond to the current state of the model because the model has changed in the mean-time.
Created attachment 42531 [details] updated patch - Removed yield as per advice. - Found serious issue where DiagramEditor wasn't removing listener from EditingDomain when closing. This was causing exception in JUnit when test was sending notification to previously closed Editor...
The model change risk is somewhat unavoidable...The usecase where this is exposed is very isolated and our hands are tied in terms of handling this differently.
Found flaw in DiagramEventBrokerThreadSafe since it is a private nested class of DiagramEditor. This means subsequent editors for the same editing domain will use this DiagramEventBrokerThreadSafe instance from the previous editor. - Probably causes memory leak of first editor - Problem that this is trying to solve is only active for first editor... Ignore proposed patch... need to go back to the drawing board.
Created attachment 42674 [details] updated patch Patch fixes: - memory leak - multiple editor handling - transaction contract
My testing is making me lose confidence in the asynchronous notification solution... I'm exploring a different tact, where the nofications still get executed on the worker thread, but updates are disabled while the notification is being handled.
Created attachment 43095 [details] new patch Current state of affairs: - New patch utilizes the PriviledgeRunnable from the transaction api in order to spawn the notifications synchronously. - Had to fix DiagramEditingDomainFactory to account for different class types of DiagramEventBroker - Disabled editing capability on diagram in main thread when there is an active transaction on a worker thread.
Committed changes... some minor updates from posted patch..
*** Bug 145439 has been marked as a duplicate of this bug. ***
[target cleanup] 1.0 RC4 was the original target milestone for this bug
[GMF Restructure] Bug 319140 : product GMF and component Runtime Diagram was the original product and component for this bug
