Bugzilla – Full Text Bug Listing
| Summary: | InvalidRegistryObjectException thrown when refreshingPackages | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Brian Lillie <brianlil> | ||||||||||||
| Component: | Compendium | Assignee: | equinox.compendium-inbox <equinox.compendium-inbox> | ||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||
| Severity: | major | ||||||||||||||
| Priority: | P3 | CC: | cbeth, jeffmcaffer, ob1.eclipse, pascal, raji, robbinsj | ||||||||||||
| Version: | 3.3 | Flags: | jeffmcaffer:
pmc_approved+
pascal: review+ dj.houghton: review+ |
||||||||||||
| Target Milestone: | 3.3.1 | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Windows XP | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 66317 [details]
test plugin (1 of 3) - ExtListener
Created attachment 66318 [details]
test plugin (2 of 3) - ExtPtUser
Created attachment 66319 [details]
test plugin (3 of 3) - ExtPtProvider
(Missing text from the description .. .should contain the following description of the plugins) ExtListener contains a listener class to listener to registry changes ExtPtProvider defines an extension point (Color), and an extension to that extension point (black) ExtPtUser defines an extension (purple) to the extension point (Color) Hi Brian, Thank you for the scenario to reproduce - it is perfect. This is a bug; thanks to your scenario I can duplicate it. At first I though this will be easy fix but the more time I spend on it, the more it became clear that there are quite a few issues in this area. First, the information on the "purple" extension has to be stored in the getAssociatedObjects() or earlier. Earlier because at that stage it has already being unlinked from the extension point and navigating from one to another might lead to inconsistent results. When I noticed another problem - we need to add configuration elements to all navigable elements we include in the deltas. Currently it is done for some extensions and not done for some extensions. Once that was hacked in I started to get exceptions in the part where we are processing bundle addition notifications. After some research it turned out that in your scenario we never remove "purple" extension from the orphan list and notification(s) generated for the bundle addition contain information on the "old" "purple" extension that has been uninstalled. Add to this a discovery that ExtensionRegistryDynamicTest that I tought was testing dynamic behavior have been disabled since at least 2004 :-). (And when I tried to use them, the listener infrastructure in those tests did not work.) In summary: ==> "Real" fix I strongly believe at this point that we need to revisit the registry notification mechanism. At present notification are processed asynchronously and we try to provide with each notification a picture of the registry the way it was when the event happened. This approach does not work very well. Providing picture of the past is rather expensive and error-prone. To make a proper picture we need to include all objects navigable from the modified extension/extension point along with configuration elements (that might need to be loaded from a disk cache). Linking/unlinking of extensions from extension points and presence of orphan mechanism further complicates this. We need to find out how people use notification mechanism and re-work it to make it both simple and reliable. The key here is to find what notification mechanisms are actually used for and, probably, provide to kinds of them: one would be synchronous notifications and another would be asynchronous notifications with limited information available in them. ==> Short-term solution The Eclipse 3.3 Release Candidate 1 is this week and, of course, we can't put a re-worked notification mechanism in it. Moreover, having in mind potential impact, I don't believe we should put it into 3.3 at this point. Two questions here for you: - When do you need this fixed? - What information do you really need from the notifications? If this is critical for 3.3, we *might* be able to put couple band-aid fixes for the specific scenario identified in this bug. Notice that this is *might*, no promises yet as the changes are not looking very straightforward so far. If it is not critical for 3.3, I'd prefer to come up with a proper plan on fixing registry notification mechanism, most likely resulting in a "version 2.0" of it. It would also be relevant to revisit the and better understand the set of usecase that are supposed to be supported. It may well be that the current support is perfectly adequate for those and that we have discovered some additional interesting scenarios. Of course, it is also possible that the whole thing is bogus... Oleg's focus on simple and safe things that can be done for 3.3 is entirely correct. The bug was generated as a result of attempting to apply feature changes immediately through the update OperationsManager.applyChangesNow(). That method performs and uninstall and reinstall of a large number of framework bundles, which generate a significant number of the InvalidRegistryObjectExceptions. Obviously, we are unable to make use of that call because of the number of problems that it generates. The information needed on the notifications will really depend on each and every one of the listeners that exist in the platform .. some of which are provided by product extensions to the platform, and others which are present in the eclipse platform itself. In order to be as dynamic as the underlying osgi framework would allow us to be, then we need to get the notification stuff right. We are currently shipping based on the 3.2 stream, so a fix in 3.3 is nice but we would like it back in 3.2 as well. The product deliverables are actually based on 3.2, so any fixes would be requested to be backported into the 3.2 as well. Created attachment 68125 [details]
Partial patch for 3.3
In order to minimize changes required late in the release cycle, the patch addresses only the original problem identified in the defect.
The patch does not address other problems in registry notifications we found while working on this bug (removed config element not being reachable from notifications; linking between extensions and extension points removed from the registry). I hope we'll return to registry notifications after 3.3 release to fix those problems.
Note we need at least two reviewers at this point... For now, setting target for 3.3 RC2 for the small patch. The patch is simple, but not a zero risk. From my view point it is 50/50 if this patch should go into 3.3RC2 or not. The patch looks good and is safe. However do we want to change this now? need to be really sure about fixes in this area... The patch is fine but I agree with the comments that we have to be careful in this area. If the patch is really only a partial patch and Oleg feels 50/50 about releasing it, then I would recommend waiting until we have a real solution. It feels too late for this kind of change in 3.3. We all agree that fixes around dynamic behaviour are needed but it would be better to do them in a considered and more complete way. We'll put this top of the list for 3.3.1. Created attachment 71371 [details]
Updated patch
Updated patch to include configuration elemetns of extensions being modified.
Know issues:
- Linking between removed extensions and extension points is inconsistent (for instance, IExtensionPoint.getExtensions() might provide only subset of extensions if extension point has been removed)
- Does not change the structure of registry deltas (it might be hard to figure out from the current notifications what exactly has been modified)
I would prefer to release the patch initially into 3.4 CVS stream [once it becomes available] and let it "polish" for a bit before backporting the patch into 3.3.x / 3.2.x.
Applied patch to CVS Head (Eclipse 3.4). The bug is kept open to back-port the patch into 3.3.1 after it gets some use in the 3.4 stream. I also created bug 193550 for the "ver 2.0" of the registry notification mechanism. The intention is that this bug will be tracking minimal changes to fix immediate problem; those minimal patches could then be back ported into previous version of Eclipse as needed. Patch applied to the 3.3 maintenance stream. |
Build ID: I20070503-1400 Steps To Reproduce: Upon refresh of a bundle, the following exception may occur 2007/05/08 10:23:04.511 SEVERE An internal error occurred during: "Registry event dispatcher". ::class.method=unknown :: thread=Worker-2 ::loggername=org.eclipse.core.jobs org.eclipse.core.runtime.InvalidRegistryObjectException: Invalid registry object at org.eclipse.core.internal.registry.TemporaryObjectManager.getObject(TemporaryObjectManager.java:98) at org.eclipse.core.internal.registry.BaseExtensionHandle.getExtension(BaseExtensionHandle.java:30) at org.eclipse.core.internal.registry.BaseExtensionHandle.getUniqueIdentifier(BaseExtensionHandle.java:67) at extlistener.Activator$MyListener.registryChanged(Activator.java:78) at org.eclipse.core.internal.registry.ExtensionRegistry.processChangeEvent(ExtensionRegistry.java:772) at org.eclipse.core.runtime.spi.RegistryStrategy.processChangeEvent(RegistryStrategy.java:246) at org.eclipse.core.internal.registry.osgi.EquinoxRegistryStrategy$ExtensionEventDispatcherJob.run(EquinoxRegist ryStrategy.java:94) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:58) from osgi console > install ExtListener plugin > install ExtPtProvider plugin > install ExtPtUser plugin > refresh ExtPtProvider (repeat until happens) The problem is in the ExtensionRegistry when plugins and the contributed extension points and extensions are removed When the ExtPtProvider is refreshed, Extension Deltas are created for the both the (black) and (purple). The associatedObjects map is built to contain the Color extension point, and black Extension are added. Once the associatedObjects map is constructed, the objects are removed from the registryObjects. The associatedObjects map is used as the TemporaryObjectManager, which is associated with the ExtensionDelta. >> The extension (purple) is not added to the associatedObjects map and therefore not in the TemporaryObjectManager. When the ExtPtProvider is refreshed, the ExtPtUser is also refreshed. If the Extension Registry removes for both of these plugins occurs prior to the RegistryChangeEvent listeners being called with the deltas, then the delta which refer to the extension (purple) will receive the InvalidRegistryObjectExtension since it is not available in the TemporaryObjectManager or in the master registry >> caveats: the exception may not always occur as it is timing related. the timing can be forced by setting a breakpoint in the ExtListener registryChangeListener and holding this thread until the contributor removals occur for both ExtPtUser and ExtPtProvider. A possible fix is in the RegistryObjectManager.addNavigableObjects to check if the object in the associatedObjects map is an ExtensionPoint, and if so, check the orphan list. Currently addNavigableObjects only adds the ExtensionPoint references (if available) into the associatedObjectsMap The following sequence shows the failing sequence of calls RefreshPackages thread ExtensionEventDispatcherJob t=0 ExtensionRegistry.basicRemove (ExtPtProvider) t=1 ExtensionRegistry.basicRemove (ExtPtUser) t=2 registryChanged If the registryChanged occurs prior to the basicRemove (ExtPtUser), then the InvalidRegistryObjectException will most likely not occur.