Bugzilla – Full Text Bug Listing
| Summary: | Should use preference service API rather than plugin preferences in core.resources | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | James Blackburn <jamesblackburn+eclipse> | ||||||||||
| Component: | Resources | Assignee: | James Blackburn <jamesblackburn+eclipse> | ||||||||||
| Status: | CLOSED WONTFIX | QA Contact: | |||||||||||
| Severity: | major | ||||||||||||
| Priority: | P3 | CC: | dj.houghton | ||||||||||
| Version: | 3.7 | ||||||||||||
| Target Milestone: | --- | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Linux-GTK | ||||||||||||
| Whiteboard: | stalebug | ||||||||||||
| Bug Depends on: | 340993 | ||||||||||||
| Bug Blocks: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
James Blackburn
I think you should ping Dj about it. I'm trying to trace the history of the EclipsePreferences class but it is difficult since the version in HEAD has not only changed repositories, but also changed locations in the original repo since being created. But if you browse the changes between 1.56 and 1.57 in: org.eclipse.core.runtime/src/org/eclipse/core/internal/preferences/EclipsePreferences.java you will see that the fix to bug 80314 was to create a separate listener registry. But in HEAD that change seems to be missing and the code was reverted to each node keeping track of its own set of listeners. Ah, found the change. This was changed between versions 1.65 and 1.66 of the file. The CVS commit comment says it was related to bug 89262. We had a case where listeners were not GC'd when nodes were being removed and not added back. In my defense I made the change 6 years ago so no wonder I didn't remember. :-) I'm open to suggestions for alternative ways of keeping track of listeners. Thanks for the historical perspective DJ :) I'm bumping this to major, as a quick call-hierarchy code inspection (in the projects I've got in my workspace) shows some of the call-sites which have migrated to the non-deprecated API may be susceptible to this bug: - core.variables.StringVariableManager#initialize - debug.internal.core.Preferences#addPreferenceListener - ui.internal.WorkbenchPreferenceInitializer#initializeDefaultPreferences - ui.editors.DefaultEncodingSupport#initialize - cdt.IndexerPreferences#addListener - cdt.ui.CNavigatorContentProvider - cdt.ui.EclipsePreferencesAdapter#addPropertyChangeListener - cdt... (In reply to comment #2) > Ah, found the change. This was changed between versions 1.65 and 1.66 of the > file. The CVS commit comment says it was related to bug 89262. We had a case > where listeners were not GC'd when nodes were being removed and not added back. Interesting. To me it's not clear that listeners should need to be aware that their current node may disappear and may naively expect (as I did) that instance/org.eclipse.core.resources wouldn't disappear without the plugin being shutdown... It's also not obvious how to register a node change listener at the parent of instance scope. I guess as this has changed a couple of times, the behaviour is 'implementation defined'. One place that gets this right is org.eclipse.ui.internal.UIPreferenceInitializer. Unfortunately the amount of boiler plate here is pretty prohibitive: IEclipsePreferences rootNode = (IEclipsePreferences) Platform .getPreferencesService().getRootNode() .node(InstanceScope.SCOPE); final String uiName = UIPlugin.getDefault().getBundle() .getSymbolicName(); try { if (rootNode.nodeExists(uiName)) { ((IEclipsePreferences) rootNode.node(uiName)) .addPreferenceChangeListener(PlatformUIPreferenceListener .getSingleton()); } } catch (BackingStoreException e) { IStatus status = new Status(IStatus.ERROR, UIPlugin.getDefault() .getBundle().getSymbolicName(), IStatus.ERROR, e .getLocalizedMessage(), e); UIPlugin.getDefault().getLog().log(status); } rootNode.addNodeChangeListener(new IEclipsePreferences.INodeChangeListener() { /* * (non-Javadoc) * * @see * org.eclipse.core.runtime.preferences.IEclipsePreferences * .INodeChangeListener * #added(org.eclipse.core.runtime.preferences * .IEclipsePreferences.NodeChangeEvent) */ public void added(NodeChangeEvent event) { if (!event.getChild().name().equals(uiName)) { return; } ((IEclipsePreferences) event.getChild()) .addPreferenceChangeListener(PlatformUIPreferenceListener .getSingleton()); } /* * (non-Javadoc) * * @see * org.eclipse.core.runtime.preferences.IEclipsePreferences * .INodeChangeListener * #removed(org.eclipse.core.runtime.preferences * .IEclipsePreferences.NodeChangeEvent) */ public void removed(NodeChangeEvent event) { // Nothing to do here } }); } That's like 50+ additional lines (admittedly a lot of comment)! > In my defense I made the change 6 years ago so no wonder I didn't remember. :-) > > I'm open to suggestions for alternative ways of keeping track of listeners. I think listeners expect to be persisted and should be in control of their own lifecycle. While we could do something clever with references, this would just lead to arbitrary and random problems in the future. Certainly in the rest of the SDK we hold a strong reference to listeners. For example: the existing deprecated plugin preferences API, resource change listeners, etc. If there is huge churn on the nodes then it's possible leaked listeners from removed nodes could be a significant leak. Was there a use case behind bug 89262? There are a few solution I see: 1) Re-add the central root node registry 2) Ensure that the provided scopes: instance ; default ; configuration ; ... and their immediate children don't get removed and re-added as they correspond to the namespace root of the plugins (or fix them such that they persist their listeners across removal / creation)... 3) Add API to make listeners transitive: I've often wanted to listen to changes on org.eclipse.myplugin/my_node/... and children. If it was possible to register a transitive listener then I would only need to register one listener rather than a listener per child node => resulting in smaller listener lists. I can't remember the motivation but chances are that bug 89262 was created as a knee-jerk reaction to a potential problem rather than a real problem that was exhibited in the field. I might be inclined to add back the central registry and see how that goes. Created attachment 192044 [details]
patch
I think this patch ports all the changes back to keep track of all node and pref change listeners outside of the current node.
(In reply to comment #5) > Created attachment 192044 [details] > patch > > I think this patch ports all the changes back to keep track of all node and > pref change listeners outside of the current node. Looks good to me. Taking a quick look at the implementation, the ListenerRegistry map seems backed by an array. Won't this perform badly in a large eclipse as lookups will be O(n) in the size of the preference node namespace? Created attachment 192047 [details]
Back ListenerRegistry by HashMap
Simple patch to replace the custom map with a HashMap
As an aside, I pinned down where the listeners are getting lost. The '/instance' node is removed when preferences are imported... Daemon Thread [Thread-1] (Suspended (breakpoint at line 875 in EclipsePreferences)) InstancePreferences(EclipsePreferences).removeNode() line: 875 PreferencesService$1.visit(IEclipsePreferences) line: 140 ExportedPreferences(EclipsePreferences).accept(IPreferenceNodeVisitor) line: 106 ExportedPreferences(EclipsePreferences).accept(IPreferenceNodeVisitor) line: 110 PreferencesService.applyPreferences(IExportedPreferences) line: 196 PreferencesService.importPreferences(InputStream) line: 685 Preferences.importPreferences(IPath) line: 388 WorkspacePreferencesTest.testImportExport() line: 209 I'm not sure the original motivation for the array-based registry but the map-based one looks fine. Yeah that makes sense re: import being the one to remove the nodes. What test failures were you getting? I've modified the code locally to use the new APIs (and keep the existing per-node list of listeners) and I'm getting 6 problems. The first one looks interested and potentially related but the last 5 I would say are unrelated. (but they still fail... not sure why though) All Resources Tests org.eclipse.core.tests.resources.AutomatedTests org.eclipse.core.tests.resources.AllTests org.eclipse.core.tests.resources.CharsetTest testBug186984(org.eclipse.core.tests.resources.CharsetTest) org.eclipse.core.internal.resources.ResourceException: Resource is out of sync with the file system: '/7068aa01305a0010124df048ff7a14b2/file.xml'. at org.eclipse.core.internal.resources.File.getContentDescription(File.java:269) at org.eclipse.core.tests.resources.CharsetTest.testBug186984(CharsetTest.java:429) org.eclipse.core.tests.resources.regression.AllTests org.eclipse.core.tests.resources.regression.Bug_303517 testExists(org.eclipse.core.tests.resources.regression.Bug_303517) junit.framework.AssertionFailedError: 1.4 at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.assertTrue(Assert.java:20) at junit.framework.Assert.assertFalse(Assert.java:34) at org.eclipse.core.tests.resources.regression.Bug_303517.testExists(Bug_303517.java:79) testGetContents(org.eclipse.core.tests.resources.regression.Bug_303517) junit.framework.AssertionFailedError: 1.1 at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.assertTrue(Assert.java:20) at org.eclipse.core.tests.resources.regression.Bug_303517.testGetContents(Bug_303517.java:90) testIsSynchronized(org.eclipse.core.tests.resources.regression.Bug_303517) junit.framework.AssertionFailedError: 1.1 at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.assertTrue(Assert.java:20) at org.eclipse.core.tests.resources.regression.Bug_303517.testIsSynchronized(Bug_303517.java:156) testGetContentsTrue(org.eclipse.core.tests.resources.regression.Bug_303517) junit.framework.AssertionFailedError: 1.1 at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.assertTrue(Assert.java:20) at org.eclipse.core.tests.resources.regression.Bug_303517.testGetContentsTrue(Bug_303517.java:123) testChangeResourceGender(org.eclipse.core.tests.resources.regression.Bug_303517) junit.framework.AssertionFailedError: 1.1 at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.assertTrue(Assert.java:20) at org.eclipse.core.tests.resources.regression.Bug_303517.testChangeResourceGender(Bug_303517.java:177) (In reply to comment #9) > What test failures were you getting? Exactly the ones you've listed :) > I've modified the code locally to use the > new APIs (and keep the existing per-node list of listeners) and I'm getting 6 > problems. The first one looks interested and potentially related but the last 5 > I would say are unrelated. (but they still fail... not sure why though) All those tests are the ones I touched for Bug_303517 and rely on the preference being setup correctly -- relying on the listener FileSystemResourceManager getting poked correctly. See: Bug_303517#setup and testBug186984 itself. I'm guess if they're all failing there's something wrong with the wiring of the preference listener? Created attachment 192127 [details]
FSRM core.resources patch to use non-deprecated prefs API
The core.resources tests look good to me with the above patches applied.
Actually got a couple NPEs in SpecificContextTest: testIsAssociatedWith(org.eclipse.core.tests.resources.content.SpecificContextTest) java.lang.NullPointerException at org.eclipse.core.internal.preferences.EclipsePreferences.getNodeChangeListenerRegistry(EclipsePreferences.java:1166) at org.eclipse.core.internal.preferences.EclipsePreferences.fireNodeEvent(EclipsePreferences.java:668) at org.eclipse.core.internal.preferences.EclipsePreferences.internalNode(EclipsePreferences.java:543) at org.eclipse.core.internal.preferences.EclipsePreferences.node(EclipsePreferences.java:664) at org.eclipse.core.internal.content.ContentTypeSettings.getFileSpecs(ContentTypeSettings.java:49) at org.eclipse.core.internal.content.ContentType.hasFileSpec(ContentType.java:371) at org.eclipse.core.internal.content.ContentType.internalIsAssociatedWith(ContentType.java:470) at org.eclipse.core.internal.content.ContentType.isAssociatedWith(ContentType.java:520) at org.eclipse.core.internal.content.ContentTypeHandler.isAssociatedWith(ContentTypeHandler.java:156) at org.eclipse.core.tests.resources.content.SpecificContextTest.testIsAssociatedWith(SpecificContextTest.java:101) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) Created attachment 192143 [details]
core.resources.tests patch
The test with the NPE uses default EclipsePreferences constructor. Fixed up the test to provide listener registries.
(In reply to comment #10) > I'm guess if they're all failing there's something wrong with the wiring of the > preference listener? Sorry I misread your comment, I understand that when the tests failed you weren't using the central registry patch. This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. If the bug is still relevant please remove the stalebug whiteboard tag. |