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

Bug 323444

Summary:

[Undo] [Commands] java.util.ConcurrentModificationException when trying to get the undo history from a source viewer

Product:

[Eclipse Project] Platform

Reporter:

Shawn Minto <shawn.minto>

Component:

Assignee:

Susan McCourt <susan>

Status:

VERIFIED FIXED

QA Contact:

Susan McCourt <susan>

Severity:

normal

Priority:

CC:

pwebster, reckord

Version:

3.6

Target Milestone:

3.7 M5

Hardware:

OS:

Windows 7

Whiteboard:

Attachments:

Description	Flags
patch	susan: iplog+

Description Shawn Minto

2010-08-23 17:28:32 EDT

I got the following exception once when attempting to get the undo history from a source viewer in my code.  Looking at the implementation of this method, I cannot seem to see how this could occur since all access to this list in DefaultOperationHistory are synchronized.  Maybe it is the creation of the iterator outside of the synchronized block?  Note that I have been unable to reproduce this since I first saw it since it seems to be a rare race condition.

Exception Stack Trace:

java.util.ConcurrentModificationException
at java.util.AbstractList$Itr.checkForComodification(AbstractList.java:372)
at java.util.AbstractList$Itr.next(AbstractList.java:343)
at org.eclipse.core.commands.operations.DefaultOperationHistory.filter(DefaultOperationHistory.java:558)
at org.eclipse.core.commands.operations.DefaultOperationHistory.getUndoHistory(DefaultOperationHistory.java:843)

Comment 1 Shawn Minto

2010-08-24 18:19:17 EDT

Created attachment 177375 [details]
patch

Attached is a patch that should fix this problem.  The issue is that the iterator needs to be created inside of the synchronized block since it stores the modcount when it is created, which is the field that is used to determine if the was concurrent modification.

Comment 2 Susan McCourt

2010-12-15 16:44:36 EST

*** Bug 330258 has been marked as a duplicate of this bug. ***

Comment 3 Susan McCourt

2010-12-16 17:28:37 EST

I'm a little worried because the comments in bug 238397 suggest that this problem should have been fixed in that bug.  However, this exception has always been difficult to reproduce/force.  Given there have been two reports of the problem since the fix for bug 238397, I released the patch.  I verified that our test cases all still pass (including the multi-thread stress tests), though I don't have a way to force the modification exception and then prove that this fixes it.

All that said, I don't see harm in this fix and have released to HEAD >20101216.

Comment 4 Carsten Reckord

2010-12-17 04:54:52 EST

Hi Susan.

Just to ease your worries :) From what I can see in the patches and comments in bug 238397, they properly synchronized write access to the history. In those cases the synchronization gap in filter() is indeed no problem because the lock has already been acquired by all callers. However, those patches apparently missed the read access cases through getUndoHistory() and getRedoHistory() that call filter() without first acquiring a lock, thus opening the gap for this ConcurrentModificationException to happen.

Comment 5 Susan McCourt

2011-01-25 14:12:10 EST

marking this verified  I20110124-1800
- the code change is in the build
- the tests are all passing

but as stated above, we don't have a case that reliably reproduces this problem in order to verify it for sure.