Summary: | [model] strange initialization dependency in OptionTests | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Stephan Herrmann <stephan.herrmann> | ||||||||||
Component: | Core | Assignee: | Stephan Herrmann <stephan.herrmann> | ||||||||||
Status: | VERIFIED FIXED | QA Contact: | |||||||||||
Severity: | normal | ||||||||||||
Priority: | P3 | CC: | amj87.iitr, jarthana, Olivier_Thomann | ||||||||||
Version: | 3.7 | ||||||||||||
Target Milestone: | 3.8 M2 | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Whiteboard: | |||||||||||||
Attachments: |
|
Description
Stephan Herrmann
2011-05-16 18:07:31 EDT
Created attachment 195797 [details]
Tweaks exposing the failure
This is how far I could get for today: apply this patch and the two-tests
setup will fail. This time I only disabled the following and only during the
first (unaffected!) test:
JavaModelManager.variables.put("JCL_LIB", variablePath);
JavaModelManager.variablePreferencesPut("JCL_LIB", variablePath)
This crippling produces the exact same effect I see when running
only the second test.
Re-enabling either statement repairs that second(!) test.
Jay, please investigate. Thanks! Those two tests and few others have been deprecated, by the way. (In reply to comment #3) > Those two tests and few others have been deprecated, by the way. Sorry I misunderstood the usage of @deprecated in the tests. Created attachment 201475 [details] proposed fix Today I saw more non-deterministic behavior of this bug: - PASS with Object Teams in the loop - FAIL with the current patch from bug 349326 After some more debugging I think I found the root cause for non-determinism: org.eclipse.jdt.internal.core.JavaProject.setOptions(Map) loops over the entries of the given Map, so this loop has non-deterministic order. This shouldn't normally hurt, but since bug 324987 JavaModelManager.storePreference(String, String, IEclipsePreferences) has a non-local side effect: if the given option is deprecated also the values for its compatible replacements are modified. Now guess what: if the deprecated option and its compatibles are both contained in the given map, then - if the deprecated option happens to be processed first all is well - if the deprecated option HAPPENS TO BE PROCESSED AFTER its compatible the replacement will implicitly overwrite an explicit value The patch fixes this by passing also the map into storePreferences so that we can avoid this kind of conflicting overwrites. Jay, does this all make sense? I'm currently working on a deterministic test case... if that is possible. Created attachment 201476 [details] deterministic test Here you go: this test uses a special Map with forced order from entrySet(). Deterministically fails without the patch from comment 5 and passes with that patch :) I will take a look at the patch and let you know. I have assigned this bug to you, meanwhile. (In reply to comment #6) > Created attachment 201476 [details] > deterministic test > > Here you go: this test uses a special Map with forced order from entrySet(). > > Deterministically fails without the patch from comment 5 and passes > with that patch :) Stephan, now I understand the fix and the patch looks good. However, this test always passes, with or without the patch. I will try and find out what is wrong. (In reply to comment #8) > However, this test > always passes, with or without the patch. I will try and find out what is > wrong. I see the option being removed (without the patch) but when it comes back to the test, the option is not null. Is it getting it from somewhere else, perhaps? Created attachment 202014 [details]
Improved and combined patch
I don't know what happened to my test, it didn't work because the
ForcedOrderMap didn't properly answer to containsKey() requests,
thus the testOptions map was seen as being empty
(see the for loop in JavaProject.setOptions(Map)).
After adding the missing method I can again reproduce the failure,
that is now reliably toggled by adding/removing the fix.
So, this is what I would release unless you see more issues, Jay?
(In reply to comment #10) > Created attachment 202014 [details] > Improved and combined patch > > I don't know what happened to my test, it didn't work because the > ForcedOrderMap didn't properly answer to containsKey() requests, > thus the testOptions map was seen as being empty > (see the for loop in JavaProject.setOptions(Map)). > > After adding the missing method I can again reproduce the failure, > that is now reliably toggled by adding/removing the fix. > > So, this is what I would release unless you see more issues, Jay? +1 to the patch. Released in HEAD for 3.8M2 Verified for 3.8M2. |