Community
Participate
Working Groups
I might we wrong, but from the context it seem like org.eclipse.mylyn.internal.context.core.InteractionContext is using mostly-add for the collected events. If that is indeed the case, it should not be using CopyOnWriteArrayList to deal with concurrency issues, because that is too costly. See corresponding javadoc at http://java.sun.com/j2se/1.5.0/docs/api/java/util/concurrent/CopyOnWriteArrayList.html Here is fragment of thread dump I am seeing quite often (even when there is no active task). "Worker-390" prio=6 tid=0x2d6c1400 nid=0xb3c runnable [0x3681f000..0x3681fd14] java.lang.Thread.State: RUNNABLE at java.util.concurrent.locks.AbstractQueuedSynchronizer.release(AbstractQueuedSynchronizer.java:1175) at java.util.concurrent.locks.ReentrantLock.unlock(ReentrantLock.java:431) at java.util.concurrent.CopyOnWriteArrayList.add(CopyOnWriteArrayList.java:391) at org.eclipse.mylyn.internal.context.core.InteractionContext.parseEvent(InteractionContext.java:69) at org.eclipse.mylyn.internal.context.core.InteractionContextManager.addAttentionEvents(InteractionContextManager.java:192) at org.eclipse.mylyn.internal.context.core.InteractionContextManager.collapseActivityMetaContext(InteractionContextManager.java:280) at org.eclipse.mylyn.internal.context.core.InteractionContextManager.saveActivityMetaContext(InteractionContextManager.java:725) at org.eclipse.mylyn.internal.tasks.ui.ActivityExternalizationParticipant.execute(ActivityExternalizationParticipant.java:51) at org.eclipse.mylyn.internal.tasks.core.externalization.ExternalizationManager$ExternalizationJob.run(ExternalizationManager.java:200) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:55)
*** Bug 272091 has been marked as a duplicate of this bug. ***
I converted the CopyOnWriteArrayList to an ArrayList in my local workspace and haven't had any CME's. By looking at the implementation, I have confirmed that all of the operations to the InteractionContext are performed on the UI thread (i.e. locking is already performed) or the context is only being written to (i.e. collapsing of the activity context[1]). Since many handlers of interaction events (e.g. context listeners) assume that operations are performed on the UI thread, I think that it is safe to change this to ArrayList until the need for concurrency arises. Steffen, Mik, what are your thoughts? fn1. The only potential problem is with InteractionContext.getInteractionHistory(), but returning a copy of the interaction history would solve this concern.
How is this change going to affect InteractionContext.collapseActivityMetaContext()? As far as I can tell the method needs a copy of the interaction history and is not called on the UI thread so there is a chance of CMEs (even if a copy is made).
InteractionContextManager.collapseActivityMetaContext collapses one context into another. The original context is iterated over (need the copy so no CME when adding an event to the activity meta context), and the second one is added to, so there shouldn't be a CME from that. Let me know if I am wrong about this.
The crucial moment is when the copy is made. If the list is modified at that same moment you might end up with an inconsistent data structure.
Good point. I will look over this again and see if this is a real problem or not, or if we should synchronize the context.
Created attachment 134895 [details] proposed patch Steffen, here is my proposed patch that synchronizes all accesses to the interaction history. Is this what you had in mind?
Created attachment 134896 [details] mylyn/context/zip
Created attachment 134902 [details] updated patch to synchronize on all collections
Looks good. I have committed the attached patch which extends synchronization to all collections in InteractionContext to ensure that the state held in the class is always consistent.
