Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 326063 - org.eclipse.debug.core.IExpressionManager.getExpressions() is not thread safe that it causes java.lang.ArrayIndexOutOfBoundsException
Summary: org.eclipse.debug.core.IExpressionManager.getExpressions() is not thread safe...
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Debug (show other bugs)
Version: 3.6   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: 3.7 M3   Edit
Assignee: Darin Wright CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 328368
  Show dependency tree
 
Reported: 2010-09-23 10:23 EDT by Winnie Lai CLA
Modified: 2010-10-21 11:08 EDT (History)
2 users (show)

See Also:
pawel.1.piech: review+


Attachments
patch with tests (33.29 KB, patch)
2010-09-24 15:20 EDT, Darin Wright CLA
no flags Details | Diff
udpated with concurrent access test (34.37 KB, patch)
2010-09-24 15:42 EDT, Darin Wright CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Winnie Lai CLA 2010-09-23 10:23:41 EDT
Build Identifier: I20100608-0911

When one thread is adding expressions through IExpressionManager.addExpressions() or addExpression() and there is another thread that is getting expressions through IExpressionManager.getExpressions, I get the following exception

Caused by: java.lang.ArrayIndexOutOfBoundsException
	at java.lang.System.arraycopy(Native Method)
	at java.util.Vector.copyInto(Unknown Source)
	at org.eclipse.debug.internal.core.ExpressionManager.getExpressions(ExpressionManager.java:297)


Looking at the code of org.eclipse.debug.internal.core.ExpressionManager
	public IExpression[] getExpressions() {
		if (fExpressions == null) {
			return new IExpression[0];
		}
		IExpression[] temp= new IExpression[fExpressions.size()];
		fExpressions.copyInto(temp);
		return temp;
	}

I think the reason of the exception is that, after the array IExpression[] temp is created according to fExpressions.size(), there is another thread modify fExpressions through addExpression() that makes temp array size be smaller then fExpressons size. So, when the code 'fExpressions.copyInto(temp);' is executed, temp is not large enough to store the elements of fExpressions (see the Vector.copyInto documentation).

One solution I can think of is to synchronize the code a bit so that these two lines 
		IExpression[] temp= new IExpression[fExpressions.size()];
		fExpressions.copyInto(temp);
will not be affected by any change of fExpression size.


I cannot find any workaround to avoid this exception because the two individual pieces of code that call getExpressions and addExpression are from different language tools in eclipse, so I need to file this bug.



Reproducible: Always

Steps to Reproduce:
1.Create a thread T1 that is scheduled to addExpression() repeatedly forever.
2. Create another thread T2 that is scheduled to getExpressions() repeatedly forever.
3. Both T1 and T2 are using different timers to add / get expressions such that they are executed in their own pace.
Comment 1 Darin Wright CLA 2010-09-24 15:20:22 EDT
Created attachment 179538 [details]
patch with tests
Comment 2 Darin Wright CLA 2010-09-24 15:42:16 EDT
Created attachment 179539 [details]
udpated with concurrent access test
Comment 3 Darin Wright CLA 2010-09-24 15:42:50 EDT
Fixed. Please verify, Pawel.
Comment 4 Pawel Piech CLA 2010-09-30 20:08:33 EDT
This is a non trivial change!  On inspection it looks fine, but I'm glad it's not a last minute change in a release.
Comment 5 Winnie Lai CLA 2010-10-21 10:30:56 EDT
The fix looks ok to me as well. Thanks.
I know it is currently fixed in 3.7 stream.
I think 3.6 has published an update to 3.6.1 and I guess there will be 3.6.2.
Would it be possible to apply the fix for 3.6 stream so that the coming updates of 3.6 can use this fix as well? Does it make sense to the plans and schedules of updates?
Comment 6 Darin Wright CLA 2010-10-21 10:46:56 EDT
Unless it's critical, I'd like to leave this as a 3.7 fix... anytime we start playing with locks/synchronization, there is potential for breakage. Since the fix has been done early in 3.7, I expect that we'll work out any issues before 3.7 ships. With that said, 3.6.2 does ship in Feb, so if there are no issues by Jan, we might consider this as a 3.6.2 fix.
Comment 7 Winnie Lai CLA 2010-10-21 11:01:23 EDT
I understand your concern, and we all need time to shake out any potential locks and synchronization problems.

This is a critical fix for us because we will not be able to pick up any 3.7 milestones and we plans to pick up the final release of 3.7. Between now and the final release of 3.7, the only doable thing is to pick up 3.6.2.

So if 3.6.2 will be released in Feb, and no one reports problems on this fix, I would appreciate that the fix will be included in 3.6.2.
Since this bug is targeted to 3.7 M3, is there any way I can remind us for including the fix in 3.6.2 when everything is positive. I don't want to miss the boat.